Add additional details to webcompat report #20052

DJAndries · 2023-09-07T19:51:05Z

Resolves brave/brave-browser#32661

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Use MITM proxy to ensure that all additional details are being appended to report.

brave-builds · 2023-09-07T20:55:10Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-09-07T21:58:40Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-09-07T22:53:28Z

A Storybook has been deployed to preview UI for the latest push

components/webcompat_reporter/browser/webcompat_report_uploader.cc

browser/ui/BUILD.gn

antonok-edm · 2023-09-16T00:25:35Z

browser/ui/webui/webcompat_reporter/webcompat_reporter_dialog.cc

+  params_dict.Set(kSiteURLField, report_url.spec());
+  params_dict.Set(kShieldsEnabledField, shields_enabled);
+  params_dict.Set(kAdBlockSettingField, GetAdBlockModeString(ad_block_mode));
+  params_dict.Set(kFPBlockSettingField,
+                  GetFingerprintModeString(fp_block_mode));


Could we define a struct for this data? I was going to let it slide since I figured it would be written directly to JSON, but that's not even the case 😛

Especially if we can expose that struct definition in a way so it can be shared with iOS, although it doesn't have to be in this PR.

could you maybe elaborate? this is kind of hacky, but the JSON is passed to the dialog via the webui params (which is JSON), and then those same params are passed back to the UI handler upon submission (also via JSON). I haven't found a way to pass a struct directly from the "open dialog" function to the UI handler.

Ah, I understand now. I was hoping we could have a custom struct as a constructor argument for the delegate, but the current one that takes base::Value just directly sets params_ from the inherited class. Nevermind!

yeah it's a shame. this is a pretty cringey way to pass data, to say the least....

antonok-edm · 2023-09-16T00:33:46Z

browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.cc

+    }
+  }
+
+  ad_block_list_names_ = base::JoinString(ad_block_list_names, ",");


I'd personally recommend using component IDs here for slightly easier machine-readability/consistency with the catalog in https://github.com/brave/adblock-resources (list names may change, infrequently). Then again, I'm not going to be the one looking at the raw report uploads 😄

happy to change this if it would be more useful. as you know, all of our webcompat reports are displayed on our dashboard in a human readable format, for manual review. i assumed it would be more useful for an adblock/easylist dev to see the name of the list in this case, but please correct me if i'm wrong since I will also not be looking at the reports! 😄 . curious to hear your thoughts on my perspective

I don't feel strongly about it. We'd be able to figure out what lists are referenced either way if reviewing raw upload data, and having the names directly available makes it easier to do so without any changes to the tooling.

The benefits of using the component IDs are

fixed size/format

constant identifier even if list names change

(1) is 100% a serverside implementation concern, so I have no preference there
(2) is slightly nicer for the sake of consistency in longer-term analysis, but we delete reports after 30 days anyways so it's not even too relevant either

Thanks for elaborating. If using names would make report reviews easier, then I would prefer to go that route. From a server-side perspective, it's much easier for us to store the names, as we will not have to maintain a mapping of component IDs to list names which would increase query complexity, risk for mapping inconsistencies, etc. etc. The size of the field is not a concern, considering the low volume of reports (compared to the rest of the stats data we manage). If you think that potential name changes won't cause too much trouble, then I hope to keep this as is

brave-builds · 2023-09-16T01:21:56Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2023-09-19T00:16:24Z

A Storybook has been deployed to preview UI for the latest push

components/resources/brave_components_strings.grd

mkarolin

strings++ (with a nit)

mkarolin

strings++

brave-builds · 2023-09-19T18:49:50Z

A Storybook has been deployed to preview UI for the latest push

pes10k

lgtm

pes10k · 2023-09-19T18:55:28Z

just checking @DJAndries , are any needed updates to the privacy policy complete?

And, will this affect android and desktop? Or just desktop?

DJAndries · 2023-09-19T20:17:27Z

And, will this affect android and desktop? Or just desktop?

For this PR, just desktop

pes10k · 2023-09-19T20:23:49Z

since most of this doesn't have a UI component, what is needed to enable it on android too?

DJAndries · 2023-09-19T21:24:21Z

since most of this doesn't have a UI component, what is needed to enable it on android too?

not too much, i think. the changes required will basically be a duplicate of this PR, but in Java. Raised brave/brave-browser#33126

pes10k · 2023-09-19T21:39:04Z

