Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11299035:
Support manual (white|black)list, previewing and allowing after interstitial (Closed)
|
Created:
8 years, 1 month ago by Sergiu Modified:
7 years, 11 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds support for manual whitelist and blacklist, previewing a page and
adding it to the whitelist for managed users.
This CL adds two user preferences (kManagedModeWhitelist and
kManagedModeBlacklist) which contain URL patterns which should be allowed
or blocked. The user preferences are used only to persistently store the
lists and the checking is done by the ManagedModeURLFilter, which uses
policy::URLBlacklist objects. The code takes care that the preferences and
the URLFilter are kept in sync all the time. It also contains the functions
needed to interact with these lists (adding, removing or checking).
It also adds a preview infobar which pops up when the user clicks Preview
on the interstitial. If the user clicks allow then the website is added
to the whitelist. The domain gets added plus all redirects that lead to
that page. The user is able to click navigate around the website without
getting another interstitial as long as he does not navigate to a different
host name.
BUG=168772
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175806
Patch Set 1 #
Total comments: 51
Patch Set 2 : Refactored and improved the code. #Patch Set 3 : Rebase #Patch Set 4 : Allow/block flow which includes preview mode. #
Total comments: 79
Patch Set 5 : Changes according to comments. #
Total comments: 38
Patch Set 6 : Changes in response to comments. #
Total comments: 10
Patch Set 7 : Split navigated_urls in two and refactor. #
Total comments: 24
Patch Set 8 : Moved back to GURLs instead of patterns, minor edits. #
Total comments: 2
Patch Set 9 : DCHECKs and minor change. #
Total comments: 2
Patch Set 10 : Moving ifs to switch. #
Total comments: 2
Patch Set 11 : Removing default in switch. #
Total comments: 27
Patch Set 12 : Updated code from integration package + replies to comments. #Patch Set 13 : Fix bad text in generated_resources.grd and rebase to ToT #
Total comments: 35
Patch Set 14 : Replying to comments. #
Total comments: 7
Patch Set 15 : Minor fixes. #
Total comments: 2
Patch Set 16 : Last nits #
Total comments: 2
Patch Set 17 : Rebase, update infobars to use Create() and minor nit. #Patch Set 18 : Update a function in navigation_observer. #
Total comments: 2
Patch Set 19 : Initialize the white/blacklist properly + nit #Patch Set 20 : Initialize manual whitelist to block everything in the begining. #
Total comments: 9
Patch Set 21 : Minor changes #Messages
Total messages: 48 (0 generated)
Here is the patch on Rietveld, it's against a pretty old branch on Bernhard's tree so expect to merge conflicts when pulling :) Sergiu
https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:15132: + Add to whitelist! So many exclamation marks! I think we could get by without this one, at least. The other one seems fine, as it's sort of a panic thing anyway if you don't like the site. https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:15134: + <message name="IDS_MANAGED_MODE_PREVIEW_CANCEL" desc="XXX"> We might also want to remove this constant in favor of IDS_MANAGED_MODE_GO_BACK_ACTION above. https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:15138: + This URL was already on the whitelist so it was not added again. We also added <ph name="ADDED_URL_COUNT">$1<ex>6</ex></ph> URLs as part of redirects. We are probably going to need to iterate on the strings. In any case, this doesn't work out to a proper sentence for the 1 case (for the 0 case it probably doesn't even make sense to mention it), and that's only in English. In general, number i18n is hard :-( https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.cc:264: // XXX Is this ok? GetList returns a const and we need non-const Yes, it is (at least with this signature). We're passing ownership of the return value, so it needs to be a copy. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.cc:265: return scoped_ptr<base::ListValue>( You can use make_scoped_ptr() here. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.cc:266: GetInstance()->managed_profile_->GetPrefs()->GetList( I would prefer to have public static Foo() methods that forward to private non-static FooImpl() methods instead of accessing the members directly. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:28: namespace policy{ Please add a blank line before the namespace. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:70: // Returns the profile whitelist. "[…] a copy of [the profile whitelist]"? Also, it's not entirely clear what "profile whitelist" means here. Also, does this need to be public? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:74: static void SetWhitelist(base::ListValue* whitelist); What happens to the pointer that is passed in? Where is this used? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:76: // Adds the |url| to the manual lists. This is a URL *pattern*, right? So, we should call both the parameter that, and update the method name accordingly. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:30: return list->Find(*Value::CreateStringValue(url_to_add)) != list->end(); This leaks a StringValue. You could just iterate over the list and check the entries, no? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:157: // Clear the pointer as the infobar was closed. The ManagedModePreviewInfobarDelegate doesn't *really* know what the ManagedModeNavigationObserver does, it just notifies it that it's being dismissed. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:215: // the navigated urls to the URL filter. Why don't we do that stuff with one method? Where the whitelist/blacklist are persisted should be an implementation detail of ManagedMode. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:218: std::string url_to_add; Move this down where it's initialized? Also, it's not a URL, it's a URL pattern (which doesn't parse as a URL, which is why I'm nitpicking about this). https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:241: if (!IsInList(ManagedMode::GetWhitelist().get(), url_to_add)) { Argh! Please don't ever do this. You create a scoped_ptr as a temporary, which means that it will be destroyed. In this case you're safe because that will happen after this statement has executed, but this is a pure recipe for use-after-free. Do we even need to check the manual whitelist as stored in prefs? Couldn't we just ask the URL filter? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:265: // This means that a new navigation was instantiated and the data related to What do you mean by "a new navigation was instantiated"? I think a navigation "happens", but even that is oversimplifying it (navigation is Complicated, as evidenced by the multitude of events here). When is this method called? When we navigate somewhere completely different? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:309: // must be blocked. I'm not sure if I understand the interplay between the different navigation events here. Can you explain what happens in what order? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.h:29: // allowed and not more general versions of them. I think this can be put a bit more succinctly by saying that we whitelist exact URLs if they redirect, and hosts if we stay on the page. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.h:33: void AddURLList(); Add NavigatedURL and AddURLList are very similarly named, but they do very different things. Could we do something like SaveNavigatedURL() and AddSavedURLsToWhitelist()? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:10: #include "chrome/browser/policy/url_blacklist_manager.h" Is this necessary? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_resource_throttle.h:19: class URLBlacklist; Is this necessary? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.cc:163: if (!url_manual_list_allow_.IsURLBlocked(url)) { Urgh, this is hella confusing. For a demo it's fine, but I'm wondering if we should use something different longer-term. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.cc:230: // XXX should do BlockingPool stuff here as well? That depends ;-) You have an asynchronous interface, so you *could* run this stuff on a different thread. OTOH, if you run it synchronously, you don't need the callback. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.cc:256: ListValue bl; Please don't use abbreviations for variable names. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_url_filter.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.h:55: // Sets the manual whitelists Nit: Punctuation at the end of the sentence please. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.h:73: // The |url_manual_list_allow_| blocks all URLs except the one that are Nit: Blank line before this would be nice. Also: "except the ones" https://codereview.chromium.org/11299035/diff/1/chrome/browser/profiles/profi... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile_impl.cc:287: // Preferences for whitelist and blacklists We usually explain what preferences are for in pref_names.cc.
I've replied to the comments but I needed to make a lot of changes in order for it to properly work so there may be some disparity there. Feedback & manual testing are welcome. Sergiu https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:15132: + Add to whitelist! On 2012/11/16 15:04:46, Bernhard Bauer wrote: > So many exclamation marks! I think we could get by without this one, at least. > The other one seems fine, as it's sort of a panic thing anyway if you don't like > the site. Done. https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:15134: + <message name="IDS_MANAGED_MODE_PREVIEW_CANCEL" desc="XXX"> On 2012/11/16 15:04:46, Bernhard Bauer wrote: > We might also want to remove this constant in favor of > IDS_MANAGED_MODE_GO_BACK_ACTION above. Done. https://codereview.chromium.org/11299035/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:15138: + This URL was already on the whitelist so it was not added again. We also added <ph name="ADDED_URL_COUNT">$1<ex>6</ex></ph> URLs as part of redirects. On 2012/11/16 15:04:46, Bernhard Bauer wrote: > We are probably going to need to iterate on the strings. In any case, this > doesn't work out to a proper sentence for the 1 case (for the 0 case it probably > doesn't even make sense to mention it), and that's only in English. In general, > number i18n is hard :-( You are right, rephrased it and removed the number, what do you think now? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.cc:265: return scoped_ptr<base::ListValue>( On 2012/11/16 15:04:46, Bernhard Bauer wrote: > You can use make_scoped_ptr() here. Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.cc:266: GetInstance()->managed_profile_->GetPrefs()->GetList( On 2012/11/16 15:04:46, Bernhard Bauer wrote: > I would prefer to have public static Foo() methods that forward to private > non-static FooImpl() methods instead of accessing the members directly. Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:28: namespace policy{ On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Please add a blank line before the namespace. Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:70: // Returns the profile whitelist. On 2012/11/16 15:04:46, Bernhard Bauer wrote: > "[…] a copy of [the profile whitelist]"? > > Also, it's not entirely clear what "profile whitelist" means here. Also, does > this need to be public? Made it private, improved the comment. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:74: static void SetWhitelist(base::ListValue* whitelist); On 2012/11/16 15:04:46, Bernhard Bauer wrote: > What happens to the pointer that is passed in? Where is this used? Deleted this function all together. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode.h:76: // Adds the |url| to the manual lists. On 2012/11/16 15:04:46, Bernhard Bauer wrote: > This is a URL *pattern*, right? So, we should call both the parameter that, and > update the method name accordingly. Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:30: return list->Find(*Value::CreateStringValue(url_to_add)) != list->end(); On 2012/11/16 15:04:46, Bernhard Bauer wrote: > This leaks a StringValue. You could just iterate over the list and check the > entries, no? Moved to managed_mode.cc and declared a StringValue there, that should be ok, right? https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:215: // the navigated urls to the URL filter. On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Why don't we do that stuff with one method? Where the whitelist/blacklist are > persisted should be an implementation detail of ManagedMode. Moved in ManagedMode. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:241: if (!IsInList(ManagedMode::GetWhitelist().get(), url_to_add)) { On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Argh! Please don't ever do this. You create a scoped_ptr as a temporary, which > means that it will be destroyed. In this case you're safe because that will > happen after this statement has executed, but this is a pure recipe for > use-after-free. > > Do we even need to check the manual whitelist as stored in prefs? Couldn't we > just ask the URL filter? Got it, refactored and as we discussed a bit, I check via the pref one because that has the exact entries while the url filter only knows the final effect. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:265: // This means that a new navigation was instantiated and the data related to On 2012/11/16 15:04:46, Bernhard Bauer wrote: > What do you mean by "a new navigation was instantiated"? I think a navigation > "happens", but even that is oversimplifying it (navigation is Complicated, as > evidenced by the multitude of events here). When is this method called? When we > navigate somewhere completely different? Gave an example in the .h. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:309: // must be blocked. On 2012/11/16 15:04:46, Bernhard Bauer wrote: > I'm not sure if I understand the interplay between the different navigation > events here. Can you explain what happens in what order? Rephrased here and provided an example in the .h https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.h:29: // allowed and not more general versions of them. On 2012/11/16 15:04:46, Bernhard Bauer wrote: > I think this can be put a bit more succinctly by saying that we whitelist exact > URLs if they redirect, and hosts if we stay on the page. Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_navigation_observer.h:33: void AddURLList(); On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Add NavigatedURL and AddURLList are very similarly named, but they do very > different things. Could we do something like SaveNavigatedURL() and > AddSavedURLsToWhitelist()? Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:10: #include "chrome/browser/policy/url_blacklist_manager.h" On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Is this necessary? Removed. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_resource_throttle.h:19: class URLBlacklist; On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Is this necessary? Deleted. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.cc:163: if (!url_manual_list_allow_.IsURLBlocked(url)) { Added a TODO in the meanwhile. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.cc:230: // XXX should do BlockingPool stuff here as well? On 2012/11/16 15:04:46, Bernhard Bauer wrote: > That depends ;-) You have an asynchronous interface, so you *could* run this > stuff on a different thread. OTOH, if you run it synchronously, you don't need > the callback. The question refers more to whether it is needed/desired in the context. Adding those values shouldn't take a long time by me but maybe I'm wrong :) https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.cc:256: ListValue bl; On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Please don't use abbreviations for variable names. Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... File chrome/browser/managed_mode/managed_mode_url_filter.h (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.h:55: // Sets the manual whitelists On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Nit: Punctuation at the end of the sentence please. Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/managed_mode/m... chrome/browser/managed_mode/managed_mode_url_filter.h:73: // The |url_manual_list_allow_| blocks all URLs except the one that are On 2012/11/16 15:04:46, Bernhard Bauer wrote: > Nit: Blank line before this would be nice. > > Also: "except the ones" Done. https://codereview.chromium.org/11299035/diff/1/chrome/browser/profiles/profi... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/11299035/diff/1/chrome/browser/profiles/profi... chrome/browser/profiles/profile_impl.cc:287: // Preferences for whitelist and blacklists On 2012/11/16 15:04:46, Bernhard Bauer wrote: > We usually explain what preferences are for in pref_names.cc. Removed and rephrased in pref_names.cc.
It's not actually as bad as it looks like with the 39 comments ;-) I would just like to make sure that we have a clear separation of concerns which is also exposed in the naming. I.e.: * MMResourceThrottle knows about network requests and notifies the interstitial when it blocks them. * MMInterstitial shows an interstitial UI and notifies the navigation observer and the resource throttle about the result. * MMNavigationObserver records navigations in "preview" mode and updates the whitelist if necessary. Apart from that, the individual components don't need to know that much about each other. For the big picture, we can have an explanation in one place in a comment somewhere. https://codereview.chromium.org/11299035/diff/12010/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11299035/diff/12010/chrome/app/generated_reso... chrome/app/generated_resources.grd:15337: + This URL was already on the whitelist so it was not added again but other URLs may have been added in order to navigate to this page. Hm, I'm not sure if we even want to mention that we may or may not have added other URLs. I think this could be more confusing than helpful. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:270: prefs::kManagedModeWhitelist)->DeepCopy()).Pass(); make_scoped_ptr() already returns a temporary, so you don't need Pass(). https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:276: GetInstance()->managed_profile_->GetPrefs()->GetList( I'm not very happy about accessing members from a static method. Could you delegate to a non-static Impl() method here? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:277: prefs::kManagedModeWhitelist)->DeepCopy()); You could probably use a ScopedListPrefUpdate here instead. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:307: const ListValue *list = GetInstance()->managed_profile_->GetPrefs()->GetList( Asterisk comes directly after the type, and a space afterwards. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:77: // Appends |whitelist| to the manual whitelist(both in URL filter and in Nit: space before opening parenthesis. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:79: static bool AddToManualWhitelist(const base::ListValue& whitelist); What does the return value mean? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:81: // Adds the |url_patter| to the manual lists in the URL filter. Nit: |url_pattern| https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:86: static scoped_ptr<base::ListValue> GetBlacklist(); Is this method used? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:163: static scoped_ptr<base::ListValue> GetWhitelist(); If this method is private, we don't need a separate Impl() method. Also, is this method even used now? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_interstitial.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_interstitial.cc:113: Nit: Unnecessary newline https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:215: DLOG(ERROR) << "--- New navigation observer"; Please remove these log statements before committing. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:219: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); "...OnIOThread()" methods are called on the IO thread. I think you don't need to expose the fact that this method posts a task *to* the IO thread in its name. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:220: if (web_contents()) { You could early-return if web_contents() is NULL. Also, when does that happen? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:253: InfoBarDelegate** dismissed_infobar_delegate) { I'm not sure if the saved lines are worth adding an interface to dismiss and NULL out _arbitrary_ InfoBarDelegate pointers. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:260: *dismissed_infobar_delegate= NULL; Nit: Space before equals sign. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:286: replaced_components.SetHostStr("."+it->host()); Nit: space around plus sign. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:295: // If the URL uses https add the protocol as well. Also since this is the Nit: HTTPS in upper case please, and commas would be nice. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:297: // hostname. ? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:405: ClearInterstitialState(); Nit: newline please. Also, I thought we wanted to trigger this from the ResourceThrottle? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:30: // Dismisses an infobar without user action on the infobar itself. "Dismiss" means a user action (clicking on the little (x) button on the infobar). I think you mean remove? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:34: void SaveNavigatedURL(const GURL& url); This method could be private? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:40: // Marks/clears the current interstitial state. I would like for this class not to know about interstitials. Can you describe these methods purely in terms of how they affect the behavior/state of this class? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:53: // An example regarding the order in which these event take place for Nit: "events" https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:95: bool redirects_completed_; I think we should have an explicit state enum here. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:10: #include "chrome/browser/managed_mode/managed_mode_navigation_observer.h" We don't need this, I think. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:12: #include "chrome/browser/tab_contents/tab_util.h" Why do we need this? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:19: // render_view_id> pairs which identify individual tabs. If a hostname The other way around. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:22: typedef std::map<std::pair<int, int>, std::string > PreviewMap; Nit: no space before > https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:55: if (in_preview_mode.Pointer()) { Why this check? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:74: PreviewMap::iterator it = in_preview_mode.Get().find( You could probably save the map pointer you get here in a local variable. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:76: if ((is_redirect && is_in_preview_mode_) || Hm. If |is_in_preview_mode_| is true, we should see only redirects, so |is_redirect| should also be true. Could you DCHECK that? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:77: (it != in_preview_mode.Get().end() && url.host() == it->second)) You could split these up into separate if-statements for readability. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:26: // Adds/removes the observer characterized by the render_process_host_id and AddObserver makes it sound like the method adds an observer to this object, which is not really what happens. What we do is (temporarily) whitelist all requests for the given RenderView and host? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:35: // |is_redirect| and the user already saw an interstitial which lead to this Nit: the simple past of "lead" is "led". https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:36: // url then do not show another one. Nit: URL in upper case if you use it as a noun, |url| in pipes if you're referring to the variable. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:37: void CheckNeedToShowInterstitial(bool is_redirect, This actually *does* show the interstitial if necessary, right? You should name the method to indicate that. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:55: // an interstitial. I think this class also shouldn't know about preview mode. Can we just talk about this in terms of behavior (i.e. in some cases we show an interstitial, in some cases we don't)? https://codereview.chromium.org/11299035/diff/12010/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/common/pref_names.... chrome/common/pref_names.cc:2150: const char kManagedModeWhitelist[] = "managed_mode.whitelist"; Could you change this to "profile.managed.whitelist"?
Uploaded a new patch set, feedback is welcome as always. Sergiu https://codereview.chromium.org/11299035/diff/12010/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11299035/diff/12010/chrome/app/generated_reso... chrome/app/generated_resources.grd:15337: + This URL was already on the whitelist so it was not added again but other URLs may have been added in order to navigate to this page. On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Hm, I'm not sure if we even want to mention that we may or may not have added > other URLs. I think this could be more confusing than helpful. Just removed the second part for now. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:270: prefs::kManagedModeWhitelist)->DeepCopy()).Pass(); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > make_scoped_ptr() already returns a temporary, so you don't need Pass(). Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:276: GetInstance()->managed_profile_->GetPrefs()->GetList( On 2012/11/26 16:06:30, Bernhard Bauer wrote: > I'm not very happy about accessing members from a static method. Could you > delegate to a non-static Impl() method here? Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:277: prefs::kManagedModeWhitelist)->DeepCopy()); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > You could probably use a ScopedListPrefUpdate here instead. Done, changed to a ListPrefUpdate, which seems to work. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:307: const ListValue *list = GetInstance()->managed_profile_->GetPrefs()->GetList( On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Asterisk comes directly after the type, and a space afterwards. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:77: // Appends |whitelist| to the manual whitelist(both in URL filter and in On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: space before opening parenthesis. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:79: static bool AddToManualWhitelist(const base::ListValue& whitelist); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > What does the return value mean? Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:81: // Adds the |url_patter| to the manual lists in the URL filter. On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: |url_pattern| Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:86: static scoped_ptr<base::ListValue> GetBlacklist(); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Is this method used? Not right now but it will almost surely be used in the future when we add a way to edit the manual blacklist. Should I delete it until then? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:163: static scoped_ptr<base::ListValue> GetWhitelist(); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > If this method is private, we don't need a separate Impl() method. Also, is this > method even used now? Yes, it is by ManagedMode::UpdateWhitelist. Initially it was public but since it was only used in the ManagedMode class I made it private. But yeah, static private methods don't make a lot of sense :). https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_interstitial.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_interstitial.cc:113: On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: Unnecessary newline Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:215: DLOG(ERROR) << "--- New navigation observer"; On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Please remove these log statements before committing. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:219: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > "...OnIOThread()" methods are called on the IO thread. I think you don't need to > expose the fact that this method posts a task *to* the IO thread in its name. Renamed. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:220: if (web_contents()) { On 2012/11/26 16:06:30, Bernhard Bauer wrote: > You could early-return if web_contents() is NULL. > > Also, when does that happen? It can happen in the RemoveObserverFunction when the browser shuts down apparently. I don't think that it should happen when we add the observer so I put a DCHECK in to make sure. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:253: InfoBarDelegate** dismissed_infobar_delegate) { On 2012/11/26 16:06:30, Bernhard Bauer wrote: > I'm not sure if the saved lines are worth adding an interface to dismiss and > NULL out _arbitrary_ InfoBarDelegate pointers. Removed the function. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:260: *dismissed_infobar_delegate= NULL; On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: Space before equals sign. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:286: replaced_components.SetHostStr("."+it->host()); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: space around plus sign. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:295: // If the URL uses https add the protocol as well. Also since this is the On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: HTTPS in upper case please, and commas would be nice. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:297: // hostname. On 2012/11/26 16:06:30, Bernhard Bauer wrote: > ? Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:405: ClearInterstitialState(); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: newline please. Also, I thought we wanted to trigger this from the > ResourceThrottle? We set the Observer state from the interstitial and act according to it here. Or do you suggest we do it another way? https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:30: // Dismisses an infobar without user action on the infobar itself. On 2012/11/26 16:06:30, Bernhard Bauer wrote: > "Dismiss" means a user action (clicking on the little (x) button on the > infobar). I think you mean remove? You were right, it's pretty unsafe to have it set arbitrary pointers to NULL so I just removed the function. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:34: void SaveNavigatedURL(const GURL& url); On 2012/11/26 16:06:30, Bernhard Bauer wrote: > This method could be private? Made private. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:40: // Marks/clears the current interstitial state. On 2012/11/26 16:06:30, Bernhard Bauer wrote: > I would like for this class not to know about interstitials. Can you describe > these methods purely in terms of how they affect the behavior/state of this > class? Refactored the two variables after_interstitial and redirects_completed into a state variable, tell me what you think about that. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:53: // An example regarding the order in which these event take place for On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: "events" Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:95: bool redirects_completed_; On 2012/11/26 16:06:30, Bernhard Bauer wrote: > I think we should have an explicit state enum here. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:10: #include "chrome/browser/managed_mode/managed_mode_navigation_observer.h" On 2012/11/26 16:06:30, Bernhard Bauer wrote: > We don't need this, I think. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:12: #include "chrome/browser/tab_contents/tab_util.h" On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Why do we need this? I needed it, then deleted the call apparently. :( https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:19: // render_view_id> pairs which identify individual tabs. If a hostname On 2012/11/26 16:06:30, Bernhard Bauer wrote: > The other way around. Keys mapped to values, makes sense. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:22: typedef std::map<std::pair<int, int>, std::string > PreviewMap; On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: no space before > Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:55: if (in_preview_mode.Pointer()) { On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Why this check? I was under the impression that in_preview_mode can become null when the browser is closing for example but I think it was just web_contents() that can return null. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:74: PreviewMap::iterator it = in_preview_mode.Get().find( On 2012/11/26 16:06:30, Bernhard Bauer wrote: > You could probably save the map pointer you get here in a local variable. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:76: if ((is_redirect && is_in_preview_mode_) || On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Hm. If |is_in_preview_mode_| is true, we should see only redirects, so > |is_redirect| should also be true. Could you DCHECK that? Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:77: (it != in_preview_mode.Get().end() && url.host() == it->second)) On 2012/11/26 16:06:30, Bernhard Bauer wrote: > You could split these up into separate if-statements for readability. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:26: // Adds/removes the observer characterized by the render_process_host_id and On 2012/11/26 16:06:30, Bernhard Bauer wrote: > AddObserver makes it sound like the method adds an observer to this object, > which is not really what happens. What we do is (temporarily) whitelist all > requests for the given RenderView and host? Renamed. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:35: // |is_redirect| and the user already saw an interstitial which lead to this On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: the simple past of "lead" is "led". Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:36: // url then do not show another one. On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Nit: URL in upper case if you use it as a noun, |url| in pipes if you're > referring to the variable. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:37: void CheckNeedToShowInterstitial(bool is_redirect, On 2012/11/26 16:06:30, Bernhard Bauer wrote: > This actually *does* show the interstitial if necessary, right? You should name > the method to indicate that. Done. https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:55: // an interstitial. On 2012/11/26 16:06:30, Bernhard Bauer wrote: > I think this class also shouldn't know about preview mode. Can we just talk > about this in terms of behavior (i.e. in some cases we show an interstitial, in > some cases we don't)? Renamed it to after_interstitial_ which I think fits and relates to the ResourceThrottle. https://codereview.chromium.org/11299035/diff/12010/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11299035/diff/12010/chrome/common/pref_names.... chrome/common/pref_names.cc:2150: const char kManagedModeWhitelist[] = "managed_mode.whitelist"; On 2012/11/26 16:06:30, Bernhard Bauer wrote: > Could you change this to "profile.managed.whitelist"? Done.
https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/12010/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:86: static scoped_ptr<base::ListValue> GetBlacklist(); On 2012/11/27 15:37:34, Sergiu wrote: > On 2012/11/26 16:06:30, Bernhard Bauer wrote: > > Is this method used? > > Not right now but it will almost surely be used in the future when we add a way > to edit the manual blacklist. Should I delete it until then? It's fine if you move it together with GetWhiteList(). https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.cc:267: // static This method isn't static. The one above is :-p https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.cc:275: unsigned int position = 0; Hm, we might simply do a regular for-loop with a size_t instead. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:76: bool IsInManualWhitelistImpl(const std::string& url_pattern); Make this private please. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:80: // in the manual whitelist or not. That seems… very specific. Could a client just use IsInManualWhitelist() for the last element before adding them? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:82: bool AddToManualWhitelistImpl(const base::ListValue& whitelist); Make this private. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:210: state_(RECORDING_URLS_BEFORE_PREVIEW), Ohhhh! I was assuming that "not recording" is the default state. What do we record in this state? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:230: if (!web_contents()) Can you add a comment to explain when web_contents() might be NULL? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:286: state_ != RECORDING_URLS_BEFORE_PREVIEW) { So, IIUC we have multiple entry points to this method (accepting the infobar, and ending up on a page that's already allowed), and then we use this check to distinguish them again, because we only want to show the message in the latter case. Couldn't we just do that at the call site instead? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:437: if (preview_infobar_delegate_) { Does this actually happen now or do we remove the infobar beforehand? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:22: // add/remove the current observer to/from that map. The map is from RenderView to hostname, not from observer (which observer?). https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:23: void AddTemporaryException(); Can you make these methods private? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:87: enum ObserverState { Type declarations like enums come before methods. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:88: RECORDING_URLS_BEFORE_PREVIEW, We should have some explanation for these states. See also my comment in the .cc file, where I originally assumed the states meant something completely different. Now I'm just confused ;-) https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:93: ObserverState state_; I think it would be nice if we could group state members together, and pointers to other things together, i.e. move this below. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:21: static base::LazyInstance<PreviewMap> "static" isn't necessary if you're in an anonymous namespace. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:22: in_preview_mode = LAZY_INSTANCE_INITIALIZER; Maybe name this |g_in_preview_mode|? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.h:35: // Checks whether the |url| needs to trigger an interstitial. If it Update the comment now? We do more than check, also the description of the logic isn't fully accurate. Maybe we can just leave it out (we have a description in the implementation)? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.h:38: void ShowInterstitialIfNeeded(bool is_redirect, Also, make this method private?
Taking another go at it :) Sergiu https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.cc:267: // static On 2012/11/27 18:44:46, Bernhard Bauer wrote: > This method isn't static. The one above is :-p Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.cc:275: unsigned int position = 0; On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Hm, we might simply do a regular for-loop with a size_t instead. Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:76: bool IsInManualWhitelistImpl(const std::string& url_pattern); On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Make this private please. Done. I'm not sure about the order of the functions in .h though.. It makes more sense to me to keep the non-Impl close to the Impl in the .cc but then they're not really grouped by visibility. Any tips here? https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:80: // in the manual whitelist or not. On 2012/11/27 18:44:46, Bernhard Bauer wrote: > That seems… very specific. Could a client just use IsInManualWhitelist() for the > last element before adding them? I'm not sure I get you here, AddToManualWhitelist adds a whole whitelist, not just an entry. The bool is returned then to show the infobar if we didn't add the last one. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:82: bool AddToManualWhitelistImpl(const base::ListValue& whitelist); On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Make this private. Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:210: state_(RECORDING_URLS_BEFORE_PREVIEW), On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Ohhhh! I was assuming that "not recording" is the default state. What do we > record in this state? By default we record all the URLs that are blacklisted until we hit an interstitial. After the user clicks "Preview" we still record all redirects that would be blacklisted and once they are complete we do not record any more. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:230: if (!web_contents()) On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Can you add a comment to explain when web_contents() might be NULL? Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:286: state_ != RECORDING_URLS_BEFORE_PREVIEW) { On 2012/11/27 18:44:46, Bernhard Bauer wrote: > So, IIUC we have multiple entry points to this method (accepting the infobar, > and ending up on a page that's already allowed), and then we use this check to > distinguish them again, because we only want to show the message in the latter > case. Couldn't we just do that at the call site instead? It should work, yes, I've refactored and simplified a bit, moved the infobar call in the call site. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:437: if (preview_infobar_delegate_) { On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Does this actually happen now or do we remove the infobar beforehand? No, it should be already removed every time we clear the state. Removed this part. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:22: // add/remove the current observer to/from that map. On 2012/11/27 18:44:46, Bernhard Bauer wrote: > The map is from RenderView to hostname, not from observer (which observer?). Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:23: void AddTemporaryException(); On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Can you make these methods private? Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:87: enum ObserverState { On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Type declarations like enums come before methods. Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:88: RECORDING_URLS_BEFORE_PREVIEW, On 2012/11/27 18:44:46, Bernhard Bauer wrote: > We should have some explanation for these states. See also my comment in the .cc > file, where I originally assumed the states meant something completely > different. Now I'm just confused ;-) I've commented the states near the definition, please provide feedback. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:93: ObserverState state_; On 2012/11/27 18:44:46, Bernhard Bauer wrote: > I think it would be nice if we could group state members together, and pointers > to other things together, i.e. move this below. Hopefully better now. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:21: static base::LazyInstance<PreviewMap> On 2012/11/27 18:44:46, Bernhard Bauer wrote: > "static" isn't necessary if you're in an anonymous namespace. Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:22: in_preview_mode = LAZY_INSTANCE_INITIALIZER; On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Maybe name this |g_in_preview_mode|? Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.h:35: // Checks whether the |url| needs to trigger an interstitial. If it On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Update the comment now? We do more than check, also the description of the logic > isn't fully accurate. Maybe we can just leave it out (we have a description in > the implementation)? Done. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.h:38: void ShowInterstitialIfNeeded(bool is_redirect, On 2012/11/27 18:44:46, Bernhard Bauer wrote: > Also, make this method private? Done.
https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:76: bool IsInManualWhitelistImpl(const std::string& url_pattern); On 2012/11/28 12:53:25, Sergiu wrote: > On 2012/11/27 18:44:46, Bernhard Bauer wrote: > > Make this private please. > > Done. I'm not sure about the order of the functions in .h though.. You mean in the .cc file? In the .h file we don't have a choice. > It makes more > sense to me to keep the non-Impl close to the Impl in the .cc but then they're > not really grouped by visibility. Any tips here? I don't have a particular preference. Personally, I'm not very good at keeping method definitions in the same order as declarations, so I would be okay with grouping them. https://codereview.chromium.org/11299035/diff/4015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.h:80: // in the manual whitelist or not. On 2012/11/28 12:53:25, Sergiu wrote: > On 2012/11/27 18:44:46, Bernhard Bauer wrote: > > That seems… very specific. Could a client just use IsInManualWhitelist() for > the > > last element before adding them? > > I'm not sure I get you here, AddToManualWhitelist adds a whole whitelist, not > just an entry. The bool is returned then to show the infobar if we didn't add > the last one. Yes, it's just that an interface "add these, and tell me if the last one of them was already on the list" is very specific to the client that calls it. So I was asking if we could simplify the interface of this class by using other methods (like the one we already have that tells whether a given site is on the whitelist), which I see you did. So... 'kay. https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:287: if (state_ == NOT_RECORDING_URLS) This check is unnecessary (or should be converted to a DCHECK). https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:20: // navigates to a new page. In this state the navigated URLs are recorded. Do we record all navigated URLs in this state, or only the blocked ones?> https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:21: // An interstitial was not shown while in this state. This sentence is difficult to understand. I think what you mean is, we only go to the next state if an interstitial has been shown and the user clicked "preview"? https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:27: enum ObserverState { This enum doesn't need to be public, it just needs to be declared at the top of its (private) section. https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.h:42: void ShowInterstitialIfNeeded(bool is_redirect, Small nit: If you switch the order of these two methods, it corresponds to the order in which they're called.
As discussed I have split navigated_urls_ in two, a list of url patterns and a final url pattern string, see what you think of it. https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:287: if (state_ == NOT_RECORDING_URLS) On 2012/11/28 14:21:05, Bernhard Bauer wrote: > This check is unnecessary (or should be converted to a DCHECK). Done. https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:20: // navigates to a new page. In this state the navigated URLs are recorded. On 2012/11/28 14:21:05, Bernhard Bauer wrote: > Do we record all navigated URLs in this state, or only the blocked ones?> Done. https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:21: // An interstitial was not shown while in this state. On 2012/11/28 14:21:05, Bernhard Bauer wrote: > This sentence is difficult to understand. I think what you mean is, we only go > to the next state if an interstitial has been shown and the user clicked > "preview"? Done. https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.h:27: enum ObserverState { On 2012/11/28 14:21:05, Bernhard Bauer wrote: > This enum doesn't need to be public, it just needs to be declared at the top of > its (private) section. Done. https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/1015/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_resource_throttle.h:42: void ShowInterstitialIfNeeded(bool is_redirect, On 2012/11/28 14:21:05, Bernhard Bauer wrote: > Small nit: If you switch the order of these two methods, it corresponds to the > order in which they're called. Done.
https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.cc:278: pref_whitelist->AppendIfNotPresent(new StringValue(url_pattern)); Wait, why do we need this if we have the IsInManualWhitelist() check above? https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:267: whitelist.AppendString(last_url_pattern_); Append all the url_patterns_ to |whitelist|, then pass it to ManagedMode::AddToManualWhitelist()? https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:282: // Search for the last |url| to see if present in the |url_patterns_| Nit: "[…] to see if it is present […]" https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:287: base::StringValue value_to_find(url.scheme() + "://." + url.host()); It might be worth it to extract this to a method. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:289: std::find_if(url_patterns_.begin(), url_patterns_.end(), I think this would be a lot easier if you'd store |url_patterns| as a std::vector<std::string>; then you don't have to work around the interface of base::ListValue. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:327: last_allowed_page_ || Nit: Indent this four more spaces? https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:328: last_url_pattern_ != url.host()) { If url is HTTPS, this check won't be correct, I think. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:354: AddSavedURLsToWhitelist(); Should we also reset the state here (or do that in AddSavedURLsToWhitelist())? https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:361: true)); Nit: I think doing an early-return in this case would be nicer, because this is sort of a special case. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:363: // Only add an exception when the final page is not allowed. OK, I can infer that behavior != ALLOW if I think about it, but it would be nice to explain that in a bit more detail. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:431: behavior == ManagedModeURLFilter::BLOCK) { Hm, we committed a load for a site that should be blocked. I think we should DCHECK that we're in RECORDING_URLS_AFTER_PREVIEW state.
Changed urls_patterns to set<GURL> and renamed them because of that, should be cleaner now. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode.cc:278: pref_whitelist->AppendIfNotPresent(new StringValue(url_pattern)); On 2012/12/03 10:23:37, Bernhard Bauer wrote: > Wait, why do we need this if we have the IsInManualWhitelist() check above? Just adding it now. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:267: whitelist.AppendString(last_url_pattern_); On 2012/12/03 10:23:37, Bernhard Bauer wrote: > Append all the url_patterns_ to |whitelist|, then pass it to > ManagedMode::AddToManualWhitelist()? Refactored with GURLs now. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:282: // Search for the last |url| to see if present in the |url_patterns_| On 2012/12/03 10:23:37, Bernhard Bauer wrote: > Nit: "[…] to see if it is present […]" Done. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:287: base::StringValue value_to_find(url.scheme() + "://." + url.host()); On 2012/12/03 10:23:37, Bernhard Bauer wrote: > It might be worth it to extract this to a method. Refactored, see new version. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:289: std::find_if(url_patterns_.begin(), url_patterns_.end(), On 2012/12/03 10:23:37, Bernhard Bauer wrote: > I think this would be a lot easier if you'd store |url_patterns| as a > std::vector<std::string>; then you don't have to work around the interface of > base::ListValue. Done, back to set<GURL> now which I think is better. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:327: last_allowed_page_ || On 2012/12/03 10:23:37, Bernhard Bauer wrote: > Nit: Indent this four more spaces? Done. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:328: last_url_pattern_ != url.host()) { On 2012/12/03 10:23:37, Bernhard Bauer wrote: > If url is HTTPS, this check won't be correct, I think. Nice catch, done. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:354: AddSavedURLsToWhitelist(); On 2012/12/03 10:23:37, Bernhard Bauer wrote: > Should we also reset the state here (or do that in AddSavedURLsToWhitelist())? Done, clearing the state in the AddSavedTo[...] https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:361: true)); On 2012/12/03 10:23:37, Bernhard Bauer wrote: > Nit: I think doing an early-return in this case would be nicer, because this is > sort of a special case. Done. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:363: // Only add an exception when the final page is not allowed. On 2012/12/03 10:23:37, Bernhard Bauer wrote: > OK, I can infer that behavior != ALLOW if I think about it, but it would be nice > to explain that in a bit more detail. Done. https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:431: behavior == ManagedModeURLFilter::BLOCK) { On 2012/12/03 10:23:37, Bernhard Bauer wrote: > Hm, we committed a load for a site that should be blocked. I think we should > DCHECK that we're in RECORDING_URLS_AFTER_PREVIEW state. Actually checking that we're in AFTER_PREVIEW should be enough so I changed it to that.
We're converging! :) https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:431: behavior == ManagedModeURLFilter::BLOCK) { On 2012/12/04 14:02:31, Sergiu wrote: > On 2012/12/03 10:23:37, Bernhard Bauer wrote: > > Hm, we committed a load for a site that should be blocked. I think we should > > DCHECK that we're in RECORDING_URLS_AFTER_PREVIEW state. > > Actually checking that we're in AFTER_PREVIEW should be enough so I changed it > to that. What I meant was that if we commit a navigation for a URL that should be blocked, it can only happen if we are in the RECORDING_URLS_AFTER_PREVIEW state, so we don't need to check that in production code, but we can make it a DCHECK instead to verify this assumption. https://codereview.chromium.org/11299035/diff/12026/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/12026/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:7: #include <set> Why do we need this here, but not in the header?
https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/2004/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:431: behavior == ManagedModeURLFilter::BLOCK) { On 2012/12/04 14:25:00, Bernhard Bauer wrote: > On 2012/12/04 14:02:31, Sergiu wrote: > > On 2012/12/03 10:23:37, Bernhard Bauer wrote: > > > Hm, we committed a load for a site that should be blocked. I think we should > > > DCHECK that we're in RECORDING_URLS_AFTER_PREVIEW state. > > > > Actually checking that we're in AFTER_PREVIEW should be enough so I changed it > > to that. > > What I meant was that if we commit a navigation for a URL that should be > blocked, it can only happen if we are in the RECORDING_URLS_AFTER_PREVIEW state, > so we don't need to check that in production code, but we can make it a DCHECK > instead to verify this assumption. Done. https://codereview.chromium.org/11299035/diff/12026/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/12026/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:7: #include <set> On 2012/12/04 14:25:01, Bernhard Bauer wrote: > Why do we need this here, but not in the header? Header makes more sense, moved.
https://codereview.chromium.org/11299035/diff/5019/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/5019/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:441: behavior == ManagedModeURLFilter::BLOCK) { Can you restructure this into one big switch statement on the |state_| (inside of the behavior check)?
Hopefully it's ok now. :) https://codereview.chromium.org/11299035/diff/5019/chrome/browser/managed_mod... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/5019/chrome/browser/managed_mod... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:441: behavior == ManagedModeURLFilter::BLOCK) { On 2012/12/04 15:50:31, Bernhard Bauer wrote: > Can you restructure this into one big switch statement on the |state_| (inside > of the behavior check)? Done.
https://codereview.chromium.org/11299035/diff/10017/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/10017/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:456: default: This is not necessary; the compiler will check for you that you handled all cases.
https://codereview.chromium.org/11299035/diff/10017/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/10017/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:456: default: On 2012/12/04 16:27:25, Bernhard Bauer wrote: > This is not necessary; the compiler will check for you that you handled all > cases. Assuming you remove these two lines, LGTM.
https://codereview.chromium.org/11299035/diff/19001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11299035/diff/19001/chrome/app/generated_reso... chrome/app/generated_resources.grd:15337: + This URL was already on the whitelist so it was not added again. nit: comma after "whitelist" nit2: Consider "page" or "site" instead of "URL" -- less geeky https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:79: void AddURLPatternToManualWhitelist(const std::string& url) { Use url_pattern for the argument, to match the header. (Also in Blacklist method.) https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:78: // preferences). Please describe what |whitelist| is (presumably, a list of URL patterns). Also, the separation between the whitelist in the prefs and the whitelist in the URL filter (which is really just a parsed/cached copy of the one in the prefs) is pretty hard to follow. I'd suggest requiring that the two are always in sync (that is, not having public AddURLPatternToManual*List methods that only do one part). Am I missing a use case for that? https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:162: // Returns a copy of the manual whitelist which is stored in each profile. Meaning the one from the prefs? Also, what format is the list in (again, I suppose it's a list of std::string URL patterns)? If possible, define a type for the list, so it's self-evident what to expect. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:195: // Taken from shill_manager_client.cc as ListValue returns a const_iterator Not sure what file you mean, but there's a typo in there somewhere. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:37: // Whitelist exact URLs for redirects and host patterns for the final URL. nit: please be descriptive rather than imperative ("Whitelists..." instead of "Whitelist..."), matching the other comments in the file (as well as the approved comment style). https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:49: // still recorded while the page redirect. The Observer moves to the next nit: ...redirects. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:65: // an interstitial for this RenderView. When would this be used? Is it so you can navigate around during preview? https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:47: url.host())); Is there not a more readable way to write this? std::map is pretty good about using []. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:61: // Only treat main frame requests for now (without subresources). This makes it sound like you only treat main-frame requests that don't have any subresources. Try "ignoring subresources" or simply "not subresources". https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:67: return; Using brackets with a multi-line condition would be more readable. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:28: // Adding saves the last approved hostname in the map. I'm having trouble following these comments. I'm not even sure what to ask to clarify... maybe I'll start with "What's the preview map?" https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:197: return BLOCK; We need to rename IsURLBlocked to something like ContainsURL. Also, try if (manual block) return BLOCK; if (manual allow) return ALLOW; // Check the list...
I've uploaded a new version pulled out from our latest patch. This was applied on ToT, I fixed some stuff and then rebased again and uploaded in this old issue, hope I didn't delete things / add things I shouldn't have. https://codereview.chromium.org/11299035/diff/19001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11299035/diff/19001/chrome/app/generated_reso... chrome/app/generated_resources.grd:15337: + This URL was already on the whitelist so it was not added again. On 2012/12/06 15:11:56, Pam wrote: > nit: comma after "whitelist" > nit2: Consider "page" or "site" instead of "URL" -- less geeky Done. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:79: void AddURLPatternToManualWhitelist(const std::string& url) { On 2012/12/06 15:11:56, Pam wrote: > Use url_pattern for the argument, to match the header. (Also in Blacklist > method.) Done. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:78: // preferences). On 2012/12/06 15:11:56, Pam wrote: > Please describe what |whitelist| is (presumably, a list of URL patterns). > > Also, the separation between the whitelist in the prefs and the whitelist in the > URL filter (which is really just a parsed/cached copy of the one in the prefs) > is pretty hard to follow. I'd suggest requiring that the two are always in sync > (that is, not having public AddURLPatternToManual*List methods that only do one > part). Am I missing a use case for that? Added a paragraph describing whitelists and url_patterns and made AddURLPatternToManualList a private function. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:162: // Returns a copy of the manual whitelist which is stored in each profile. On 2012/12/06 15:11:56, Pam wrote: > Meaning the one from the prefs? Also, what format is the list in (again, I > suppose it's a list of std::string URL patterns)? If possible, define a type for > the list, so it's self-evident what to expect. I've typedefed base::ListValue to URLPatternList, hopefully it is clearer now. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:195: // Taken from shill_manager_client.cc as ListValue returns a const_iterator On 2012/12/06 15:11:56, Pam wrote: > Not sure what file you mean, but there's a typo in there somewhere. Removed, not needed anymore. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:37: // Whitelist exact URLs for redirects and host patterns for the final URL. On 2012/12/06 15:11:56, Pam wrote: > nit: please be descriptive rather than imperative ("Whitelists..." instead of > "Whitelist..."), matching the other comments in the file (as well as the > approved comment style). Done. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:49: // still recorded while the page redirect. The Observer moves to the next On 2012/12/06 15:11:56, Pam wrote: > nit: ...redirects. Done. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:65: // an interstitial for this RenderView. On 2012/12/06 15:11:56, Pam wrote: > When would this be used? Is it so you can navigate around during preview? Exactly, added a comment to clarify that. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:47: url.host())); On 2012/12/06 15:11:56, Pam wrote: > Is there not a more readable way to write this? std::map is pretty good about > using []. Done. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:61: // Only treat main frame requests for now (without subresources). On 2012/12/06 15:11:56, Pam wrote: > This makes it sound like you only treat main-frame requests that don't have any > subresources. Try "ignoring subresources" or simply "not subresources". Done. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.cc:67: return; On 2012/12/06 15:11:56, Pam wrote: > Using brackets with a multi-line condition would be more readable. Done. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:28: // Adding saves the last approved hostname in the map. On 2012/12/06 15:11:56, Pam wrote: > I'm having trouble following these comments. I'm not even sure what to ask to > clarify... maybe I'll start with "What's the preview map?" Hopefully they're clearer now, the preview map is defined and detailed in the .cc file of resource throttle. https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:197: return BLOCK; On 2012/12/06 15:11:56, Pam wrote: > We need to rename IsURLBlocked to something like ContainsURL. > > Also, try > if (manual block) > return BLOCK; > if (manual allow) > return ALLOW; > > // Check the list... It's called that way because I am using the functions from the URLBlacklist. I've updated the conditions.
https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/19001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:162: // Returns a copy of the manual whitelist which is stored in each profile. On 2013/01/04 15:44:07, Sergiu wrote: > On 2012/12/06 15:11:56, Pam wrote: > > Meaning the one from the prefs? Also, what format is the list in (again, I > > suppose it's a list of std::string URL patterns)? If possible, define a type > for > > the list, so it's self-evident what to expect. > > I've typedefed base::ListValue to URLPatternList, hopefully it is clearer now. I'm sorry, but I disagree. When I see a base::ListValue type, I at least know the class, just not the type of the list contents. With URLPatternList I have to look up the definition of the type (and how do I know where it's defined?), and you don't even gain anything in type safety. The comment is great, but I don't think we should have the typedef. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:68: void LoadManualLists(scoped_ptr<ListValue> whitelist, Can you name this SetManualLists? It's in line with the method on ManagedModeURLFilter, and we don't need to "load" the lists from disk anymore like we do for LoadWhitelists(). https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:530: void ManagedMode::AddURLPatternToManualList( Please keep this method consistent with the style used in other places in the file, i.e. static method forwards to class method instead of directly acessing member variables. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:72: // URLBlacklist filter. A description of the format used can be found here: Nit: policy::URLBacklist is the fully qualified class name, and it tells readers what area the class is coming from. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:175: static void AddURLPatternToManualList(const bool isWhitelist, Can you move this method to an anonymous namespace? Static private methods always trigger a flag for me :) https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:318: DLOG(ERROR) << "NavigateToPendingEntry: " << url; Remove these log statements before committing. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:452: default: The compiler will check for you that you handle all cases if you leave out the default branch. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:457: if (behavior == ManagedModeURLFilter::ALLOW) { Nit: Braces aren't necessary. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:38: // If the URL uses HTTPS whitelists only the host on HTTPS. Clears the Nit: comma after HTTPS please, otherwise the sentence is a bit tricky to parse. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:54: bool after_interstitial_; I would like to have a more descriptive name for this variable. |temporarily_allowed_| maybe? https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:288: DLOG(ERROR) << "Loaded whitelist: "; Debugging log statements are fine, but could you downgrade the importance (or use (D)VLOG)? I don't want to spam other developers' consoles. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl.cc:319: prefs->RegisterListPref(prefs::kManagedModeWhitelist, Please don't lump new preferences into arbitrary places, otherwise it will get really messy real soon. ManagedMode is the class that uses the preferences, and it already has a RegisterUserPrefs() method. https://codereview.chromium.org/11299035/diff/35001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/common/pref_names.... chrome/common/pref_names.h:811: extern const char kManagedModeBlacklist[]; Sorry, nope. These are per-profile preferences, so they should be in the first section. This section is for global preferences.
https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:76: static bool IsInManualList(const bool isWhitelist, Style nit: is_whitelist (also below) Comment nit: Describe |is_whitelist| https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:90: static void UpdateWhitelist(); If this updates both the white- and blacklists, it should have a better name. UpdateURLFilteringLists UpdateWhiteAndBlacklists https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:194: // TODO(sergiu): Find a less confusing way to do this. Maybe add a TODO to renamethe functions in URLBlacklist (and also add an IsURLAllowed() method, as well as perhaps renaming URLBlacklist itself to URLFilterList or similar). https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:295: LOG(ERROR) << "Loaded blacklist: "; ...or users' consoles. (see Bernhard's comment) https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:309: const base::Closure& continuation) { Not used? https://codereview.chromium.org/11299035/diff/35001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/common/pref_names.... chrome/common/pref_names.h:811: extern const char kManagedModeBlacklist[]; On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Sorry, nope. These are per-profile preferences, so they should be in the first > section. This section is for global preferences. Why kInManagedMode is a global preference is left as an exercise for the reader.
Hopefully getting closer :) https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:68: void LoadManualLists(scoped_ptr<ListValue> whitelist, On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Can you name this SetManualLists? It's in line with the method on > ManagedModeURLFilter, and we don't need to "load" the lists from disk anymore > like we do for LoadWhitelists(). Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:530: void ManagedMode::AddURLPatternToManualList( On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Please keep this method consistent with the style used in other places in the > file, i.e. static method forwards to class method instead of directly acessing > member variables. You were right, sorry.. it doesn't need to be static anymore so it will remain private and that's it. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:72: // URLBlacklist filter. A description of the format used can be found here: On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Nit: policy::URLBacklist is the fully qualified class name, and it tells readers > what area the class is coming from. Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:76: static bool IsInManualList(const bool isWhitelist, On 2013/01/07 13:00:16, Pam wrote: > Style nit: is_whitelist (also below) > Comment nit: Describe |is_whitelist| Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:90: static void UpdateWhitelist(); On 2013/01/07 13:00:16, Pam wrote: > If this updates both the white- and blacklists, it should have a better name. > UpdateURLFilteringLists > UpdateWhiteAndBlacklists Done, renamed to UpdateManualLists to fit with the other functions. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:175: static void AddURLPatternToManualList(const bool isWhitelist, On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Can you move this method to an anonymous namespace? Static private methods > always trigger a flag for me :) It shouldn't have been static at all, made it just private and it should be ok. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:318: DLOG(ERROR) << "NavigateToPendingEntry: " << url; On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Remove these log statements before committing. Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:452: default: On 2013/01/07 12:34:19, Bernhard Bauer wrote: > The compiler will check for you that you handle all cases if you leave out the > default branch. Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:457: if (behavior == ManagedModeURLFilter::ALLOW) { On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Nit: Braces aren't necessary. Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.h:38: // If the URL uses HTTPS whitelists only the host on HTTPS. Clears the On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Nit: comma after HTTPS please, otherwise the sentence is a bit tricky to parse. Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_resource_throttle.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_resource_throttle.h:54: bool after_interstitial_; On 2013/01/07 12:34:19, Bernhard Bauer wrote: > I would like to have a more descriptive name for this variable. > |temporarily_allowed_| maybe? Sounds better indeed. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:194: // TODO(sergiu): Find a less confusing way to do this. On 2013/01/07 13:00:16, Pam wrote: > Maybe add a TODO to renamethe functions in URLBlacklist (and also add an > IsURLAllowed() method, as well as perhaps renaming URLBlacklist itself to > URLFilterList or similar). Well since URLBlacklist is used a lot in the policy stuff I'm not sure how they would feel about renaming it. I've expanded the TODO for now. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:288: DLOG(ERROR) << "Loaded whitelist: "; On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Debugging log statements are fine, but could you downgrade the importance (or > use (D)VLOG)? I don't want to spam other developers' consoles. Changed to use DVLOG(1) as recommended. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:295: LOG(ERROR) << "Loaded blacklist: "; On 2013/01/07 13:00:16, Pam wrote: > ...or users' consoles. (see Bernhard's comment) Done. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:309: const base::Closure& continuation) { On 2013/01/07 13:00:16, Pam wrote: > Not used? Removed. https://codereview.chromium.org/11299035/diff/35001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/11299035/diff/35001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl.cc:319: prefs->RegisterListPref(prefs::kManagedModeWhitelist, On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Please don't lump new preferences into arbitrary places, otherwise it will get > really messy real soon. ManagedMode is the class that uses the preferences, and > it already has a RegisterUserPrefs() method. Got it, I was under the wrong impression that the prefs should go in here, moved it in managed_mode.cc. https://codereview.chromium.org/11299035/diff/35001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11299035/diff/35001/chrome/common/pref_names.... chrome/common/pref_names.h:811: extern const char kManagedModeBlacklist[]; On 2013/01/07 12:34:19, Bernhard Bauer wrote: > Sorry, nope. These are per-profile preferences, so they should be in the first > section. This section is for global preferences. Got it, didn't see that there were separate sections initially.
Getting there ;-) I found an additional nit I hadn't noticed before, sorry. https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:78: void AddURLPatternToManualList(const bool is_whitelist, const bool is a bit overkill. https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.h (right): https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.h:18: } // namespace policy Nit: two spaces before comment. https://codereview.chromium.org/11299035/diff/49001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11299035/diff/49001/chrome/common/pref_names.... chrome/common/pref_names.cc:2201: const char kInManagedMode[] = "managed_mode"; Can you also move these?
LGTM with nits (mine and Bernhard's) squashed. - Pam https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.h (right): https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.h:74: // Checks if the |url_pattern| is in the manual whitelist. "...whitelist (if |is_whitelist| is true) or blacklist (if not)."
https://codereview.chromium.org/11299035/diff/46002/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11299035/diff/46002/chrome/common/pref_names.... chrome/common/pref_names.h:23: extern const char kInManagedMode[]; Hang on -- unless Bernhard's work is finished, this one still is a global.
https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode.cc (right): https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode.cc:78: void AddURLPatternToManualList(const bool is_whitelist, On 2013/01/07 17:31:49, Bernhard Bauer wrote: > const bool is a bit overkill. Done. https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.h (right): https://codereview.chromium.org/11299035/diff/49001/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.h:18: } // namespace policy On 2013/01/07 17:31:49, Bernhard Bauer wrote: > Nit: two spaces before comment. Done. https://codereview.chromium.org/11299035/diff/49001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/11299035/diff/49001/chrome/common/pref_names.... chrome/common/pref_names.cc:2201: const char kInManagedMode[] = "managed_mode"; On 2013/01/07 17:31:49, Bernhard Bauer wrote: > Can you also move these? Done. https://codereview.chromium.org/11299035/diff/46002/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11299035/diff/46002/chrome/common/pref_names.... chrome/common/pref_names.h:23: extern const char kInManagedMode[]; On 2013/01/08 09:17:17, Pam wrote: > Hang on -- unless Bernhard's work is finished, this one still is a global. Whoops, I didn't actually move it anyway :( fixed.
LGTM. - Pam
LGTM with one nit below. Also, description nit: You should add a reference to "managed users" or "managed mode" here, otherwise no one would know what black-/whitelists we're talking about. https://codereview.chromium.org/11299035/diff/49015/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11299035/diff/49015/chrome/common/pref_names.... chrome/common/pref_names.h:811: extern const char kInManagedMode[]; Mini-nit: Can you add the newline back? kInManagedMode is different from the network stuff below.
Commiting now. https://codereview.chromium.org/11299035/diff/49015/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/11299035/diff/49015/chrome/common/pref_names.... chrome/common/pref_names.h:811: extern const char kInManagedMode[]; On 2013/01/08 12:15:30, Bernhard Bauer wrote: > Mini-nit: Can you add the newline back? kInManagedMode is different from the > network stuff below. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11299035/46004
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11299035/49016
Found another nit, sorry ;-) https://codereview.chromium.org/11299035/diff/49016/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/49016/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:290: for (ListValue::iterator it = whitelist.get()->begin(); scoped_ptr has the "->" operator overloaded, so you don't need the .get().
On 2013/01/08 13:50:10, Bernhard Bauer wrote: > Found another nit, sorry ;-) > > https://codereview.chromium.org/11299035/diff/49016/chrome/browser/managed_mo... > File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): > > https://codereview.chromium.org/11299035/diff/49016/chrome/browser/managed_mo... > chrome/browser/managed_mode/managed_mode_url_filter.cc:290: for > (ListValue::iterator it = whitelist.get()->begin(); > scoped_ptr has the "->" operator overloaded, so you don't need the .get(). Feel free to fix it in a follow-up commit though if this one sticks.
Retried try job too often on linux_aura for step(s) unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11299035/59001
Retried try job too often on linux_rel for step(s) unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11299035/67002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11299035/67002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11299035/67002
This keeps failing for some tests that seem unrelated, can you look at my latest changes and suggest any solutions?. https://codereview.chromium.org/11299035/diff/49016/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/49016/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:290: for (ListValue::iterator it = whitelist.get()->begin(); On 2013/01/08 13:50:10, Bernhard Bauer wrote: > scoped_ptr has the "->" operator overloaded, so you don't need the .get(). Done.
If the tests don't seem related, and it's not even consistently the same test that fails, then you have two options: keep trying, or commit without using the commit queue. Well, three: you could also fix all the flaky tests. :) https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:95: last_allowed_page))); nit: Ugh. Could you please use a local variable or two so it's readable? https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:171: return infobar_service->AddInfoBar(scoped_ptr<InfoBarDelegate>( Same here. https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:165: // Initialize the manual whitelist to block everything, otherwise all the Wait, what? Shouldn't the whitelist only ever allow things, and the blacklist be what's blocking everything?
On 2013/01/09 11:51:24, Sergiu wrote: > This keeps failing for some tests that seem unrelated, can you look at my latest > changes and suggest any solutions?. Try again? I think the commit queue might actually be able to deal with intermittent failures like these. https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:166: // websites are allowed until SetManualLists is called. I think this comment creates more confusion than it clears up. The effect that we want to have is that both the manual whitelist and blacklist should be empty. Could you simply call SetManualLists() with empty lists?
Retried try job too often on win_aura for step(s) content_browsertests
Done, retrying to commit https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right): https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:95: last_allowed_page))); On 2013/01/09 12:00:41, Pam wrote: > nit: Ugh. Could you please use a local variable or two so it's readable? Done. https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:171: return infobar_service->AddInfoBar(scoped_ptr<InfoBarDelegate>( On 2013/01/09 12:00:41, Pam wrote: > Same here. Done. https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_navigation_observer.cc:171: return infobar_service->AddInfoBar(scoped_ptr<InfoBarDelegate>( On 2013/01/09 12:00:41, Pam wrote: > Same here. Done. https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... File chrome/browser/managed_mode/managed_mode_url_filter.cc (right): https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:165: // Initialize the manual whitelist to block everything, otherwise all the On 2013/01/09 12:00:41, Pam wrote: > Wait, what? Shouldn't the whitelist only ever allow things, and the blacklist be > what's blocking everything? Done. https://codereview.chromium.org/11299035/diff/67002/chrome/browser/managed_mo... chrome/browser/managed_mode/managed_mode_url_filter.cc:166: // websites are allowed until SetManualLists is called. On 2013/01/09 12:01:36, Bernhard Bauer wrote: > I think this comment creates more confusion than it clears up. The effect that > we want to have is that both the manual whitelist and blacklist should be empty. > Could you simply call SetManualLists() with empty lists? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11299035/64004
It's hitting that NOTIMPLEMENTED in desktop_root_window_host_win.cc pretty consistently, actually. I'll see if I can reproduce it locally.
Message was sent while issue was closed.
Change committed as 175806 |
