Follow up on comments for c76 update #2903

darkdh · 2019-07-11T23:08:58Z

fix brave/brave-browser#5222

Submitter Checklist:

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

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.

browser/search_engines/tor_window_search_engine_provider_service.cc

bridiver · 2019-07-12T19:19:58Z

browser/search_engines/search_engine_provider_util.cc

@@ -34,4 +35,16 @@ bool IsRegionForQwant(Profile* profile) {
      TemplateURLPrepopulateData::PREPOPULATED_ENGINE_ID_QWANT;
 }

+bool IsOTRGuestProfile(Profile* profile) {


I'm still confused by this. Is there more than one kind of guest profile?

I guess it depends on what you call a guest profile. IsGuestSession is set to true for regular and off-the-record profiles of a profile with "guest" path. GetProfileType only returns GUEST_PROFILE type for the off-the-record profile.

For a short while (5/20 - 5/29) Chromium had an ideal method Profile::IsGuestProfile for what we need, but they removed it: https://chromium.googlesource.com/chromium/src/+/5f95c46d5ce37d3b271b1f99063459e5d715761e

bridiver · 2019-07-17T17:15:15Z

browser/search_engines/guest_window_search_engine_provider_service.cc

-  DCHECK(IsGuestProfile(otr_profile));
-  DCHECK(!otr_profile->IsTorProfile());
+  DCHECK(brave::IsOTRGuestProfile(otr_profile));
+  DCHECK(!tor::IsTorProfile(otr_profile));


I don't think this is necessary. If it's a guest profile from the path then it's definitely not a tor profile and this check will be even more irrelevant once Tor profiles are not based on guest profiles

I think the original intent (Profile::GetProfileType() == GUEST_PROFILE) here was not that it was a guest profile, but that it was an OTR profile.

bridiver · 2019-07-17T17:15:33Z

browser/search_engines/search_engine_provider_service_factory.cc

@@ -32,7 +34,7 @@ KeyedService* InitializeSearchEngineProviderServiceIfNeeded(Profile* profile) {

  // Regardless of qwant region, tor profile needs controller to store
  // previously set search engine provider.
-  if (profile->IsTorProfile() && IsGuestProfile(profile)) {
+  if (tor::IsTorProfile(profile) && brave::IsOTRGuestProfile(profile)) {


IsOTRGuestProfile is not needed here

bridiver · 2019-07-17T17:15:58Z

browser/search_engines/tor_window_search_engine_provider_service.cc

-  DCHECK(otr_profile->IsTorProfile());
-  DCHECK(IsGuestProfile(otr_profile));
+  DCHECK(tor::IsTorProfile(otr_profile));
+  DCHECK(brave::IsOTRGuestProfile(otr_profile));


this check is not needed and won't be relevant when tor profiles are not based on guest profiles

bridiver · 2019-07-17T19:11:55Z

browser/search_engines/search_engine_provider_util.cc

+  // profile to check for guest.
+  DCHECK(profile);
+  return (profile->HasOffTheRecordProfile() &&
+    profile->GetOffTheRecordProfile() == profile &&


lint - 4 spaces for continuation of a line

IsOTRGuestProfile() in SearchEngineProviderServiceFactory

bridiver · 2019-07-18T16:29:08Z

browser/profiles/profile_util.cc

+#endif
+}
+
+bool IsOffTheRecordProfile(Profile* profile) {


this isn't needed BrowserContext::IsOffTheRecord already exists

bridiver · 2019-07-18T16:30:46Z

browser/search_engines/search_engine_provider_service_factory.cc

@@ -32,7 +34,7 @@ KeyedService* InitializeSearchEngineProviderServiceIfNeeded(Profile* profile) {

  // Regardless of qwant region, tor profile needs controller to store
  // previously set search engine provider.
-  if (profile->IsTorProfile() && IsGuestProfile(profile)) {
+  if (brave::IsTorProfile(profile) && brave::IsOffTheRecordProfile(profile)) {


I don't think we want to differentiate with OTR here. All Tor profiles should use TorWindowSearchEngineProviderService

bridiver · 2019-07-18T16:30:54Z

browser/search_engines/search_engine_provider_service_factory.cc

@@ -41,7 +43,7 @@ KeyedService* InitializeSearchEngineProviderServiceIfNeeded(Profile* profile) {
  if (brave::IsRegionForQwant(profile))
    return nullptr;

-  if (IsGuestProfile(profile)) {
+  if (brave::IsGuestProfile(profile) && brave::IsOffTheRecordProfile(profile)) {


same here as above

bridiver · 2019-07-18T17:40:19Z

browser/search_engines/search_engine_provider_service_factory.cc

  // That means changing normal profile's provider doesn't affect otr profile's.
-  // This controller monitor's normal profile's service and apply it's change to
+  // This controller monitor's normal profile's service and apply its change to
  // otr profile to use same provider.
  // Private profile's setting is shared with normal profile's setting.
  if (profile->IsIncognitoProfile()) {


please move this check to the end

IsGuestProfile was IsOTRGuestProfile, but no longer checks for OTR profile.

Follow up on comments for c76 update

darkdh force-pushed the 76.0.3809.62-followup branch 2 times, most recently from 7384a2c to c46a565 Compare July 12, 2019 00:27

mkarolin reviewed Jul 12, 2019

View reviewed changes

browser/search_engines/tor_window_search_engine_provider_service.cc Outdated Show resolved Hide resolved

mkarolin self-assigned this Jul 12, 2019

mkarolin added this to the 0.69.x - Nightly milestone Jul 12, 2019

mkarolin assigned darkdh Jul 12, 2019

darkdh force-pushed the 76.0.3809.62-followup branch from 5156e08 to cd39beb Compare July 12, 2019 16:46

darkdh marked this pull request as ready for review July 12, 2019 17:01

darkdh requested a review from bridiver as a code owner July 12, 2019 17:01

mkarolin force-pushed the 76.0.3809.62-followup branch from cd39beb to 5f90b71 Compare July 12, 2019 17:37

bridiver reviewed Jul 12, 2019

View reviewed changes

mkarolin requested a review from bridiver July 16, 2019 19:20

mkarolin mentioned this pull request Jul 16, 2019

Upgrade from Chromium 75.0.3770.100 to Chromium 76.0.3809.62 (0.68.x) #2934

Merged

32 tasks

mkarolin requested a review from bsclifton July 17, 2019 16:39

bridiver reviewed Jul 17, 2019

View reviewed changes

mkarolin force-pushed the 76.0.3809.62-followup branch 2 times, most recently from a7306ec to 0294bdc Compare July 17, 2019 22:30

darkdh and others added 8 commits July 17, 2019 19:14

Use tor::IsTorProfile(const Profile*) and move IsGuestProfile() to

2d35a04

IsOTRGuestProfile() in SearchEngineProviderServiceFactory

c76 followup: fixed wrong #undef

d7cad7f

c76 followup: moved large patch into a macro.

aeaceb8

c76 followup: moved multiline patch to a macro.

48c1c6f

c76 followup: removed a blank line from a patch.

7a1e2e0

Added comments for IsOTRGuestProfile.

fed6b4b

c76 followup: deps don't need to be public.

386b9ff

Introduce brave_profile_menu_view_helper

3412237

mkarolin force-pushed the 76.0.3809.62-followup branch from 0294bdc to 1fb5f08 Compare July 17, 2019 23:15

bridiver reviewed Jul 18, 2019

View reviewed changes

mkarolin force-pushed the 76.0.3809.62-followup branch from 1fb5f08 to fa55e87 Compare July 18, 2019 17:15

bridiver reviewed Jul 18, 2019

View reviewed changes

Moved IsGuestProfile and IsTorProfile to profile_util.h/cc

a0212eb

IsGuestProfile was IsOTRGuestProfile, but no longer checks for OTR profile.

mkarolin force-pushed the 76.0.3809.62-followup branch from fa55e87 to a0212eb Compare July 18, 2019 17:51

bridiver approved these changes Jul 18, 2019

View reviewed changes

mkarolin merged commit de551ba into master Jul 18, 2019

mkarolin deleted the 76.0.3809.62-followup branch July 18, 2019 18:03

mkarolin added a commit that referenced this pull request Jul 18, 2019

Merge pull request #2903 from brave/76.0.3809.62-followup

59da138

Follow up on comments for c76 update

mkarolin added a commit that referenced this pull request Jul 18, 2019

Merge pull request #2903 from brave/76.0.3809.62-followup

0ccfd05

Follow up on comments for c76 update

mkarolin mentioned this pull request Jul 18, 2019

Upgrade from Chromium 75.0.3770.100 to Chromium 76.0.3809.62 (0.67.x) #2939

Merged

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up on comments for c76 update #2903

Follow up on comments for c76 update #2903

darkdh commented Jul 11, 2019 •

edited by mkarolin

Loading

bridiver Jul 12, 2019

mkarolin Jul 12, 2019

mkarolin Jul 15, 2019

bridiver Jul 17, 2019

mkarolin Jul 17, 2019

bridiver Jul 17, 2019

bridiver Jul 17, 2019

bridiver Jul 17, 2019

bridiver Jul 18, 2019

bridiver Jul 18, 2019

bridiver Jul 18, 2019

bridiver Jul 18, 2019

Follow up on comments for c76 update #2903

Follow up on comments for c76 update #2903

Conversation

darkdh commented Jul 11, 2019 • edited by mkarolin Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

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

darkdh commented Jul 11, 2019 •

edited by mkarolin

Loading