Include component versions in webcompat reports #25688

vadimstruts · 2024-09-22T21:55:58Z

Resolves brave/brave-browser#35674

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 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Make sure that Webcompat Report contains the component versions information.
Steps:

Open Brave Shields pop up dialog
Switch off Brave Shields if It is ON
Click "Report broken site", then click "Submit"
Go to the webcompat reporter dashboard and see the component versions information.

Signed-off-by: Vadym Struts <[email protected]>

…d to add unittests Signed-off-by: Vadym Struts <[email protected]>

Signed-off-by: Vadym Struts <[email protected]>

Signed-off-by: vadym <[email protected]>

Signed-off-by: Vadym Struts <[email protected]>

goodov · 2024-10-11T15:00:19Z

browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.cc

@@ -50,12 +52,14 @@ namespace {
 constexpr char kUISourceHistogramName[] = "Brave.Webcompat.UISource";
 constexpr int kMaxScreenshotPixelCount = 1280 * 720;

+const std::string BoolToString(bool value) {


Suggested change

const std::string BoolToString(bool value) {

std::string BoolToString(bool value) {

goodov · 2024-10-15T09:10:16Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+content::BrowserContext*
+WebcompatReporterServiceFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextRedirectedInIncognito(context);


pls use ProfileKeyedServiceFactory configured for incognito appropriately.

now that you have ProfileKeyedServiceFactory, you need to remove GetBrowserContextToUse override.

goodov · 2024-10-15T09:11:27Z

browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.h

@@ -63,7 +63,8 @@ class WebcompatReporterDOMHandler : public content::WebUIMessageHandler {

  raw_ptr<content::RenderWidgetHostView> render_widget_host_view_;
  scoped_refptr<base::SequencedTaskRunner> ui_task_runner_;
-  std::unique_ptr<webcompat_reporter::WebcompatReportUploader> uploader_;
+  std::unique_ptr<webcompat_reporter::WebcompatReporterService>
+      reporter_service_;


is this variable used somehow or is it a leftover? It looks wrong, because you should never own a KeyedService.

it was leftover

goodov · 2024-10-15T09:12:13Z

browser/webcompat_reporter/webcompat_reporter_service_delegate.h

+
+ private:
+  raw_ptr<component_updater::ComponentUpdateService> component_update_service_;
+  raw_ptr<brave_shields::AdBlockService> adblock_service_;


const raw_ptr<...> both members.

goodov · 2024-10-15T09:15:39Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+
+// static
+mojo::PendingRemote<mojom::WebcompatReporterHandler>
+WebcompatReporterServiceFactory::GetForContext(


GetMojoReportHandlerForContext

goodov · 2024-10-15T09:44:35Z

ios/brave-ios/Sources/Brave/Frontend/Shields/SubmitReportView.swift

+        format: "%@ (%@)",
+        AppInfo.appVersion,
+        AppInfo.buildNumber
+      ),


channel is missing?

no, channel has separate property: "channel": "developer"
mentioned code helps to combine version value. ex: "version": "1.73 (0)"

goodov · 2024-10-15T09:45:55Z

ios/browser/api/webcompat_reporter/BUILD.gn

+  ]
+
+  deps = [
+    ":webcompat_reporter_mojom_wrappers",


shouldn't this be a public_deps instead?

goodov · 2024-10-15T09:47:06Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_delegate.h

+  std::optional<std::vector<ComponentInfo>> GetComponentInfos() const override;
+
+ private:
+  raw_ptr<component_updater::ComponentUpdateService> component_update_service_;


goodov · 2024-10-15T09:48:30Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_delegate.mm

+
+std::optional<std::vector<std::string>>
+WebcompatReporterServiceDelegateImpl::GetAdblockFilterListNames() const {
+  return std::nullopt;


is this accessible in some other way on iOS? If so, this should have a comment.

goodov · 2024-10-15T09:51:04Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_delegate.mm

+    return std::nullopt;
+  }
+
+  return result;


seems like this exact code is duplicated. Pls create a base delegate impl that will be shared on iOS and desktop/android.

Signed-off-by: Vadym Struts <[email protected]>

github-actions · 2024-10-16T18:32:35Z

The following commits were not verified:
f9ab08f (unsigned)

Signed-off-by: Vadym Struts <[email protected]>

...oid/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java

Signed-off-by: Vadym Struts <[email protected]>

AlexeyBarabash · 2024-10-16T20:52:59Z

...oid/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java

+    public WebcompatReporterHandler getWebcompatReporterHandler(
+            ConnectionErrorHandler connectionErrorHandler) {
+        Profile profile = Utils.getProfile(false); // Always use regular profile
+        if (profile == null) {


Utils.getProfile is marked as @NonNull and it tries to get the profile in several different ways, so I would not use the null check

AlexeyBarabash

Android Java code lgtm.

These imports look strange:

import org.chromium.chrome.browser.crypto_wallet.util.Utils;
at src/brave/android/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java

and

import org.chromium.chrome.browser.BraveRewardsHelper;
import org.chromium.chrome.browser.BraveRewardsNativeWorker;

at src/brave/android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java

And this is an issue beyond the current PR, crypto_wallet.util.Utils and BraveRewardsHelper are widely used beyond rewards/wallet.

Follow-up issues:
brave/brave-browser#41689
brave/brave-browser#41690

goodov · 2024-10-17T05:25:21Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory_wrapper.mm

+  auto service =
+      webcompat_reporter::WebcompatReporterServiceFactory::GetForBrowserState(
+          browserState);
+  if (!service) {


this is a mojo interface, not a keyed service.

I think the variable and the method name GetForBrowserState should be renamed accordingly to not confuse people.

goodov · 2024-10-17T05:27:52Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+content::BrowserContext*
+WebcompatReporterServiceFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextRedirectedInIncognito(context);


now that you have ProfileKeyedServiceFactory, you need to remove GetBrowserContextToUse override.

goodov · 2024-10-17T05:29:00Z

components/webcompat_reporter/browser/BUILD.gn

    "//brave/components/brave_shields/core/common:mojom",
    "//brave/components/brave_stats/browser",
    "//brave/components/constants",
    "//brave/components/version_info",
    "//brave/components/webcompat_reporter/buildflags",
-    "//content/public/browser",


let's also do "-content" in components/webcompat_reporter/browser/DEPS.

goodov · 2024-10-17T05:29:51Z

components/webcompat_reporter/browser/BUILD.gn

+  if (!is_ios) {
+    deps += [
+      "//brave/components/brave_shields/content/browser",
+      "//chrome/test:test_support",


what is it for? you shouldn't have //chrome in components.

goodov · 2024-10-17T05:43:34Z

components/webcompat_reporter/browser/webcompat_report_uploader.h

+  std::optional<std::string> brave_vpn_connected;
+  std::optional<std::string> details;
+  std::optional<std::string> contact;
+  std::optional<base::Value> ad_block_components;


you added a mojo structure that's just similar to this one. Do we really need to have both? Why not just use mojo structure everywhere from now on? Seems like the right thing to do.

Signed-off-by: Vadym Struts <[email protected]>

github-actions · 2024-10-17T22:44:29Z

[puLL-Merge] - brave/brave-core@25688

Description

This PR introduces a new WebcompatReporter service for reporting web compatibility issues in the Brave browser. The changes span across multiple platforms (Android, iOS, and desktop) and include the implementation of a new mojo interface for handling webcompat reports.

Possible Issues

The implementation differs slightly between platforms, which could lead to inconsistent behavior or maintenance challenges.
The PR introduces a significant amount of new code, which may increase the complexity of the codebase and the potential for bugs.

Security Hotspots

The WebcompatReporter service handles user-submitted data and screenshots, which could potentially contain sensitive information. Ensure proper sanitization and handling of this data.
The service interacts with network operations to submit reports, which could be a vector for data leaks if not properly secured.

Changes

Changes

Android:
- Added WebcompatReporterServiceFactory.java to handle the creation of the WebcompatReporter service.
- Modified BraveShieldsHandler.java to use the new WebcompatReporter service for submitting reports.
iOS:
- Added new files for WebcompatReporter service implementation, including WebcompatReporterServiceFactory and WebcompatReporterServiceDelegate.
- Updated WebcompatReporter.swift to use the new service for report submission.
Desktop/Common:
- Added new files for WebcompatReporter service implementation, including webcompat_reporter_service.cc, webcompat_reporter_service.h, and related test files.
- Created a new mojo interface webcompat_reporter.mojom for handling webcompat reports.
Build System:
- Updated various build files (GN) to include the new WebcompatReporter service components.
Testing:
- Added unit tests for the new WebcompatReporter service implementation.

Overall, this PR introduces a more unified and structured approach to handling webcompat reports across different platforms in the Brave browser.

github-actions bot assigned vadimstruts Sep 22, 2024

vadimstruts force-pushed the 35674-include-component-versions-webcompat-reports branch 3 times, most recently from 9a6a930 to dc56223 Compare October 2, 2024 11:54

vadimstruts marked this pull request as ready for review October 3, 2024 11:49

vadimstruts requested review from a team as code owners October 3, 2024 11:49

vadimstruts marked this pull request as draft October 3, 2024 20:11

vadimstruts added 14 commits October 10, 2024 14:52

adding mojo interface

6805d6d

Signed-off-by: Vadym Struts <[email protected]>

adding webcompat service

1fb1849

Signed-off-by: Vadym Struts <[email protected]>

added using of the same code for sending webcompat report

b58d7eb

Signed-off-by: Vadym Struts <[email protected]>

changed to use just common c++ uploader for desktop + android, starte…

85453bc

…d to add unittests Signed-off-by: Vadym Struts <[email protected]>

adding tests for webcompat service

dadc485

Signed-off-by: Vadym Struts <[email protected]>

adding tests for webcompat service

7211759

Signed-off-by: Vadym Struts <[email protected]>

adding tests for webcompat service

d9f67b6

Signed-off-by: Vadym Struts <[email protected]>

adding tests for webcompat service + fix format

9d0cdee

Signed-off-by: Vadym Struts <[email protected]>

fix format

9e91ef6

Signed-off-by: Vadym Struts <[email protected]>

fix format

9624dbd

Signed-off-by: Vadym Struts <[email protected]>

deps

75c48be

Signed-off-by: Vadym Struts <[email protected]>

fix unittest

d111490

Signed-off-by: Vadym Struts <[email protected]>

removed unneded callback

58897f2

Signed-off-by: Vadym Struts <[email protected]>

added iOS part

b54f416

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts force-pushed the 35674-include-component-versions-webcompat-reports branch from ea63de7 to b54f416 Compare October 10, 2024 12:55

vadimstruts and others added 7 commits October 10, 2024 21:53

added fix for deps

55200ad

Signed-off-by: Vadym Struts <[email protected]>

fix for dep #2

f9ab08f

Signed-off-by: vadym <[email protected]>

fix unittest

d8e5396

Signed-off-by: Vadym Struts <[email protected]>

fix format

90526f8

Signed-off-by: Vadym Struts <[email protected]>

fix ios build

0cfdd5f

Signed-off-by: Vadym Struts <[email protected]>

fix android build

9fb7f27

Signed-off-by: Vadym Struts <[email protected]>

removed unneeded comments

614e89e

Signed-off-by: Vadym Struts <[email protected]>

removed deps

0572ce7

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts marked this pull request as ready for review October 11, 2024 13:38

vadimstruts requested a review from a team as a code owner October 11, 2024 13:38

StephenHeaps self-requested a review October 11, 2024 14:03

goodov reviewed Oct 15, 2024

View reviewed changes

fixes according suggestions

af1c5a9

Signed-off-by: Vadym Struts <[email protected]>

github-actions bot added the puLL-Merge label Oct 16, 2024

vadimstruts added 3 commits October 16, 2024 22:07

fixes according suggestions

fa97d03

Signed-off-by: Vadym Struts <[email protected]>

fixes according suggestions

d5c79c8

Signed-off-by: Vadym Struts <[email protected]>

fixes according suggestions

8ef9d2a

Signed-off-by: Vadym Struts <[email protected]>

AlexeyBarabash reviewed Oct 16, 2024

View reviewed changes

...oid/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java Outdated Show resolved Hide resolved

removed unneeded comment

23f12db

Signed-off-by: Vadym Struts <[email protected]>

AlexeyBarabash reviewed Oct 16, 2024

View reviewed changes

AlexeyBarabash approved these changes Oct 16, 2024

View reviewed changes

goodov reviewed Oct 17, 2024

View reviewed changes

AlexeyBarabash mentioned this pull request Oct 17, 2024

org.chromium.chrome.browser.crypto_wallet.Utils.getProfile() should be moved to some common utils brave/brave-browser#41689

Open

fixed for review

a4125ba

Signed-off-by: Vadym Struts <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include component versions in webcompat reports #25688

Include component versions in webcompat reports #25688

vadimstruts commented Sep 22, 2024 •

edited

Loading

goodov Oct 11, 2024

goodov Oct 15, 2024

goodov Oct 17, 2024

goodov Oct 15, 2024

vadimstruts Oct 15, 2024

goodov Oct 15, 2024

goodov Oct 15, 2024

goodov Oct 15, 2024

vadimstruts Oct 16, 2024

goodov Oct 15, 2024

goodov Oct 15, 2024

goodov Oct 15, 2024

goodov Oct 15, 2024

github-actions bot commented Oct 16, 2024

AlexeyBarabash Oct 16, 2024

AlexeyBarabash left a comment •

edited

Loading

goodov Oct 17, 2024

goodov Oct 17, 2024

goodov Oct 17, 2024

goodov Oct 17, 2024

goodov Oct 17, 2024

github-actions bot commented Oct 17, 2024

Changes

	const std::string BoolToString(bool value) {
	std::string BoolToString(bool value) {

Include component versions in webcompat reports #25688

Are you sure you want to change the base?

Include component versions in webcompat reports #25688

Conversation

vadimstruts commented Sep 22, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024

Choose a reason for hiding this comment

AlexeyBarabash left a comment • edited Loading

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

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2024

Description

Possible Issues

Security Hotspots

Changes

vadimstruts commented Sep 22, 2024 •

edited

Loading

AlexeyBarabash left a comment •

edited

Loading