thanks thanks, though im surprised that Java is needed (again since there isn't a UI/UX component to the change) will follow up with the android experts. Thanks!

kjozwiak · 2023-10-04T01:05:37Z

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.60.76 Chromium: 118.0.5993.32 (Official Build) nightly (64-bit)
-- | --
Revision | c66376dd699f9327afd2f556c90b029bf4aa32e9
OS | Windows 11 Version 22H2 (Build 22621.2361)

installed/launched 1.60.76 Chromium: 118.0.5993.32
visited brave.com and disabled shields which displayed the Report a broken site UI within the shields panel
clicked on Report site and then submitted a report via Submit
ensured that the data mentioned via Add additional details to webcompat report brave-browser#32661 (comment) was present via https://webcompat.brave.com/1/webcompat

`Example`	`Example`	`Example`

Uplift of #20052 (squashed) to beta

bridiver · 2024-04-16T18:37:28Z

browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.cc

+
+  ad_block_list_names_ = base::JoinString(ad_block_list_names, ",");
+
+#if BUILDFLAG(ENABLE_BRAVE_VPN)


the UI code is not the correct place to put this because it's creating an unnecessary dependency from //brave/browser/ui -> //brave/browser/tor which already has deps issues and require check_includes = false. This should be encapsulated in another interface for report generation that only exposes a header target to //brave/browser/ui

bridiver · 2024-06-20T16:11:32Z

components/webcompat_reporter/browser/fields.cc

+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#include "brave/components/webcompat_reporter/browser/fields.h"


why is this file named fields? Also why does it even need to be public in the first place? It appears to only be called in WebcompatReporterDialog

DJAndries requested review from pes10k, cuba and antonok-edm September 7, 2023 19:51

DJAndries requested a review from a team as a code owner September 7, 2023 19:51

github-actions bot assigned DJAndries Sep 7, 2023

github-actions bot added potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false CI/storybook-url Deploy storybook and provide a unique URL for each build labels Sep 7, 2023

DJAndries force-pushed the webcompat-add-details branch from b2bab5c to 459c8ae Compare September 7, 2023 21:13

DJAndries force-pushed the webcompat-add-details branch from 459c8ae to 46d9fd6 Compare September 7, 2023 22:23

cuba reviewed Sep 13, 2023

View reviewed changes

components/webcompat_reporter/browser/webcompat_report_uploader.cc Show resolved Hide resolved

DJAndries force-pushed the webcompat-add-details branch from 46d9fd6 to 0ddec7c Compare September 16, 2023 00:18

antonok-edm reviewed Sep 16, 2023

View reviewed changes

DJAndries force-pushed the webcompat-add-details branch from 0ddec7c to 5e48387 Compare September 18, 2023 23:26

mkarolin reviewed Sep 19, 2023

View reviewed changes

components/resources/brave_components_strings.grd Outdated Show resolved Hide resolved

mkarolin approved these changes Sep 19, 2023

View reviewed changes

Add additional details to webcompat report

1d316b5

DJAndries force-pushed the webcompat-add-details branch from 5e48387 to 1d316b5 Compare September 19, 2023 18:19

mkarolin approved these changes Sep 19, 2023

View reviewed changes

antonok-edm approved these changes Sep 19, 2023

View reviewed changes

pes10k approved these changes Sep 19, 2023

View reviewed changes

DJAndries mentioned this pull request Sep 19, 2023

Add additional details to Android webcompat report brave/brave-browser#33126

Open

DJAndries merged commit adafb1f into master Sep 19, 2023
6 checks passed

DJAndries deleted the webcompat-add-details branch September 19, 2023 22:02

brave-builds added a commit that referenced this pull request Sep 19, 2023

Uplift of #20052 (squashed) to beta

c062037

brave-builds mentioned this pull request Sep 19, 2023

Add additional details to webcompat report (uplift to 1.59.x) #20224

Merged

7 tasks

kjozwiak pushed a commit that referenced this pull request Oct 4, 2023

Add additional details to webcompat report (uplift to 1.59.x) (#20224)

c905875

Uplift of #20052 (squashed) to beta

bridiver reviewed Apr 16, 2024

View reviewed changes

bridiver reviewed Jun 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional details to webcompat report #20052

Add additional details to webcompat report #20052

DJAndries commented Sep 7, 2023 •

edited

Loading

brave-builds commented Sep 7, 2023

brave-builds commented Sep 7, 2023

brave-builds commented Sep 7, 2023

antonok-edm Sep 16, 2023

DJAndries Sep 18, 2023

antonok-edm Sep 18, 2023

DJAndries Sep 18, 2023

antonok-edm Sep 16, 2023

DJAndries Sep 18, 2023 •

edited

Loading

antonok-edm Sep 18, 2023

DJAndries Sep 18, 2023 •

edited

Loading

brave-builds commented Sep 16, 2023

brave-builds commented Sep 19, 2023

mkarolin left a comment

mkarolin left a comment

brave-builds commented Sep 19, 2023

pes10k left a comment

pes10k commented Sep 19, 2023

DJAndries commented Sep 19, 2023

pes10k commented Sep 19, 2023

DJAndries commented Sep 19, 2023

pes10k commented Sep 19, 2023

kjozwiak commented Oct 4, 2023

bridiver Apr 16, 2024

bridiver Jun 20, 2024


		ad_block_list_names_ = base::JoinString(ad_block_list_names, ",");

		#if BUILDFLAG(ENABLE_BRAVE_VPN)

Add additional details to webcompat report #20052

Add additional details to webcompat report #20052

Conversation

DJAndries commented Sep 7, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Sep 7, 2023

brave-builds commented Sep 7, 2023

brave-builds commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJAndries Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJAndries Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

brave-builds commented Sep 16, 2023

brave-builds commented Sep 19, 2023

mkarolin left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

brave-builds commented Sep 19, 2023

pes10k left a comment

Choose a reason for hiding this comment

pes10k commented Sep 19, 2023

DJAndries commented Sep 19, 2023

pes10k commented Sep 19, 2023

DJAndries commented Sep 19, 2023

pes10k commented Sep 19, 2023

kjozwiak commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJAndries commented Sep 7, 2023 •

edited

Loading

DJAndries Sep 18, 2023 •

edited

Loading

DJAndries Sep 18, 2023 •

edited

Loading