Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2298283002:
Site Settings Desktop: Support adding exceptions for incognito mode. (Closed)
|
Created:
4 years, 3 months ago by Finnur Modified:
4 years, 3 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSite Settings Desktop: Support adding exceptions for incognito mode.
BUG=635855, 635862, 614277
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/1d5f2515c355ca11dd8ca9cdd785df3f3a5b41a3
Cr-Commit-Position: refs/heads/master@{#416534}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Address feedback #Patch Set 3 : Fix test #Patch Set 4 : Fix test #
Total comments: 19
Patch Set 5 : Address feedback #
Total comments: 17
Patch Set 6 : Address feedback #Patch Set 7 : Fix remaining tests #
Total comments: 7
Patch Set 8 : Address feedback, fix test #Patch Set 9 : Fix test #Messages
Total messages: 65 (45 generated)
Description was changed from ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 614277 ========== to ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
finnur@chromium.org changed reviewers: + dschuyler@chromium.org, sky@chromium.org
Dave, can you review? Scott, can you give OWNERS approval for the changes to the TestingProfile?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
testing_profile LGTM
The real review item here is the one asking about notification_registrar_. (The rest are all nits or optional). https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.html (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.html:27: nit: is the blank line communicating something? Maybe remove it. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:79: */ nit: This function could sort alphabetically above onSubmit_. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:83: profileHasIncognito_: { nit: should this be profileIsIncognito? Does a profile 'have-a' or 'is-a' profile incognito? (I'm asking because I'm not sure). https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:13: * incognito: boolean, optional: isIncognito would be a good name for this boolean, imo (or maybe hasIncognito if that's more accurate). It's not a big deal though. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:181: observer_.RemoveAll(); Should we do something with notification_registrar_ here? Maybe notification_registrar_.RemoveAll(); (Try doing a page refresh on the page using this handler, if that crashes or triggers a DCHECK, then clearing the notification_registrar_ may clear that up). https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:240: Profile* profile = content::Source<Profile>(source).ptr(); Just a question (feel free to ignore): Will we get profiles other than this.profile_? i.e. will this profile be the same as profile_? or will it sometimes be profile_->GetOffTheRecordProfile() or something else? https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:434: ? profile_->GetOffTheRecordProfile() : nullptr) : profile_; Optional: I'm found this hard to parse and I'm wondering if a different layout would help. WDYT of: Profile* profile = incognito ? (profile_->HasOffTheRecordProfile() ? profile_->GetOffTheRecordProfile() : nullptr) : profile_; or Profile* profile = incognito ? (profile_->HasOffTheRecordProfile() ? profile_->GetOffTheRecordProfile() : nullptr) : profile_; https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:470: ? profile_->GetOffTheRecordProfile() : nullptr) : profile_; If you decide to change the layout of the ternary above, this could be formatted in a similar way. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:515: // return false until after the profile actually been destroyed. Thanks for that comment! https://codereview.chromium.org/2298283002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:533: // Select 'Remove from menu' on 'foo.com'. nit: maybe move quote? So that it reads like this: Select 'Remove' from menu on 'foo.com'
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
finnur@chromium.org changed reviewers: + stevenjb@chromium.org
Dave: Comments addressed as per below. Steven: Can you give OWNERS approval for content_settings_handler.cc? https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.html (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.html:27: On 2016/08/31 21:48:52, dschuyler wrote: > nit: is the blank line communicating something? Maybe remove it. Done. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/add_site_dialog.js:79: */ On 2016/08/31 21:48:52, dschuyler wrote: > nit: This function could sort alphabetically > above onSubmit_. Done. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:83: profileHasIncognito_: { Both, I would say. A profile can be incognito, but here I'm trying to convey that the current profile also has an incognito profile counterpart. But, I can see how it can be confusing for a casual reader. I'll change it to... hmm... incognitoProfileActive_. Is that better? https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:13: * incognito: boolean, Remember that this variable is an attribute of a site exception, so both 'isIncognito' and 'hasIncognito' are a bit misleading. I wondered if 'forIncognito' would be better, but ended up deciding it wasn't really an improvement over simply using 'incognito'. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:181: observer_.RemoveAll(); NotificationRegistrar calls RemoveAll in the dtor, so we should be good. I looked up a few samples on code search and the first five I found did not clean up explicitly. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:240: Profile* profile = content::Source<Profile>(source).ptr(); > Just a question (feel free to ignore): No, it is an excellent question. > Will we get profiles other than this.profile_? Yes, we can get a pointer to any random profile that is created/destroyed, incognito or not. And we don't want to handle all of them. Addressed. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:434: ? profile_->GetOffTheRecordProfile() : nullptr) : profile_; On 2016/08/31 21:48:52, dschuyler wrote: > Optional: I'm found this hard to parse and I'm > wondering if a different layout would help. WDYT > of: > > Profile* profile = incognito > ? (profile_->HasOffTheRecordProfile() > ? profile_->GetOffTheRecordProfile() > : nullptr) > : profile_; > > or > > Profile* profile = > incognito ? > (profile_->HasOffTheRecordProfile() ? > profile_->GetOffTheRecordProfile() : > nullptr) : > profile_; Done. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:470: ? profile_->GetOffTheRecordProfile() : nullptr) : profile_; On 2016/08/31 21:48:52, dschuyler wrote: > If you decide to change the layout of the > ternary above, this could be formatted in > a similar way. Done. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:515: // return false until after the profile actually been destroyed. No problem. :) https://codereview.chromium.org/2298283002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/site_list_tests.js:533: // Select 'Remove from menu' on 'foo.com'. On 2016/08/31 21:48:52, dschuyler wrote: > nit: maybe move quote? So that it reads like this: > Select 'Remove' from menu on 'foo.com' Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 635862, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I lust looked at the C++ code. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:951: web_ui(), false, false /* not incognito */ here and elsewhere. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:240: if (profile != profile_ && !IsOurIncognitoProfile(profile)) Can this just be if (!profile->IsSameProfile(prfofile_)) ? https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:242: SendIncognitoStatus(true, profile); SendIncognitoStatus(true /* destroy */, profile); (here and below) Also, I think that would be more intuitive with the args reversed. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:439: : profile_; This is super confusing, maybe: if (incognito) { if (!profile_->HasOffTheRecordProfile()) return; profile = profile_->GetOffTheRecordProfile(); } else { profile = profile_; } https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:479: return; ditto https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:522: // return false until after the profile actually been destroyed. "destroy" implies that this function does the destruction. We should rename the parameter "was_destroyed". https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:527: } Combine these: bool incognito_enabled = profile_->HasOffTheRecordProfile() && !(was_destroyed && profile == profile_->GetOffTheRecordProfile()); https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_settings_helper.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_settings_helper.cc:264: exception->SetBoolean(kIncognito, incognito); It *looks* like the only thing we use |incognito| for is to set exception[kIncognito] = true. If that is the case, I think this code would be more clear with a separate call: site_settings::SetIncognito(exceptions). Otherwise, anywhere we call these functions we need to document the boolean, e.g. true /* incognito */ or false /* not incognito */. (Or use a named variable).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Steven. All addressed. Scott, can you do me a favor and see if you spot something obvious for the test failures here: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... I think they are related to the TestingProfile changes. I'm going to dig a little deeper into them tomorrow. Would appreciate your input, if you have any. Thanks! https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:951: web_ui(), false, On 2016/09/01 15:47:55, stevenjb wrote: > false /* not incognito */ here and elsewhere. Done. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:240: if (profile != profile_ && !IsOurIncognitoProfile(profile)) I'm a little worried that TestingProfile offers quite a different IsSameProfile implementation than ProfileImpl does (the latter considers incognito to be the same profile, but the former does not). https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:242: SendIncognitoStatus(true, profile); On 2016/09/01 15:47:55, stevenjb wrote: > SendIncognitoStatus(true /* destroy */, profile); (here and below) > > Also, I think that would be more intuitive with the args reversed. Done. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:439: : profile_; Fair enough. Done. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:479: return; On 2016/09/01 15:47:55, stevenjb wrote: > ditto Done. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:522: // return false until after the profile actually been destroyed. Makes sense. Done. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:527: } On 2016/09/01 15:47:55, stevenjb wrote: > Combine these: > > bool incognito_enabled = profile_->HasOffTheRecordProfile() && !(was_destroyed > && profile == profile_->GetOffTheRecordProfile()); > Done. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_settings_helper.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_settings_helper.cc:264: exception->SetBoolean(kIncognito, incognito); Correct. We send down (to .js) a single Array<SiteException> where some of the SiteExceptions have the |incognito| bit set while others don't. I think your comment can be read in two ways. Either you are saying: 1) Once we've gathered the exceptions for incognito, we should (before gathering the other exceptions) iterate through the incognito exceptions and add the incognito flag to each one. ... OR ... 2) We should change what we send down to .js, from: Array<SiteException> ... to something like ... [ {incognito : boolean, exceptions: Array<SiteException>}, ... ] My concern about 1 is that it is less efficient than the current solution and my concern about 2 is that changing the format we send down is a breaking change for the old settings code, which I was hoping to avoid (with a non-breaking change like adding an extra field)...
https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:240: if (profile != profile_ && !IsOurIncognitoProfile(profile)) On 2016/09/01 16:48:17, Finnur wrote: > I'm a little worried that TestingProfile offers quite a different IsSameProfile > implementation than ProfileImpl does (the latter considers incognito to be the > same profile, but the former does not). Hmm. It seems unfortunate that we have to write our own version. That seems like a bug in TestingProfile. If it actually is a problem, I would rather see us fix that. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_settings_helper.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_settings_helper.cc:264: exception->SetBoolean(kIncognito, incognito); On 2016/09/01 16:48:17, Finnur wrote: > Correct. > > We send down (to .js) a single Array<SiteException> where some of the > SiteExceptions have the |incognito| bit set while others don't. > > I think your comment can be read in two ways. Either you are saying: > > 1) Once we've gathered the exceptions for incognito, we should (before gathering > the other exceptions) iterate through the incognito exceptions and add the > incognito flag to each one. > > ... OR ... > > 2) We should change what we send down to .js, from: > Array<SiteException> > ... to something like ... > [ {incognito : boolean, exceptions: Array<SiteException>}, ... ] > > > My concern about 1 is that it is less efficient than the current solution and my > concern about 2 is that changing the format we send down is a breaking change > for the old settings code, which I was hoping to avoid (with a non-breaking > change like adding an extra field)... I was suggesting (1). I don't think the inefficiency of an extra call to SetBoolean is a concern, but since you commented all of the calls to this, that's fine. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:242: SendIncognitoStatus(profile, true); // Destroy -> true. was_destroyed throughout
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:83: profileHasIncognito_: { On 2016/09/01 11:10:15, Finnur wrote: > Both, I would say. A profile can be incognito, but here I'm trying to convey > that the current profile also has an incognito profile counterpart. But, I can > see how it can be confusing for a casual reader. I'll change it to... hmm... > incognitoProfileActive_. Is that better? I like it! https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:181: observer_.RemoveAll(); On 2016/09/01 11:10:15, Finnur wrote: > NotificationRegistrar calls RemoveAll in the dtor, so we should be good. > I looked up a few samples on code search and the first five I found did not > clean up explicitly. Ah, please look specifically within chrome/browser/ui/webui/settings. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/add_site_dialog.html (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/add_site_dialog.html:28: <div>$i18n{incognitoSiteOnly}</div> Does this need the <div> (Asking because I'm not sure). https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/add_site_dialog.js:37: this.browserProxy.updateIncognitoStatus(); Optional: does the UI going into the dialog flicker at all? I'm wondering if there is a timing issue where the update would arrive after the dialog just opened and create a flicker. If so, maybe return a promise from updateIncognitoStatus() and call this.$.dialog.showModal() inside the .then(). If there is no flicker to fix then there's no need for the promise (I'm just guessing that there may be flicker). https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:499: } Optional: It seems common to have a blank line following an 'if' or 'if/else'. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:953: false, // Incognito. We can use /* */ in the parameter list. That may allow this to be wrapped less. site_settings::GetExceptionsFromHostContentSettingsMap(settings_map, type, web_ui(), /*incognito=*/false, &exceptions); See "Function Argument Comments" in the C++ coding standard. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:116: content::NotificationService::AllSources()); Both of theses Add() calls should move from the ctor to the OnJavascriptAllowed() function. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:180: observer_.RemoveAll(); After the notification_registrar_.Add() calls are moved to OnJavascriptAllowed(), reason may be more clear for having notification_registrar_.RemoveAll() here. Search for "registrar" within chrome/browser/ui/webui/settings for examples. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:242: SendIncognitoStatus(profile, true); // Destroy -> true. Coding style comment: SendIncognitoStatus(profile, /*was_detroyed=*/true); https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:510: SendIncognitoStatus(profile_, false); // Destroy -> false. Change comment here too :)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Scott, FYI: I've reverted my changes to TestingProfile (broke too many unrelated tests) and instead fleshed out the IsSameProfile function (it basically does the same as ProfileImpl does). Dave and Steven: All comments addressed, I believe. https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:181: observer_.RemoveAll(); On 2016/09/01 23:13:49, dschuyler wrote: > On 2016/09/01 11:10:15, Finnur wrote: > > NotificationRegistrar calls RemoveAll in the dtor, so we should be good. > > I looked up a few samples on code search and the first five I found did not > > clean up explicitly. > > Ah, please look specifically within > chrome/browser/ui/webui/settings. Acknowledged. https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:240: if (profile != profile_ && !IsOurIncognitoProfile(profile)) No problem. Fixed! https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/add_site_dialog.html (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/add_site_dialog.html:28: <div>$i18n{incognitoSiteOnly}</div> Nope. Totally redundant. Removed. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/add_site_dialog.js:37: this.browserProxy.updateIncognitoStatus(); No, I haven't noticed that but I'll keep an eye on it. Appreciate the suggestion on how to potentially fix it! https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_list.js:499: } On 2016/09/01 23:13:49, dschuyler wrote: > Optional: It seems common to have a blank line > following an 'if' or 'if/else'. Done. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:953: false, // Incognito. On 2016/09/01 23:13:49, dschuyler wrote: > We can use /* */ in the parameter list. That may allow this to be wrapped less. > > site_settings::GetExceptionsFromHostContentSettingsMap(settings_map, type, > web_ui(), /*incognito=*/false, &exceptions); > > See "Function Argument Comments" in the C++ coding standard. Done. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:116: content::NotificationService::AllSources()); OK, fine. But I'm concerned about that because profile creation and destruction seems orthogonal to whether javascript is allowed/not. I'm worried we might miss a message or two... https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:180: observer_.RemoveAll(); Yeah, I now understand the RemoveAll comment better... https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:242: SendIncognitoStatus(profile, true); // Destroy -> true. On 2016/09/01 23:13:49, dschuyler wrote: > Coding style comment: > SendIncognitoStatus(profile, /*was_detroyed=*/true); Done. https://codereview.chromium.org/2298283002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:510: SendIncognitoStatus(profile_, false); // Destroy -> false. On 2016/09/01 23:13:49, dschuyler wrote: > Change comment here too :) Done.
Thanks for fixing TestingProfile! LGTM https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:951: web_ui(), /*incognito=*/ false, &exceptions); nit: We generally do: false /* incognito */,
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:951: web_ui(), /*incognito=*/ false, &exceptions); On 2016/09/02 15:54:01, stevenjb wrote: > nit: We generally do: false /* incognito */, Interesting, I got a different impression. Rather than discuss it here, I shared a doc with my understanding of the usage at: https://docs.google.com/a/chromium.org/document/d/15rL7FMf24YO7hdHDkVXve8v7pr... https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1096: web_ui(), /*incognito=*/ false, &exceptions); nit: inside a parameter list (as an exception to the normal spacing), spaces are not used in or after the comment. Visually, it helps make the thing look like a single unit. This helps readability because parameters are set positionally. So: Foo(settings_map, type, web_ui(), /*incognito=*/false, &exceptions); is easier to see the five parameters. E.g. https://cs.chromium.org/search/?q=%5B%5E%5Cs%5D%3D%5C*/%5B%5E%5Cs%5D&sq=packa... https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler.cc:177: content::NotificationService::AllSources()); Just FYI: Thanks for doing this. One of the objectives here is that we try not to send messages when the JavaScript is not ready for it (because it can lead to dropped messages or random/infrequent crashes - which is worse than frequent crashes :) ). https://codereview.chromium.org/2298283002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2298283002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:483: settings.ContentSettingsTypes.COOKIES, contentType); nit: Could this unwrap?
https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/2298283002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:951: web_ui(), /*incognito=*/ false, &exceptions); On 2016/09/02 22:49:38, dschuyler wrote: > On 2016/09/02 15:54:01, stevenjb wrote: > > nit: We generally do: false /* incognito */, > > Interesting, I got a different impression. Rather than > discuss it here, I shared a doc with my understanding > of the usage at: > > https://docs.google.com/a/chromium.org/document/d/15rL7FMf24YO7hdHDkVXve8v7pr... Caveat: This is entirely a nit, for discussion prurposes only: Based on prevalence in the existing code. ~1000 examples of "false /*", 300 examples of "*/ false", and 500 examples of "*/false" (not an exact metric by any means, but consistent with my impression of the code). So, take your pick. (The one example in the style guide does show /*incognito=*/ false, but it's not a specific rule, and I generally find prevalence a better argument than interpretation of an example).
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, stevenjb@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2298283002/#ps180001 (title: "Address feedback, fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2298283002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_list_tests.js (right): https://codereview.chromium.org/2298283002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/site_list_tests.js:483: settings.ContentSettingsTypes.COOKIES, contentType); On 2016/09/02 22:49:38, dschuyler wrote: > nit: Could this unwrap? Done.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org, stevenjb@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2298283002/#ps200001 (title: "Fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 635862, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 635862, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 635862, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Support adding exceptions for incognito mode. BUG=635855, 635862, 614277 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/1d5f2515c355ca11dd8ca9cdd785df3f3a5b41a3 Cr-Commit-Position: refs/heads/master@{#416534} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1d5f2515c355ca11dd8ca9cdd785df3f3a5b41a3 Cr-Commit-Position: refs/heads/master@{#416534} |
