Upgrade to Chromium 70.0.3528.4 #358

bbondy · 2018-08-24T18:53:38Z

Fix brave/brave-browser#834

Related: brave/brave-browser#835

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

bbondy · 2018-08-24T19:29:15Z

This is ready for review

bridiver · 2018-08-24T20:07:58Z

DEPS

@@ -10,7 +10,7 @@ deps = {
  "vendor/boto": "https://github.com/boto/boto@f7574aa6cc2c819430c1f05e9a1a1a666ef8169b",
  "vendor/python-patch": "https://github.com/svn2github/python-patch@a336a458016ced89aba90dfc3f4c8222ae3b1403",
  "vendor/sparkle": "https://github.com/brave/Sparkle.git@c0759cce415d7c0feae45005c8a013b1898711f0",
-  "vendor/bat-native-ledger": "https://github.com/brave-intl/bat-native-ledger@a3e64797bf6ebbfa39a60240db8d82995f97ebd9",
+  "vendor/bat-native-ledger": "https://github.com/brave-intl/bat-native-ledger@123d1dec129528390eaf583d04e5dc2b6c426713",


what was this bumped to?

Bumped to same as what it was + this:
brave-intl/bat-native-ledger@123d1de

I see, I guess I had already removed the post task stuff but just forgot about the header at that point

bridiver · 2018-08-24T20:10:52Z

browser/mac/sparkle_glue.mm

@@ -46,7 +46,7 @@ static void PostPerform(id target, SEL sel, id arg) {

    scoped_refptr<PerformBridge> op = new PerformBridge(target, sel, arg);
    base::PostTaskWithTraits(FROM_HERE,
-                             {base::MayBlock(), base::TaskPriority::BACKGROUND,
+                             {base::MayBlock(), base::TaskPriority::BEST_EFFORT,


are you sure this is equivalent? Sounds worse?

git log -S for BACKGROUND changed to that in the commit I looked at.

bridiver · 2018-08-24T20:12:34Z

chromium_src/content/child/runtime_features.cc

@@ -7,8 +7,6 @@ namespace content {
 void SetRuntimeFeaturesDefaultsAndUpdateFromArgs(
    const base::CommandLine& command_line) {
  SetRuntimeFeaturesDefaultsAndUpdateFromArgs_ChromiumImpl(command_line);
-
-  WebRuntimeFeatures::EnableAutoplayMutedVideos(false);


addressed here https://github.com/brave/brave-core/pull/358/files#diff-f45e2eb337d4abc3431ff908952a36c6R10
for this upstream commit
https://chromium.googlesource.com/chromium/src/+/49bb4c724406e9095603ad0b121c467ecd746474

bridiver · 2018-08-24T20:14:18Z

patches/chrome-browser-chrome_browser_main.cc.patch

+     chrome_extra_parts_[i]->PreEarlyInitialization();
+
+-  browser_process_ = std::make_unique<BrowserProcessImpl>();
+  browser_process_ = std::make_unique<BraveBrowserProcessImpl>();


this is fine, but I thought you were going to replace these with chromium_src overrides?

Not added here, will be doing it in a followup.

sounds good

bridiver · 2018-08-24T20:14:49Z

patches/chrome-browser-importer-importer_list.cc.patch

-   firefox.services_supported = importer::HISTORY | importer::FAVORITES |
-                                importer::PASSWORDS | importer::SEARCH_ENGINES |
-                               importer::AUTOFILL_FORM_DATA;
-+                               importer::AUTOFILL_FORM_DATA | importer::COOKIES;


I know you didn't work on this for brave-core, but you have experience with it from muon

@bbondy why removing this? This is required for import cookies option from Firefox

Thanks for finding, this was an unintentional miss when applying the patch manually. Btw this patch is really big and we should consider reformatting some of the code to make it easier to rebase.

diff --git a/chrome/browser/importer/importer_list.cc b/chrome/browser/importer/importer_list.cc index c8b1156c1be3d4b31c266b723e3a3ea9e50c73e8..a0dfd62cb0a9aad31573604d4bd884ca4a9cb7e6 100644 --- a/chrome/browser/importer/importer_list.cc +++ b/chrome/browser/importer/importer_list.cc @@ -7,9 +7,13 @@ #include <stdint.h> #include "base/bind.h" +#include "base/strings/utf_string_conversions.h" #include "base/task/post_task.h" #include "base/task/task_traits.h" #include "base/threading/thread_restrictions.h" +#include "base/values.h" +#include "brave/common/importer/brave_importer_utils.h" +#include "brave/common/importer/chrome_importer_utils.h" #include "build/build_config.h" #include "chrome/browser/shell_integration.h" #include "chrome/common/importer/firefox_importer_utils.h" @@ -29,6 +33,9 @@ #include "chrome/common/importer/edge_importer_utils_win.h" #endif +void DetectChromeProfiles(std::vector<importer::SourceProfile>* profiles); +void DetectBraveProfiles(std::vector<importer::SourceProfile>* profiles); + namespace { #if defined(OS_WIN) @@ -136,21 +143,37 @@ std::vector<importer::SourceProfile> DetectSourceProfilesWorker( #if defined(OS_WIN) if (shell_integration::IsFirefoxDefaultBrowser()) { DetectFirefoxProfiles(locale, &profiles); + DetectBraveProfiles(&profiles); DetectBuiltinWindowsProfiles(&profiles); + DetectChromeProfiles(&profiles); } else { DetectBuiltinWindowsProfiles(&profiles); + DetectBraveProfiles(&profiles); DetectFirefoxProfiles(locale, &profiles); + DetectChromeProfiles(&profiles); } #elif defined(OS_MACOSX) if (shell_integration::IsFirefoxDefaultBrowser()) { DetectFirefoxProfiles(locale, &profiles); + DetectBraveProfiles(&profiles); DetectSafariProfiles(&profiles); + DetectChromeProfiles(&profiles); } else { DetectSafariProfiles(&profiles); + DetectBraveProfiles(&profiles); DetectFirefoxProfiles(locale, &profiles); + DetectChromeProfiles(&profiles); } #else - DetectFirefoxProfiles(locale, &profiles); + if (shell_integration::IsFirefoxDefaultBrowser()) { + DetectFirefoxProfiles(locale, &profiles); + DetectBraveProfiles(&profiles); + DetectChromeProfiles(&profiles); + } else { + DetectBraveProfiles(&profiles); + DetectChromeProfiles(&profiles); + DetectFirefoxProfiles(locale, &profiles); + } #endif if (include_interactive_profiles) { importer::SourceProfile bookmarks_profile;

yeah, we have a main issue for minimizing importer patch brave/brave-browser#511

bridiver · 2018-08-24T20:16:51Z

patches/chrome-browser-profiles-profile_io_data.cc.patch

+
+     std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate(
+-        new ChromeNetworkDelegate(
+        new BraveProfileNetworkDelegate(


same as above. I'm fine with approving these, just thought you said you were going to change them so it didn't come up again

I had said I was going to do a PR after the rebase.

This one not possible using the current methods because there are other ChromeNetworkDelegate in the same file that we don't want to replace. We need a stronger preprocessor for this one or yet to be discovered methods.

bridiver · 2018-08-24T20:18:56Z

patches/chrome-browser-ui-app_list-search-omnibox_provider.cc.patch

-      controller_(new AutocompleteController(
-+      controller_(new BraveAutocompleteController(
-           base::WrapUnique(new ChromeAutocompleteProviderClient(profile)),
+-      controller_(std::make_unique<AutocompleteController>(


just marking these for potential chromium_src overrides

bridiver · 2018-08-24T20:19:21Z

patches/chrome-browser-ui-browser.cc.patch

@@ -20,6 +20,7 @@ index 38382324ffac53ed38d12b69cf4931b17823099a..e2bb074bc919929bed40bec065c4563a
       toolbar_model_delegate_(new BrowserToolbarModelDelegate(this)),
       live_tab_context_(new BrowserLiveTabContext(this)),
       synced_window_delegate_(new BrowserSyncedWindowDelegate(this)),
+       hosted_app_controller_(MaybeCreateHostedAppController(this)),
       bookmark_bar_state_(BookmarkBar::HIDDEN),
 -      command_controller_(new chrome::BrowserCommandController(this)),


just marking these for potential chromium_src overrides

bridiver · 2018-08-24T20:20:21Z

patches/chrome-browser-ui-webui-chrome_web_ui_controller_factory.cc.patch

@@ -9,8 +9,8 @@ index be8b073e7bd2778e03d99f953ffae13d90b3b151..b7308cd52bcbb50d9ed6f8e95f3c47cc
 +#include "brave/browser/ui/webui/brave_web_ui_controller_factory.h"


just marking these for potential chromium_src overrides

bridiver · 2018-08-24T20:21:01Z

patches/chrome-common-pref_names.h.patch

 --- a/chrome/common/pref_names.h
 +++ b/chrome/common/pref_names.h
-@@ -358,6 +358,8 @@ extern const char kImportDialogBookmarks[];
+@@ -359,6 +359,8 @@ extern const char kImportDialogBookmarks[];
 extern const char kImportDialogHistory[];
 extern const char kImportDialogSavedPasswords[];
 extern const char kImportDialogSearchEngine[];
 +extern const char kImportDialogCookies[];


this seems related to cookies above for import cc @darkdh

this seems like only patch line chagnes

correct, I only flagged it because I wasn't sure if the other patch change was correct or not. I thought maybe it got integrated into upstream and this would no longer be needed, but sounds like the other patch update was incorrect

bridiver · 2018-08-24T20:21:54Z

patches/third_party-blink-renderer-core-html-media-autoplay_policy.cc.patch

+ bool AutoplayPolicy::DocumentShouldAutoplayMutedVideos(
+     const Document& document) {
+#if defined(BRAVE_CHROMIUM_BUILD)
+  if (GetAutoplayPolicyForDocument(document) == AutoplayPolicy::Type::kUserGestureRequired)


cc @darkdh related to above

@bbondy discussed this with me on Slack. And this is the solution we came up with.

bridiver · 2018-08-24T20:22:14Z

patches/third_party-blink-renderer-modules-peerconnection-rtc_peer_connection.cc.patch

@@ -1,16 +1,16 @@
 diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc
-index 1506e6e6346454b6c4c78cf4d541e1beb5bc06c8..56a8df86b8ce1e6d03d865b62edae617c16be0ab 100644
+index 63ab728344e16edafca56e9c97f868b81b70f1cd..d13e0ec8117e15e1ef144c2b37cca1a3dd43b0c7 100644
 --- a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc
 +++ b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc
 @@ -37,6 +37,7 @@

 #include "base/memory/ptr_util.h"
 #include "base/optional.h"
 +#include "brave/renderer/brave_content_settings_observer_helper.h"


just marking these for potential chromium_src overrides

bridiver · 2018-08-24T20:23:06Z

patches/third_party-blink-renderer-modules-webgl-webgl_rendering_context_base.cc.patch

@@ -9,8 +9,8 @@ index d22b722fcad726e68e41005718a2546765be21be..807db049d22c3878a77d10230e1c8a08
 +#include "brave/renderer/brave_content_settings_observer_helper.h"


is this supposed to be here? cc @yrliou

not a blocker either way, just curious if this was accidental or not

It was here for various WebGL functions calling AllowFingerprinting.

ok, I guess it just didn't show up in the patch change. Thanks!

bbondy · 2018-08-24T21:00:40Z

importer::COOKIES thing fixed, I think that's the only blocker, please review again.

simonhong · 2018-08-24T22:53:40Z

Sorry, my last PR introduced conflicts.
You can just delete patches/chrome-browser-themes-theme_properties.cc.patch.
That patch file was deleted in #351

I realize this is already fixed once we merge Brave Rewards, but this is needed in the meantime

…EPARATOR

bundle_egl_libraries is not declared though so we can't override it without a patch

bbondy added the pr/work-in-progress label Aug 24, 2018

bbondy self-assigned this Aug 24, 2018

bbondy requested a review from bridiver August 24, 2018 18:53

bbondy force-pushed the 70.0.3528.4 branch from 1e45af2 to cb69d71 Compare August 24, 2018 19:10

bbondy changed the title ~~WIP: 70.0.3528.4~~ Upgrade to Chromium 70.0.3528.4 Aug 24, 2018

bbondy removed the pr/work-in-progress label Aug 24, 2018

bbondy mentioned this pull request Aug 24, 2018

Upgrade to Chromium 70.0.3528.4 brave/brave-browser#835

Merged

16 tasks

bridiver reviewed Aug 24, 2018

View reviewed changes

bbondy force-pushed the 70.0.3528.4 branch from cb69d71 to dc12321 Compare August 24, 2018 20:58

bbondy force-pushed the 70.0.3528.4 branch from dc12321 to 55c5d25 Compare August 24, 2018 21:01

bridiver previously approved these changes Aug 24, 2018

View reviewed changes

bbondy added 4 commits August 24, 2018 21:56

Update patches for 70.0.3528.4

7935bdc

Fix for native ledger lib for 70.0.3528.4

7194123

I realize this is already fixed once we merge Brave Rewards, but this is needed in the meantime

Update source strings for Chromium 70.0.3528.4

f8fca88

Update XTB files for Chromium 70.0.3528.4

b641dd7

bbondy added 9 commits August 24, 2018 21:56

Rename base/task_scheduler -> base/task

b1a5eb5

Rename COLOR_TOOLBAR_BOTTOM_SEPARATOR -> COLOR_TOOLBAR_CONTENT_AREA_S…

ee0e041

…EPARATOR

Split generated api bundle and types

01e9461

sql/connection -> sql/database

730d833

DEFINE_WEB_CONTENTS_USER_DATA_KEY no longer needed

e3d7068

base::TaskPriority::BACKGROUND -> BEST_EFFORT

5f248b8

Various updates for Chromium 70.0.3528.4

f3c4078

Sync up run_all_tests.cc with Chromium's version

d7fd9a6

We want use_egl=true but bundle_egl_libraries=false

e7b3238

bundle_egl_libraries is not declared though so we can't override it without a patch

bbondy dismissed bridiver’s stale review via e7b3238 August 25, 2018 01:56

bbondy force-pushed the 70.0.3528.4 branch from 55c5d25 to e7b3238 Compare August 25, 2018 01:56

bbondy merged commit 7913a70 into master Aug 25, 2018

bsclifton deleted the 70.0.3528.4 branch September 26, 2018 05:45

		@@ -9,8 +9,8 @@ index be8b073e7bd2778e03d99f953ffae13d90b3b151..b7308cd52bcbb50d9ed6f8e95f3c47cc
		+#include "brave/browser/ui/webui/brave_web_ui_controller_factory.h"

		@@ -9,8 +9,8 @@ index d22b722fcad726e68e41005718a2546765be21be..807db049d22c3878a77d10230e1c8a08
		+#include "brave/renderer/brave_content_settings_observer_helper.h"

Upgrade to Chromium 70.0.3528.4 #358

Upgrade to Chromium 70.0.3528.4 #358

Conversation

bbondy commented Aug 24, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

bbondy commented Aug 24, 2018

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

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

bbondy commented Aug 24, 2018

simonhong commented Aug 24, 2018

bbondy commented Aug 24, 2018 •

edited

Loading