Add additional details to webcompat report #20052
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| /* Copyright (c) 2023 The Brave Authors. All rights reserved. | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * 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/browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.h" | ||
|
|
||
| #include <memory> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| #include "base/strings/string_util.h" | ||
| #include "brave/browser/brave_browser_process.h" | ||
| #include "brave/browser/ui/brave_shields_data_controller.h" | ||
| #include "brave/browser/ui/webui/brave_webui_source.h" | ||
| #include "brave/components/brave_shields/browser/ad_block_regional_service_manager.h" | ||
| #include "brave/components/brave_shields/browser/ad_block_service.h" | ||
| #include "brave/components/brave_shields/browser/filter_list_catalog_entry.h" | ||
| #include "brave/components/brave_shields/common/pref_names.h" | ||
| #include "brave/components/brave_vpn/common/buildflags/buildflags.h" | ||
| #include "brave/components/constants/webui_url_constants.h" | ||
| #include "brave/components/webcompat_reporter/browser/fields.h" | ||
| #include "brave/components/webcompat_reporter/browser/webcompat_report_uploader.h" | ||
| #include "brave/components/webcompat_reporter/resources/grit/webcompat_reporter_generated_map.h" | ||
| #include "chrome/browser/profiles/profile.h" | ||
| #include "components/grit/brave_components_resources.h" | ||
| #include "components/language/core/browser/pref_names.h" | ||
| #include "components/prefs/pref_service.h" | ||
| #include "content/public/browser/web_ui_data_source.h" | ||
| #include "content/public/browser/web_ui_message_handler.h" | ||
| #include "services/network/public/cpp/shared_url_loader_factory.h" | ||
| #include "ui/web_dialogs/web_dialog_delegate.h" | ||
| #include "url/gurl.h" | ||
|
|
||
| #if BUILDFLAG(ENABLE_BRAVE_VPN) | ||
| #include "brave/browser/brave_vpn/brave_vpn_service_factory.h" | ||
| #include "brave/components/brave_vpn/browser/brave_vpn_service.h" | ||
| #endif | ||
|
|
||
| namespace webcompat_reporter { | ||
|
|
||
| namespace { | ||
|
|
||
| class WebcompatReporterDOMHandler : public content::WebUIMessageHandler { | ||
| public: | ||
| explicit WebcompatReporterDOMHandler(Profile* profile); | ||
| WebcompatReporterDOMHandler(const WebcompatReporterDOMHandler&) = delete; | ||
| WebcompatReporterDOMHandler& operator=(const WebcompatReporterDOMHandler&) = | ||
| delete; | ||
| ~WebcompatReporterDOMHandler() override; | ||
|
|
||
| // WebUIMessageHandler implementation. | ||
| void RegisterMessages() override; | ||
|
|
||
| private: | ||
| void InitAdditionalParameters(Profile* profile); | ||
| void HandleSubmitReport(const base::Value::List& args); | ||
|
|
||
| std::unique_ptr<webcompat_reporter::WebcompatReportUploader> uploader_; | ||
| std::string ad_block_list_names_; | ||
| std::string languages_; | ||
| bool language_farbling_enabled_ = true; | ||
| bool brave_vpn_connected_ = false; | ||
| }; | ||
|
|
||
| WebcompatReporterDOMHandler::WebcompatReporterDOMHandler(Profile* profile) | ||
| : uploader_(std::make_unique<webcompat_reporter::WebcompatReportUploader>( | ||
| profile->GetURLLoaderFactory())) { | ||
| InitAdditionalParameters(profile); | ||
| } | ||
|
|
||
| void WebcompatReporterDOMHandler::InitAdditionalParameters(Profile* profile) { | ||
| std::vector<std::string> ad_block_list_names; | ||
|
|
||
| // Collect all enabled adblock list names | ||
| brave_shields::AdBlockService* ad_block_service = | ||
| g_brave_browser_process->ad_block_service(); | ||
| if (ad_block_service != nullptr) { | ||
| brave_shields::AdBlockRegionalServiceManager* regional_service_manager = | ||
| ad_block_service->regional_service_manager(); | ||
| CHECK(regional_service_manager); | ||
| for (const brave_shields::FilterListCatalogEntry& entry : | ||
| regional_service_manager->GetFilterListCatalog()) { | ||
| if (regional_service_manager->IsFilterListEnabled(entry.uuid)) { | ||
| ad_block_list_names.push_back(entry.title); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ad_block_list_names_ = base::JoinString(ad_block_list_names, ","); | ||
|
There was a problem hiding this comment. 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 😄 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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
(1) is 100% a serverside implementation concern, so I have no preference there There was a problem hiding this comment. 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 |
||
|
|
||
| #if BUILDFLAG(ENABLE_BRAVE_VPN) | ||
|
There was a problem hiding this comment. the UI code is not the correct place to put this because it's creating an unnecessary dependency from |
||
| brave_vpn::BraveVpnService* vpn_service = | ||
| brave_vpn::BraveVpnServiceFactory::GetForProfile(profile); | ||
| if (vpn_service != nullptr) { | ||
| brave_vpn_connected_ = vpn_service->IsConnected(); | ||
| } | ||
| #endif | ||
|
|
||
| PrefService* profile_prefs = profile->GetPrefs(); | ||
| languages_ = profile_prefs->GetString(language::prefs::kAcceptLanguages); | ||
| language_farbling_enabled_ = | ||
| profile_prefs->GetBoolean(brave_shields::prefs::kReduceLanguageEnabled); | ||
| } | ||
|
|
||
| WebcompatReporterDOMHandler::~WebcompatReporterDOMHandler() = default; | ||
|
|
||
| void WebcompatReporterDOMHandler::RegisterMessages() { | ||
| web_ui()->RegisterMessageCallback( | ||
| "webcompat_reporter.submitReport", | ||
| base::BindRepeating(&WebcompatReporterDOMHandler::HandleSubmitReport, | ||
| base::Unretained(this))); | ||
| } | ||
|
|
||
| void WebcompatReporterDOMHandler::HandleSubmitReport( | ||
| const base::Value::List& args) { | ||
| DCHECK_EQ(args.size(), 1U); | ||
| if (!args[0].is_dict()) { | ||
| return; | ||
| } | ||
|
|
||
| const base::Value::Dict& submission_args = args[0].GetDict(); | ||
|
|
||
| const std::string* url_arg = submission_args.FindString(kSiteURLField); | ||
| const std::string* ad_block_setting_arg = | ||
| submission_args.FindString(kAdBlockSettingField); | ||
| const std::string* fp_block_setting_arg = | ||
| submission_args.FindString(kFPBlockSettingField); | ||
| const base::Value* details_arg = submission_args.Find(kDetailsField); | ||
| const base::Value* contact_arg = submission_args.Find(kContactField); | ||
| bool shields_enabled = | ||
| submission_args.FindBool(kShieldsEnabledField).value_or(false); | ||
|
|
||
| std::string url; | ||
| std::string ad_block_setting; | ||
| std::string fp_block_setting; | ||
| base::Value details; | ||
| base::Value contact; | ||
|
|
||
| if (url_arg != nullptr) { | ||
| url = *url_arg; | ||
| } | ||
| if (ad_block_setting_arg != nullptr) { | ||
| ad_block_setting = *ad_block_setting_arg; | ||
| } | ||
| if (fp_block_setting_arg != nullptr) { | ||
| fp_block_setting = *fp_block_setting_arg; | ||
| } | ||
| if (details_arg != nullptr) { | ||
| details = details_arg->Clone(); | ||
| } | ||
| if (contact_arg != nullptr) { | ||
| contact = contact_arg->Clone(); | ||
| } | ||
|
|
||
| uploader_->SubmitReport(GURL(url), shields_enabled, ad_block_setting, | ||
| fp_block_setting, ad_block_list_names_, languages_, | ||
| language_farbling_enabled_, brave_vpn_connected_, | ||
| details, contact); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| WebcompatReporterUI::WebcompatReporterUI(content::WebUI* web_ui, | ||
| const std::string& name) | ||
| : ConstrainedWebDialogUI(web_ui) { | ||
| CreateAndAddWebUIDataSource(web_ui, name, kWebcompatReporterGenerated, | ||
| kWebcompatReporterGeneratedSize, | ||
| IDR_WEBCOMPAT_REPORTER_HTML); | ||
| Profile* profile = Profile::FromWebUI(web_ui); | ||
|
|
||
| web_ui->AddMessageHandler( | ||
| std::make_unique<WebcompatReporterDOMHandler>(profile)); | ||
| } | ||
|
|
||
| WebcompatReporterUI::~WebcompatReporterUI() = default; | ||
|
|
||
| } // namespace webcompat_reporter | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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::Valuejust directly setsparams_from the inherited class. Nevermind!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it's a shame. this is a pretty cringey way to pass data, to say the least....