Enable safe_browsing_api for muon #409

jumde · 2017-12-20T00:01:12Z

No description provided.

bridiver · 2017-12-21T18:33:38Z

atom/browser/browser.cc

@@ -156,6 +156,8 @@ void Browser::WillFinishLaunching() {

 void Browser::DidFinishLaunching(const base::DictionaryValue& launch_info) {
  is_ready_ = true;
+  sb_service = safe_browsing::SafeBrowsingService::CreateSafeBrowsingService();


I think this should go in chromium_src/chrome/browser/browser_process_impl.cc to match the implementation in src/chrome/browser/browser_process_impl.cc

I am working on the changes, I realized this yesterday. This is still in progress. Should've added the WIP tag to the header.

bridiver · 2017-12-21T18:36:00Z

chromium_src/BUILD.gn

@@ -290,6 +295,113 @@ source_set("browser") {
    "chrome/browser/content_settings/host_content_settings_map_factory.cc",
    "chrome/browser/content_settings/host_content_settings_map_factory.h",

+    "//chrome/browser/renderer_preferences_util.cc",


can we add //chrome/browser/safe_browsing as a dep instead?

bridiver · 2017-12-21T18:37:12Z

chromium_src/chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc

@@ -0,0 +1,60 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.


why are these copied instead of using chrome src? Are there differences?

bridiver

see comments

bridiver · 2017-12-21T19:29:43Z

I added the wip label, so just remove that when you're ready to review again

bridiver · 2018-01-04T23:00:22Z

atom/browser/BUILD.gn

    ":importer",
    "//electron/vendor/ad-block/muon:ad_block",
    "//electron/vendor/tracking-protection/muon:tp_node_addon",
  ]

  sources = [
+    "//chrome/browser/loader/safe_browsing_resource_throttle.h",


these should go in chromium_src/BUILD.gn prerender target

bridiver · 2018-01-04T23:00:56Z

atom/browser/BUILD.gn

    "//electron:common",
    "//electron/muon/app",
    # @todo(bridiver) fix circular dep
    # "//electron/muon/browser",
    "//base",
+    "//chrome/browser:resource_prefetch_predictor_proto",


these should be deps of the the chromium_src prerender target

bridiver · 2018-01-04T23:02:08Z

atom/browser/BUILD.gn

@@ -21,22 +21,29 @@ source_set("browser") {
  ]

  deps = [
+    "../../chromium_src/chrome/browser/prerender",


deps should always absolute paths //electron/chromium_src...

bridiver · 2018-01-04T23:02:58Z

chromium_src/BUILD.gn

+    "//chrome/browser/renderer_preferences_util.cc",
+    "//chrome/browser/renderer_preferences_util.h",
+
+    "//chrome/browser/safe_browsing/chunk_range.cc",


I would put these in a separate safe_browsing target and make that a dep of browser

bridiver · 2018-01-19T07:27:18Z

atom/browser/atom_download_manager_delegate.cc

+
+SafeBrowsingState::~SafeBrowsingState() {}
+
+void CheckDownloadUrlDone(


seems like we're copying a lot of code from src/chrome here which we really try to avoid. Can we subclass or use some other method to reuse the existing code?

Most of the code is similar to chrome_download_manager_delegate.cc. I optimized some of it, let me know if you think we can optimize more.

bridiver · 2018-01-19T07:30:06Z

patches/master_patch.patch

+
+   next_state_ = STATE_RESERVE_VIRTUAL_PATH;
+-
+-  if (!should_notify_extensions_ ||


can you provide some explanation for these? This is adding a lot of patches and it doesn't seem like they should all be necessary

bridiver · 2018-01-19T07:32:09Z

muon/browser/muon_browser_process_impl.cc

@@ -58,9 +93,20 @@ MuonBrowserProcessImpl::component_updater() {
  return component_updater(component_updater_, false);
 }

+void MuonBrowserProcessImpl::ApplyAllowCrossOriginAuthPromptPolicy() {


is this part of safe browsing?

bridiver · 2018-01-19T07:34:03Z

chromium_src/BUILD.gn

@@ -599,6 +612,144 @@ source_set("browser") {
  }
 }

+source_set("safe_browsing") {
+  sources = [
+    "//chrome/browser/download/download_completion_blocker.cc",


safe browsing has an existing target in chrome so I don't think we should have to add all these files individually

bridiver · 2018-03-02T01:14:31Z

brave/browser/brave_content_browser_client.cc

@@ -658,7 +658,9 @@ void BraveContentBrowserClient::OverrideWebkitPrefs(
  prefs->hyperlink_auditing_enabled = false;
  // Custom preferences of guest page.
  auto web_contents = content::WebContents::FromRenderViewHost(host);
-  atom::WebContentsPreferences::OverrideWebkitPrefs(web_contents, prefs);
+  if (web_contents) {


when is this null?

bridiver · 2018-03-02T01:15:43Z

brave/browser/password_manager/brave_password_manager_client.cc

@@ -395,8 +395,11 @@ void BravePasswordManagerClient::CheckSafeBrowsingReputation(
    const GURL& form_action,
    const GURL& frame_url) {}

+void BravePasswordManagerClient::LogPasswordReuseDetectedEvent() {}


? cc @darkdh

bridiver · 2018-03-02T01:20:23Z

atom/browser/api/atom_api_download_item.cc

 #include "native_mate/dictionary.h"
 #include "net/base/filename_util.h"

-#include "atom/common/node_includes.h"


this is intentionally added at the end of the list

bridiver · 2018-03-02T01:21:47Z

atom/browser/extensions/atom_extensions_browser_client.cc

@@ -67,12 +67,10 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/api/cryptotoken_private/cryptotoken_private_api.h"
-#include "extensions/common/file_util.h"


these are also here intentionally to group them with other URLResourceRequestBundleJob headers

it's best not to mix in unrelated changes because it just confuses the purpose of the PR. Unrelated changes should be put in a separate PR

bridiver · 2018-03-12T20:49:52Z

patches/master_patch.patch

+ #define DUMMY_API_TOKEN "dummytoken"
+
+ #if !defined(GOOGLE_API_KEY)
+-#define GOOGLE_API_KEY DUMMY_API_TOKEN


we don't want to hard code keys like this, what is the purpose of this change?

bridiver · 2018-03-12T21:42:50Z

chromium_src/chrome/browser/safe_browsing/BUILD.gn

@@ -1,19 +1,44 @@
+source_set("install_verification") {
+  sources = [


same thing with source set configs here

bridiver · 2018-03-12T21:44:27Z

patches/master_patch.patch

+   danger_level_ = GetDangerLevel(
+       visited_referrer_before ? VISITED_REFERRER : NO_VISITS_TO_REFERRER);
+   if (danger_level_ != DownloadFileType::NOT_DANGEROUS &&
+      danger_level_ != DownloadFileType::ALLOW_ON_USER_GESTURE &&


can you explain this change? We can set the user gesture just like chrome does

bridiver · 2018-03-12T21:44:49Z

patches/master_patch.patch

+     return false;
+   }
+   // This check should be last, so we know the earlier checks passed.
+#if defined(MUON_CHROMIUM_BUILD)


also why this change?

bridiver · 2018-03-20T19:45:31Z

muon/browser/muon_browser_process_impl.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/io_thread.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"


bridiver · 2018-03-20T19:47:57Z

chromium_src/chrome/browser/ui/browser.cc

@@ -52,6 +52,10 @@ Browser::CreateParams::CreateParams(Type type, Profile* profile)

 Browser::CreateParams::CreateParams(const CreateParams& other) = default;

+content::WebContents* Browser::OpenURL(const content::OpenURLParams& params) {
+  return NULL;


I'm a little worried about this one causing a crash. Where does it get called from? Is it just windows doing its usual terrible job of dead code elimination or is there an actual legit code path that leads here?

bridiver · 2018-03-29T01:52:00Z

atom/browser/atom_resource_dispatcher_host_delegate.cc

+                                  resource_context,
+                                  resource_type,
+                                  throttles);
+#if BUILDFLAG(ENABLE_NACL)


I don't think this block should be here

bridiver

++

bridiver reviewed Dec 21, 2017

View reviewed changes

bridiver suggested changes Dec 21, 2017

View reviewed changes

jumde changed the title ~~Enable safe_browsing_api for muon~~ [WIP] Enable safe_browsing_api for muon Dec 21, 2017

bridiver added the PR/work-in-progress ⚒ label Dec 21, 2017

jumde force-pushed the safe_browsing_api branch 5 times, most recently from 7671fc4 to e3896f2 Compare December 27, 2017 20:42

jumde force-pushed the safe_browsing_api branch 4 times, most recently from aac0a4d to 5046ff0 Compare January 2, 2018 17:50

bridiver reviewed Jan 4, 2018

View reviewed changes

jumde force-pushed the safe_browsing_api branch from f3d07e6 to 2e34198 Compare January 5, 2018 03:29

jumde force-pushed the safe_browsing_api branch 2 times, most recently from 03257a7 to c6d2aa0 Compare January 18, 2018 22:13

jumde requested review from diracdeltas, tildelowengrimm and darkdh January 19, 2018 02:28

bridiver reviewed Jan 19, 2018

View reviewed changes

bridiver reviewed Mar 2, 2018

View reviewed changes

darkdh mentioned this pull request Mar 5, 2018

Using DIR_DEFAULT_DOWNLOADS_SAFE #508

Merged

jumde force-pushed the safe_browsing_api branch from aa96a6e to 5817092 Compare March 5, 2018 21:45

bridiver reviewed Mar 12, 2018

View reviewed changes

jumde force-pushed the safe_browsing_api branch from 115f4ec to 7198582 Compare March 20, 2018 18:06

bridiver reviewed Mar 20, 2018

View reviewed changes

brave deleted a comment from jumde Mar 20, 2018

bridiver reviewed Mar 20, 2018

View reviewed changes

jumde added the PR/work-in-progress ⚒ label Mar 21, 2018

jumde force-pushed the safe_browsing_api branch from 7198582 to d871145 Compare March 22, 2018 21:53

jumde removed the PR/work-in-progress ⚒ label Mar 22, 2018

darkdh mentioned this pull request Mar 22, 2018

Edit download path doesn't show the current download path brave/browser-laptop#13570

Closed

jumde force-pushed the safe_browsing_api branch 2 times, most recently from 974749e to 66439d7 Compare March 28, 2018 03:46

bridiver reviewed Mar 29, 2018

View reviewed changes

jumde force-pushed the safe_browsing_api branch 2 times, most recently from c61e04d to 8e83a50 Compare March 29, 2018 03:42

bridiver previously approved these changes Mar 30, 2018

View reviewed changes

Enable Safe Browsing api for muon

3eb006d

jumde dismissed bridiver’s stale review via 3eb006d March 30, 2018 19:34

jumde force-pushed the safe_browsing_api branch from 9c28546 to 3eb006d Compare March 30, 2018 19:34

bridiver approved these changes Mar 30, 2018

View reviewed changes

bridiver merged commit 213d1cf into master Mar 30, 2018

bsclifton deleted the safe_browsing_api branch June 18, 2018 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable safe_browsing_api for muon #409

Enable safe_browsing_api for muon #409

jumde commented Dec 20, 2017

bridiver Dec 21, 2017

jumde Dec 21, 2017

bridiver Dec 21, 2017

bridiver Dec 21, 2017

bridiver left a comment

bridiver commented Dec 21, 2017

bridiver Jan 4, 2018

bridiver Jan 4, 2018

bridiver Jan 4, 2018

bridiver Jan 4, 2018

bridiver Jan 19, 2018

jumde Jan 23, 2018

bridiver Jan 19, 2018

bridiver Jan 19, 2018

bridiver Jan 19, 2018

bridiver Mar 2, 2018

bridiver Mar 2, 2018

bridiver Mar 2, 2018

bridiver Mar 2, 2018

bridiver Mar 2, 2018

bridiver Mar 12, 2018

bridiver Mar 12, 2018

bridiver Mar 12, 2018

bridiver Mar 12, 2018

bridiver Mar 20, 2018

bridiver Mar 20, 2018

bridiver Mar 29, 2018 •

edited

Loading

bridiver left a comment

		@@ -0,0 +1,60 @@
		// Copyright 2014 The Chromium Authors. All rights reserved.


		SafeBrowsingState::~SafeBrowsingState() {}

		void CheckDownloadUrlDone(

		@@ -1,19 +1,44 @@
		source_set("install_verification") {
		sources = [

Enable safe_browsing_api for muon #409

Enable safe_browsing_api for muon #409

Conversation

jumde commented Dec 20, 2017

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

bridiver left a comment

Choose a reason for hiding this comment

bridiver commented Dec 21, 2017

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

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

bridiver Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

bridiver Mar 29, 2018 •

edited

Loading