Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11186002:
Add a SafeSearch preference, policy and implementation. (Closed)
|
Created:
8 years, 2 months ago by Sergiu Modified:
8 years, 1 month ago Reviewers:
Bernhard Bauer, Peter Kasting, willchan no longer on Chromium, Pam (message me for reviews), Elliot Glaysher, Sergiu (please use other one), Nico, Joao da Silva, battre CC:
chromium-reviews, Aaron Boodman, cbentzel+watch_chromium.org, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, Bernhard Bauer Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a SafeSearch preference, policy and implementation.
When the SafeSearch preference is set and the user searches on Google the
safe=active&ssui=on parameters are added at the end of the query if they are
not already set to the correct value. We do this after the request was handled by
extensions.
Also added a policy tied to the SafeSearch preference and both unit tests for the
SafeSearch code and browser tests for the policy part.
TBR=thakis@chromium.org
BUG=159241
TEST=Included unit and browser tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166656
Patch Set 1 #Patch Set 2 : Add a SafeSearch preference, policy and implementation. #
Total comments: 82
Patch Set 3 : Updated version with improvements and new mechanism. #
Total comments: 22
Patch Set 4 : Fine grain parameter match + more tests. #
Total comments: 30
Patch Set 5 : Minor fixes and refactoring from comments #
Total comments: 24
Patch Set 6 : More improvements based on comments. #Patch Set 7 : Minor fixes that got by the last patch set. #
Total comments: 34
Patch Set 8 : Changes suggested by comments. #
Total comments: 13
Patch Set 9 : Minor last changes. Merged with the latest version from the tree. #
Total comments: 6
Patch Set 10 : Minor improvement and bugfix. #
Total comments: 4
Patch Set 11 : Removed SupportsSafeSearch from google_util. #
Total comments: 1
Patch Set 12 : Improved SafeSearch policy test to prevent flakyness. #Patch Set 13 : Removed an extra line. #Patch Set 14 : Disable the policy unittest until we resolve the flakyness on trybots. #Patch Set 15 : Added IO thread to unit test. #Patch Set 16 : Policy browser test improvements. #Patch Set 17 : Rebased with the latest tree version #
Total comments: 1
Messages
Total messages: 50 (0 generated)
Comments/observations are welcome. Please try this on the trybots on my behalf as well.
some first feedback... have to catch a plane now. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:234: if (enable_safesearch_ && enable_safesearch_->GetValue()) { how about moving lines 235-264 into a function? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:249: // Make sure that the last our parameters are either the only ones nit: grammar in sentence above "the last our" https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:251: if ((query.rfind(chrome::kSSearchParamsSingle) != std::string::npos && KSSearch --> this is again an abbreviation that is not obvious. Please expand the abbrev. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:252: query.length() == strlen(chrome::kSSearchParamsSingle)) || if query contains kSSearchParamsSingle and has the same length it is equal right? Why don't you test that? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:304: mod_by_managed_mode_.erase(request); I would consider adding a return statement here and deleting the else below. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:133: BooleanPrefMember* enable_safesearch_; how about renaming this to enforce_google_safesearch_? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:147: std::set<net::URLRequest*> mod_by_managed_mode_; I think we should rename this to "modified_by_managed_mode_" even it if its long. We don't allow abbreviations. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:124: const bool exact) { style nit: all parameters should be aligned at the same column if they don't fit into one row. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:128: TestURLRequest url_req( s/url_req/request/ https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:142: loop_ = NULL; how about a scoped_ptr? do you need to use a pointer here? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:153: TestDelegate tdelegate_; s/tdelegate_/delegate_/
The policy changes look good to me (nice test there!). I've left a couple other comments. https://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:571: 'desc': '''Enables <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>'s SafeSearch feature and prevents users from changing this setting. SafeSearch is not a Google Chrome feature, it's a Google feature. Remove the <ph> element and just write Google in its place. https://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:579: If this policy is left not set users will be able to choose their own SafeSearch setting''', End the sentence with a dot. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:7: #include <string.h> // For strlen nit: newline https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:254: return net::OK; |request| is not added to mod_by_managed_mode_ in this case? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:264: mod_by_managed_mode_.insert(request); Suggestion: instead of using this std::set, why not store a private key in the request? net::URLRequest implements base::SupportsUserData, and you can attach anything to it. In this case, you could attach a bogus value (e.g. a new int) and then later test that GetUserData(g_safe_search_key) is not NULL. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:303: if (mod_by_managed_mode_.find(request) != mod_by_managed_mode_.end()) { I'm not sure about this, because the extension will see other events (OnHeadersReceived, OnBeforeSendHeaders, etc) but not these. This might confuse some extensions that expect to see them all. I leave it to Dominic to decide if this is acceptable. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:147: std::set<net::URLRequest*> mod_by_managed_mode_; Use a more descriptive name: modified_by_managed_mode_; https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:154: MessageLoopForIO* loop_; Use a scoped_ptr https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:217: // Test that another website is not affected, without parameters nit: terminate sentence https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:220: // Test that another website is not affected, with parameters nit: terminate sentence https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:223: // Test that another website is not affected with redirects, with parameters nit: terminate sentence https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:233: // Test the home page nit: terminate sentences in this test too https://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/poli... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/poli... chrome/browser/policy/policy_browsertest.cc:664: GURL expected_wo("http://google.com/"); Use a descriptive variable name, such as |expected_without|. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/poli... chrome/browser/policy/policy_browsertest.cc:684: // Verify that searching from the google.com works nit: terminate sentence https://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/poli... chrome/browser/policy/policy_browsertest.cc:694: GURL expected_w(expected_url); Use a descriptive name: |expected_with_params| https://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constant... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constant... chrome/common/url_constants.cc:499: const char kSSearchParamsMultiple[] = "&safe=active&ssui=on"; This is redundant; the first constant is enough and the & can be inserted manually where appropriate.
https://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:560: 'name': 'SafeSearchEnabled', I would be happier if we called this "ForceSafeSearch" or something. "Enabled" sounds like the possibility of using SafeSearch (and conversely, setting the policy to false would imply that SafeSearch can't be used, when in reality it just means that it's not enforced). https://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:577: If you enable or disable this setting, users cannot change or override this setting in <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>. This setting is not accessible to users at all. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:234: if (enable_safesearch_ && enable_safesearch_->GetValue()) { On 2012/10/16 16:05:49, battre wrote: > how about moving lines 235-264 into a function? Yesplz. That would make unit-testing it a lot easier. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:238: request->url().spec()); Can we add a sanity-DCHECK that |is_search_url| and |is_homepage_url| are mutually exclusive? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:239: if (mod_by_managed_mode_.find(request) == mod_by_managed_mode_.end() && Can OnBeforeURLRequest() be called multiple times for the same request? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:244: if (is_homepage_url && query.length() == 0) { `query.length() == 0` can be written a bit shorter as `query.empty()`. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:254: return net::OK; Nit: newline https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:303: if (mod_by_managed_mode_.find(request) != mod_by_managed_mode_.end()) { On 2012/10/16 16:26:46, Joao da Silva wrote: > I'm not sure about this, because the extension will see other events > (OnHeadersReceived, OnBeforeSendHeaders, etc) but not these. This might confuse > some extensions that expect to see them all. I leave it to Dominic to decide if > this is acceptable. +1. Can't we just apply the filter after any extensions? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:147: std::set<net::URLRequest*> mod_by_managed_mode_; What is this "managed mode" you speak of? ;-) The setting is general.
+willchan Will, I've done a quick pass over the ProfileIOData modifications, and I don't think there are any problems, but I'd like a quick sanity check from you.
ProfileIOData changes look kosher to me.
+mmenke since you're going to want his c/b/n/ approval later.
then the profile changes lgtm2 (I echo requests that references in code should be named "force_safe_search_"; I was very confused and was wondering if I could set a preference to disable safe search.)
https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:301: // If the request has been modified by the managed mode then do not route "...by policy" rather than "by the managed mode", for generality https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:303: if (mod_by_managed_mode_.find(request) != mod_by_managed_mode_.end()) { On 2012/10/16 16:37:33, Bernhard Bauer wrote: > On 2012/10/16 16:26:46, Joao da Silva wrote: > > I'm not sure about this, because the extension will see other events > > (OnHeadersReceived, OnBeforeSendHeaders, etc) but not these. This might > confuse > > some extensions that expect to see them all. I leave it to Dominic to decide > if > > this is acceptable. > > +1. Can't we just apply the filter after any extensions? Agreed, it's odd to remove these requests from what an extension sees. We don't know what the extension might be doing; it could be something else entirely, not affecting the URL. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:57: BooleanPrefMember* enable_safesearch, As above, "force" (or "require", etc.) rather than "enable". The others should be changed too, but that might want to match policy-name changes, which is a much bigger deal. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:147: std::set<net::URLRequest*> mod_by_managed_mode_; On 2012/10/16 16:37:33, Bernhard Bauer wrote: > What is this "managed mode" you speak of? ;-) The setting is general. So "modified_by_policy_", with a comment to match. https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:129: GURL(url_string), &tdelegate_, &context_); Does this not fit up on the line above? https://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:153: TestDelegate tdelegate_; On 2012/10/16 16:05:49, battre wrote: > s/tdelegate_/delegate_/ Or test_delegate_ https://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/poli... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/poli... chrome/browser/policy/policy_browsertest.cc:652: // set the safe=active&ssui=on parameters at the end of the query. nit: missing some words here ("with the SafeSearch set the [blah] parameters") https://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/poli... chrome/browser/policy/policy_browsertest.cc:684: // Verify that searching from the google.com works On 2012/10/16 16:26:46, Joao da Silva wrote: > nit: terminate sentence Also no "the" before "google.com" https://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constants.h File chrome/common/url_constants.h (right): https://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constant... chrome/common/url_constants.h:408: // Parameters for SafeSearch when used as the single parameters. "Single" is a bit confusing here, since it contains more than one. "...as the only parameters" perhaps, or "with no other parameters".
Changed the way the check is done, updated variable/constant names to better variants and various other improvements. Someone run this again on the trybots please, also any idea why the Windows one failed the last time, doesn't seem related? http://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_te... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_te... chrome/app/policy/policy_templates.json:560: 'name': 'SafeSearchEnabled', On 2012/10/16 16:37:33, Bernhard Bauer wrote: > I would be happier if we called this "ForceSafeSearch" or something. "Enabled" > sounds like the possibility of using SafeSearch (and conversely, setting the > policy to false would imply that SafeSearch can't be used, when in reality it > just means that it's not enforced). Done. http://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_te... chrome/app/policy/policy_templates.json:571: 'desc': '''Enables <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>'s SafeSearch feature and prevents users from changing this setting. On 2012/10/16 16:26:46, Joao da Silva wrote: > SafeSearch is not a Google Chrome feature, it's a Google feature. Remove the > <ph> element and just write Google in its place. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_te... chrome/app/policy/policy_templates.json:577: If you enable or disable this setting, users cannot change or override this setting in <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>. On 2012/10/16 16:37:33, Bernhard Bauer wrote: > This setting is not accessible to users at all. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/app/policy/policy_te... chrome/app/policy/policy_templates.json:579: If this policy is left not set users will be able to choose their own SafeSearch setting''', On 2012/10/16 16:26:46, Joao da Silva wrote: > End the sentence with a dot. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:7: #include <string.h> // For strlen On 2012/10/16 16:26:46, Joao da Silva wrote: > nit: newline Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:234: if (enable_safesearch_ && enable_safesearch_->GetValue()) { On 2012/10/16 16:05:49, battre wrote: > how about moving lines 235-264 into a function? Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:234: if (enable_safesearch_ && enable_safesearch_->GetValue()) { On 2012/10/16 16:37:33, Bernhard Bauer wrote: > On 2012/10/16 16:05:49, battre wrote: > > how about moving lines 235-264 into a function? > > Yesplz. That would make unit-testing it a lot easier. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:238: request->url().spec()); On 2012/10/16 16:37:33, Bernhard Bauer wrote: > Can we add a sanity-DCHECK that |is_search_url| and |is_homepage_url| are > mutually exclusive? Well, they're not completely, that's the problem. For example: http://google.com -> !is_search_url && is_homepage_url http://google.com?q=test -> is_search_url && is_homepage_url http://google.com/search?q=test -> is_search_url && !is_homepage_url http://cnn.com -> !is_search_url && !is_homepage_url http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:239: if (mod_by_managed_mode_.find(request) == mod_by_managed_mode_.end() && On 2012/10/16 16:37:33, Bernhard Bauer wrote: > Can OnBeforeURLRequest() be called multiple times for the same request? No longer relevant since we changed the way we do it http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:244: if (is_homepage_url && query.length() == 0) { On 2012/10/16 16:37:33, Bernhard Bauer wrote: > `query.length() == 0` can be written a bit shorter as `query.empty()`. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:249: // Make sure that the last our parameters are either the only ones On 2012/10/16 16:05:49, battre wrote: > nit: grammar in sentence above "the last our" Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:251: if ((query.rfind(chrome::kSSearchParamsSingle) != std::string::npos && On 2012/10/16 16:05:49, battre wrote: > KSSearch --> this is again an abbreviation that is not obvious. Please expand > the abbrev. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:252: query.length() == strlen(chrome::kSSearchParamsSingle)) || On 2012/10/16 16:05:49, battre wrote: > if query contains kSSearchParamsSingle and has the same length it is equal > right? Why don't you test that? Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:254: return net::OK; On 2012/10/16 16:37:33, Bernhard Bauer wrote: > Nit: newline Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:254: return net::OK; On 2012/10/16 16:26:46, Joao da Silva wrote: > |request| is not added to mod_by_managed_mode_ in this case? Changed the way this works, see new patch. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:301: // If the request has been modified by the managed mode then do not route On 2012/10/17 11:37:37, Pam wrote: > "...by policy" rather than "by the managed mode", for generality No longer applicable (done). http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:303: if (mod_by_managed_mode_.find(request) != mod_by_managed_mode_.end()) { On 2012/10/17 11:37:37, Pam wrote: > On 2012/10/16 16:37:33, Bernhard Bauer wrote: > > On 2012/10/16 16:26:46, Joao da Silva wrote: > > > I'm not sure about this, because the extension will see other events > > > (OnHeadersReceived, OnBeforeSendHeaders, etc) but not these. This might > > confuse > > > some extensions that expect to see them all. I leave it to Dominic to decide > > if > > > this is acceptable. > > > > +1. Can't we just apply the filter after any extensions? > > Agreed, it's odd to remove these requests from what an extension sees. We don't > know what the extension might be doing; it could be something else entirely, not > affecting the URL. Please review the new way in which we are doing this. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:304: mod_by_managed_mode_.erase(request); On 2012/10/16 16:05:49, battre wrote: > I would consider adding a return statement here and deleting the else below. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:57: BooleanPrefMember* enable_safesearch, On 2012/10/17 11:37:37, Pam wrote: > As above, "force" (or "require", etc.) rather than "enable". The others should > be changed too, but that might want to match policy-name changes, which is a > much bigger deal. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:133: BooleanPrefMember* enable_safesearch_; On 2012/10/16 16:05:49, battre wrote: > how about renaming this to enforce_google_safesearch_? Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:147: std::set<net::URLRequest*> mod_by_managed_mode_; On 2012/10/16 16:05:49, battre wrote: > I think we should rename this to "modified_by_managed_mode_" even it if its > long. We don't allow abbreviations. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:147: std::set<net::URLRequest*> mod_by_managed_mode_; On 2012/10/16 16:26:46, Joao da Silva wrote: > Use a more descriptive name: > > modified_by_managed_mode_; Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:147: std::set<net::URLRequest*> mod_by_managed_mode_; On 2012/10/16 16:37:33, Bernhard Bauer wrote: > What is this "managed mode" you speak of? ;-) The setting is general. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:124: const bool exact) { On 2012/10/16 16:05:49, battre wrote: > style nit: all parameters should be aligned at the same column if they don't fit > into one row. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:128: TestURLRequest url_req( On 2012/10/16 16:05:49, battre wrote: > s/url_req/request/ Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:142: loop_ = NULL; On 2012/10/16 16:05:49, battre wrote: > how about a scoped_ptr? do you need to use a pointer here? Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:153: TestDelegate tdelegate_; On 2012/10/16 16:05:49, battre wrote: > s/tdelegate_/delegate_/ Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:154: MessageLoopForIO* loop_; On 2012/10/16 16:26:46, Joao da Silva wrote: > Use a scoped_ptr Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:217: // Test that another website is not affected, without parameters On 2012/10/16 16:26:46, Joao da Silva wrote: > nit: terminate sentence Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:220: // Test that another website is not affected, with parameters On 2012/10/16 16:26:46, Joao da Silva wrote: > nit: terminate sentence Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:223: // Test that another website is not affected with redirects, with parameters On 2012/10/16 16:26:46, Joao da Silva wrote: > nit: terminate sentence Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:233: // Test the home page On 2012/10/16 16:26:46, Joao da Silva wrote: > nit: terminate sentences in this test too Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/polic... File chrome/browser/policy/policy_browsertest.cc (right): http://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:664: GURL expected_wo("http://google.com/"); On 2012/10/16 16:26:46, Joao da Silva wrote: > Use a descriptive variable name, such as |expected_without|. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:684: // Verify that searching from the google.com works On 2012/10/16 16:26:46, Joao da Silva wrote: > nit: terminate sentence Done. http://codereview.chromium.org/11186002/diff/6001/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:694: GURL expected_w(expected_url); On 2012/10/16 16:26:46, Joao da Silva wrote: > Use a descriptive name: |expected_with_params| Done. http://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constants.cc File chrome/common/url_constants.cc (right): http://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constants... chrome/common/url_constants.cc:499: const char kSSearchParamsMultiple[] = "&safe=active&ssui=on"; On 2012/10/16 16:26:46, Joao da Silva wrote: > This is redundant; the first constant is enough and the & can be inserted > manually where appropriate. Done. http://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constants.h File chrome/common/url_constants.h (right): http://codereview.chromium.org/11186002/diff/6001/chrome/common/url_constants... chrome/common/url_constants.h:408: // Parameters for SafeSearch when used as the single parameters. On 2012/10/17 11:37:37, Pam wrote: > "Single" is a bit confusing here, since it contains more than one. "...as the > only parameters" perhaps, or "with no other parameters". Done.
http://codereview.chromium.org/11186002/diff/12002/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/11186002/diff/12002/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:571: 'desc': '''Forces users to do queries in Google Web Search with SafeSearch set to active and prevents users from changing this setting. We don't really force users to do something, we force queries to have the SafeSearch setting active ;-) http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:219: std::string multiple_parameters = "&"+single_parameters; Nit: space before and after operators like "+" http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:230: return; Nit: newline please. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:99: forwarder_.get(), NULL, NULL, NULL, NULL, NULL, &enable_referrers_, Do we need |enable_referrers_|? http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:120: profile_.GetTestingPrefService(), NULL); Nit: align with parameter on previous line. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:141: EXPECT_STREQ(expected.c_str(), request.url().query().c_str()); You should be able just to do EXPECT_EQ. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:143: EXPECT_TRUE(request.url().query().rfind(expected)); rfind() returns the position, so we check here that the query does not start with |expected|, no? In general, I'm not really sure why we have this inexact matching here. It would seem easier for someone looking at the test if they could see exactly what query string is expected. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:150: virtual void TearDown() OVERRIDE { This is unnecessary. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:165: TEST_F(ChromeNetworkDelegateSafesearchTest, SafesearchOn) { Some more test cases: ?q=google&safe=active&ssui=on (we shouldn't modify the request in that case) ?q=google&ssui=on&safe=active (I think we shouldn't modify that request either, as all the parameters are already set) ?q=google&safe=active&ssui=onemilliondollars ?q=google&unsafe=active&ssui=on http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:172: CheckAddedParameters("http://google.com/", kSingleParameters, I think we could directly test the ForceGoogleSafeSearch() method now, no? http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:180: // Test different vaild search pages with parameters. Nit: "valid"
Updated the code with an individual check of the parameters, plus removing parameters set to the incorrect value. Also added some more tests. This hopefully is the last major change so please review this. Thanks, Sergiu http://codereview.chromium.org/11186002/diff/12002/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/11186002/diff/12002/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:571: 'desc': '''Forces users to do queries in Google Web Search with SafeSearch set to active and prevents users from changing this setting. On 2012/10/17 15:23:54, Bernhard Bauer wrote: > We don't really force users to do something, we force queries to have the > SafeSearch setting active ;-) Done. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:219: std::string multiple_parameters = "&"+single_parameters; On 2012/10/17 15:23:54, Bernhard Bauer wrote: > Nit: space before and after operators like "+" Done. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:230: return; On 2012/10/17 15:23:54, Bernhard Bauer wrote: > Nit: newline please. Done. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:99: forwarder_.get(), NULL, NULL, NULL, NULL, NULL, &enable_referrers_, On 2012/10/17 15:23:54, Bernhard Bauer wrote: > Do we need |enable_referrers_|? The other test has it as well, because on line 272 chrome_network_delegate does: if (!enable_referrers_->GetValue()) request->set_referrer(std::string()); so it can't be NULL. I'm not sure why it doesn't check for enable_referrers first. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:120: profile_.GetTestingPrefService(), NULL); On 2012/10/17 15:23:54, Bernhard Bauer wrote: > Nit: align with parameter on previous line. Done. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:141: EXPECT_STREQ(expected.c_str(), request.url().query().c_str()); On 2012/10/17 15:23:54, Bernhard Bauer wrote: > You should be able just to do EXPECT_EQ. True, didn't modify it since one of them was a string and the other was not, now fixed. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:143: EXPECT_TRUE(request.url().query().rfind(expected)); On 2012/10/17 15:23:54, Bernhard Bauer wrote: > rfind() returns the position, so we check here that the query does not start > with > |expected|, no? > > In general, I'm not really sure why we have this inexact matching here. It would > seem easier for someone looking at the test if they could see exactly what query > string is expected. Makes sense, I've refactored the code such that we do a direct check for the query part, it should be clearer that way. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:150: virtual void TearDown() OVERRIDE { On 2012/10/17 15:23:54, Bernhard Bauer wrote: > This is unnecessary. Done. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:165: TEST_F(ChromeNetworkDelegateSafesearchTest, SafesearchOn) { On 2012/10/17 15:23:54, Bernhard Bauer wrote: > Some more test cases: > > ?q=google&safe=active&ssui=on (we shouldn't modify the request in that case) > ?q=google&ssui=on&safe=active (I think we shouldn't modify that request either, > as all the parameters are already set) > ?q=google&safe=active&ssui=onemilliondollars > ?q=google&unsafe=active&ssui=on Done with the new way. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:172: CheckAddedParameters("http://google.com/", kSingleParameters, On 2012/10/17 15:23:54, Bernhard Bauer wrote: > I think we could directly test the ForceGoogleSafeSearch() method now, no? I think it's better to test it this way, at least like this you know you don't introduce any redirect loops, etc. http://codereview.chromium.org/11186002/diff/12002/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:180: // Test different vaild search pages with parameters. On 2012/10/17 15:23:54, Bernhard Bauer wrote: > Nit: "valid" Done.
Just a quick, non-exhaustive review. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:94: void ForceGoogleSafeSearchCallbackWrapper( Please add a comment describing the purpose of this function and why it works in case new_url is not empty. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:110: // Checks a parameter to see if it is present in the query. Please describe the parameters. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:130: void ExamineQuery(std::string query, std::string* new_query) { This function name is not very helpful. Please add a comment what the function does. We can then decide whether we need a more descriptive function name. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:130: void ExamineQuery(std::string query, std::string* new_query) { Strings should be passed as const references. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:131: std::vector<std::string> parameters; Please declare variables where they are used first (does not work for all variables obviously). http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:139: Tokenize(query, std::string("&"), ¶meters); I am not sure whether this approach is sufficient. It probably does not work with %xx encoded characters. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:159: new_parameters.insert(new_parameters.end(), safe_parameter); nit: -2 spaces, also below http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:330: base::Bind(&ForceGoogleSafeSearchCallbackWrapper, nit: +2 spaces http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:79: // parameters. This description is not accurate. The operation is not a "check".
http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:99: if (rv == net::OK && new_url->is_empty()) If the extension rewrites the URL but it still lands on google search then SafeSearch will not be enforced, right? Is that the intended action? http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:345: } There are now two paths exiting this function from here, and future code will have to handle both. How about this: bool force_safesearch = force_google_safesearch_ && force_google_safesearch_->GetValue(); if (force_safesearch) { // Wrap |callback|. callback = base::Bind(&ForceGoogleSafeSearchCallbackWrapper, callback, base::Unretained(request), base::Unretained(new_url)); } int rv = ExtensionWebRequestEventRouter:: ... callback, ...; if (force_safesearch && rv == net::OK && new_url->is_empty()) ForceGoogleSafeSearch(request, new_url); return rv; http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:8: #include <set> Not used http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:24: const std::string kBothParameters = kSafeParameter + "&" + kSsuiParameter; This requires a static initializer to call the string ctors. Move the string constants to the SafeSearchOn test instead. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/policy/polic... File chrome/browser/policy/policy_browsertest.cc (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:650: IN_PROC_BROWSER_TEST_F(PolicyTest, SafeSearch) { ForceSafeSeach http://codereview.chromium.org/11186002/diff/5004/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:376: mutable BooleanPrefMember enable_safesearch_; force_safesearch?
Fixed the previous comments, extra feedback is welcome :) http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:94: void ForceGoogleSafeSearchCallbackWrapper( On 2012/10/19 09:02:35, battre wrote: > Please add a comment describing the purpose of this function and why it works in > case new_url is not empty. Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:99: if (rv == net::OK && new_url->is_empty()) On 2012/10/19 10:00:38, Joao da Silva wrote: > If the extension rewrites the URL but it still lands on google search then > SafeSearch will not be enforced, right? Is that the intended action? As far as I have understood if the extension rewrites the URL a redirect is triggered and the OnBeforeURLRequest is triggered again after that, and we should enforce it then. @battre, can you confirm this? http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:110: // Checks a parameter to see if it is present in the query. On 2012/10/19 09:02:35, battre wrote: > Please describe the parameters. Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:130: void ExamineQuery(std::string query, std::string* new_query) { On 2012/10/19 09:02:35, battre wrote: > This function name is not very helpful. Please add a comment what the function > does. We can then decide whether we need a more descriptive function name. Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:130: void ExamineQuery(std::string query, std::string* new_query) { On 2012/10/19 09:02:35, battre wrote: > Strings should be passed as const references. Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:131: std::vector<std::string> parameters; On 2012/10/19 09:02:35, battre wrote: > Please declare variables where they are used first (does not work for all > variables obviously). Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:139: Tokenize(query, std::string("&"), ¶meters); On 2012/10/19 09:02:35, battre wrote: > I am not sure whether this approach is sufficient. It probably does not work > with %xx encoded characters. Added a percent-encoded unit test and it seems to work, don't know if I am missing something though, shouldn't things be percent-encoded when we get them in the request? http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:159: new_parameters.insert(new_parameters.end(), safe_parameter); On 2012/10/19 09:02:35, battre wrote: > nit: -2 spaces, also below Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:330: base::Bind(&ForceGoogleSafeSearchCallbackWrapper, On 2012/10/19 09:02:35, battre wrote: > nit: +2 spaces Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:345: } On 2012/10/19 10:00:38, Joao da Silva wrote: > There are now two paths exiting this function from here, and future code will > have to handle both. How about this: > > bool force_safesearch = force_google_safesearch_ && > force_google_safesearch_->GetValue(); > if (force_safesearch) { > // Wrap |callback|. > callback = base::Bind(&ForceGoogleSafeSearchCallbackWrapper, > callback, > base::Unretained(request), > base::Unretained(new_url)); > } > > int rv = ExtensionWebRequestEventRouter:: ... callback, ...; > > if (force_safesearch && rv == net::OK && new_url->is_empty()) > ForceGoogleSafeSearch(request, new_url); > > return rv; Rewritten, but using wrapped_callback since callback is const. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:8: #include <set> On 2012/10/19 10:00:38, Joao da Silva wrote: > Not used Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.h:79: // parameters. On 2012/10/19 09:02:35, battre wrote: > This description is not accurate. The operation is not a "check". Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate_unittest.cc:24: const std::string kBothParameters = kSafeParameter + "&" + kSsuiParameter; On 2012/10/19 10:00:38, Joao da Silva wrote: > This requires a static initializer to call the string ctors. Move the string > constants to the SafeSearchOn test instead. Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/policy/polic... File chrome/browser/policy/policy_browsertest.cc (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/policy/polic... chrome/browser/policy/policy_browsertest.cc:650: IN_PROC_BROWSER_TEST_F(PolicyTest, SafeSearch) { On 2012/10/19 10:00:38, Joao da Silva wrote: > ForceSafeSeach Done. http://codereview.chromium.org/11186002/diff/5004/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.h (right): http://codereview.chromium.org/11186002/diff/5004/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.h:376: mutable BooleanPrefMember enable_safesearch_; On 2012/10/19 10:00:38, Joao da Silva wrote: > force_safesearch? Done.
http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:127: if (parameter == parameter_to_find) You could do this check right at the beginning, no? http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:130: if (parameter.substr(0, parameter_prefix.length()) == parameter_prefix) { You could write this as `base::StartsWith(parameter, parameter_prefix, false)`. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:144: std::string* new_query) { Do you need to return the value by outparam? http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:152: Tokenize(query, std::string("&"), ¶meters); I think the explicit constructor isn't necessary. Also, you could use base::SplitString()? It might me ever so slightly faster (as it doesn't have to search for the first of multiple separators). http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:173: new_parameters.insert(new_parameters.end(), safe_parameter); I think you can just do push_back(). http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:21: namespace { Nit: unnecessary. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:231: CheckAddedParameters("http://google.com/?" + kSsuiParameter + "&" + Nit: I think it might be more readable if you just write out the parameters.
Mostly good for me! Please have a look at the suggestions to simplify the code in the delegate, that's the only issue I have with the current patch. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:159: &keep_current_parameter); The ChecForParameter() function and the enum values are a bit more complicated than they have to be. Why not just 1) always remove the matching parameters and 2) always append the desired values at the end? So this loop's body becomes (in forward traversal order): if (!ParameterMatches(*it, safe_parameter) && !ParameterMatches(*it, ssui_parameter)) new_parameters.push_back(*it); and after the loop: new_parameters.push_back(safe_parameter); new_parameters.push_back(ssui_parameter); This handles duplicate parameters, and will only reorder the parameter order if they are already present. Is that a problem? http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:168: new_parameters.insert(new_parameters.begin(), *rit); Why not iterate forward (not reverse) and push_back here? http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:287: void ChromeNetworkDelegate::ForceGoogleSafeSearch(net::URLRequest* request, It's not clear to me why this is a static method of the delegate, while the other functions are in the anonymous namespace. Can't this be there too? http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:14: #include "net/url_request/url_request.h" nit: net::URLRequest can be forward declared in this header, and url_request.h only has to be included in the .cc file. http://codereview.chromium.org/11186002/diff/15001/chrome/common/url_constants.h File chrome/common/url_constants.h (right): http://codereview.chromium.org/11186002/diff/15001/chrome/common/url_constant... chrome/common/url_constants.h:408: // Parameters that get appended to force SafeSearch nit: terminate sentence with a dot
This set has more simplifications/improvements based on your comments, please review. Sorry for the big delay between commits, had Noogler training classes. Thanks, Sergiu http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:127: if (parameter == parameter_to_find) On 2012/10/22 09:50:18, Bernhard Bauer wrote: > You could do this check right at the beginning, no? Done. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:130: if (parameter.substr(0, parameter_prefix.length()) == parameter_prefix) { On 2012/10/22 09:50:18, Bernhard Bauer wrote: > You could write this as `base::StartsWith(parameter, parameter_prefix, false)`. Done. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:144: std::string* new_query) { On 2012/10/22 09:50:18, Bernhard Bauer wrote: > Do you need to return the value by outparam? Returned a string object now. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:152: Tokenize(query, std::string("&"), ¶meters); On 2012/10/22 09:50:18, Bernhard Bauer wrote: > I think the explicit constructor isn't necessary. > > Also, you could use base::SplitString()? It might me ever so slightly faster (as > it doesn't have to search for the first of multiple separators). Done. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:159: &keep_current_parameter); On 2012/10/22 21:14:20, Joao da Silva wrote: > The ChecForParameter() function and the enum values are a bit more complicated > than they have to be. Why not just 1) always remove the matching parameters and > 2) always append the desired values at the end? So this loop's body becomes (in > forward traversal order): > > if (!ParameterMatches(*it, safe_parameter) && !ParameterMatches(*it, > ssui_parameter)) > new_parameters.push_back(*it); > > and after the loop: > > new_parameters.push_back(safe_parameter); > new_parameters.push_back(ssui_parameter); > > This handles duplicate parameters, and will only reorder the parameter order if > they are already present. Is that a problem? No, it makes total sense.. the initial idea was to alter the query as less as possible but since I'm already doing that it's a better idea to keep everything simple. Fixed. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:168: new_parameters.insert(new_parameters.begin(), *rit); On 2012/10/22 21:14:20, Joao da Silva wrote: > Why not iterate forward (not reverse) and push_back here? Done. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:173: new_parameters.insert(new_parameters.end(), safe_parameter); On 2012/10/22 09:50:18, Bernhard Bauer wrote: > I think you can just do push_back(). Done. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:287: void ChromeNetworkDelegate::ForceGoogleSafeSearch(net::URLRequest* request, On 2012/10/22 21:14:20, Joao da Silva wrote: > It's not clear to me why this is a static method of the delegate, while the > other functions are in the anonymous namespace. Can't this be there too? Initially I touched some stuff in the delegate but that has changed and not I've moved this in the anonymous space as well. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.h (right): http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.h:14: #include "net/url_request/url_request.h" On 2012/10/22 21:14:20, Joao da Silva wrote: > nit: net::URLRequest can be forward declared in this header, and url_request.h > only has to be included in the .cc file. It as it is already forward declared in network_delegate.h but I should also declare it here, correct? http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:21: namespace { On 2012/10/22 09:50:18, Bernhard Bauer wrote: > Nit: unnecessary. Whoops. Done. http://codereview.chromium.org/11186002/diff/15001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:231: CheckAddedParameters("http://google.com/?" + kSsuiParameter + "&" + On 2012/10/22 09:50:18, Bernhard Bauer wrote: > Nit: I think it might be more readable if you just write out the parameters. It's a bit simpler now but still uses variables... should I just replace the variables with the text version everywhere? http://codereview.chromium.org/11186002/diff/15001/chrome/common/url_constants.h File chrome/common/url_constants.h (right): http://codereview.chromium.org/11186002/diff/15001/chrome/common/url_constant... chrome/common/url_constants.h:408: // Parameters that get appended to force SafeSearch On 2012/10/22 21:14:20, Joao da Silva wrote: > nit: terminate sentence with a dot Done.
lgtm! http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:107: return false; return StartsWithASCII(parameter, parameter_prefix, false); http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:135: // it enforces that the SafeSearch query parameters are set to active nit: terminate sentence
LGTM!
http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:100: const std::string& parameter_to_find) { I think this function does way more than necessary. Why don't you replace the second parameter with parameter_name? Then you don't need to deal with "=" in |parameter_to_find|. Also you can change the function signature to " // Returns whether a URL parameter, |parameter| (e.g. foo=bar), has a specific |name| (e.g. "foo"). bool IsMatchingParameterName(const std::string& parameter, const std::string& name) {" (I don't like the "Check" so much because it does not tell me what the function does.) http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:105: if (StartsWithASCII(parameter, parameter_prefix, false)) This does not work if the key in |parameter| is %xx encoded. Did you check whether GURL resolves such encoding for you? Can you add a unit test for that or check whether the RFC prevents this? http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:113: std::string ExamineQueryForSafeSearch(const std::string& query) { How about "void AddSafeSearchParameters(const GURL& request, GURL* new_url)"? ("Examine" is a bad word for a function in my opinion) http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:228: BooleanPrefMember* force_google_safesearch, if your functions use the spelling SafeSearch, I think this should be safe_search. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:98: void SetSafesearch(bool value) { In the chrome_network_delegate.cc you use the spelling SafeSearch http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:99: force_google_safesearch_.SetValue(value); if you use SafeSearch, this should be force_google_safe_search_ http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:124: // ? and #) of the url_string. This comment is incorrect. The function does not check that the part between ? and # of *url_string* is equal to expected. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:126: const std::string& expected) { nit: rename |expected| to |expected_query_parameter|? http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:142: BooleanPrefMember enable_referrers_; do you use the referrers anywhere? http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:157: SetDelegate(delegate.get()); how about this? net::NetworkDelegate delegate; SetDelegate(&delegate); http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:184: "q=google&" + kBothParameters); do the same thing with "safe" replaced with "%73afe" http://codereview.chromium.org/11186002/diff/24001/chrome/common/url_constant... File chrome/common/url_constants.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/common/url_constant... chrome/common/url_constants.cc:504: // Safesearch query parameters. nit: s/Safesearch/Google SafeSearch/
Dominic, please check the replies to some of your comments, there are some areas where I'm not sure which is the best solution. Thanks. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:100: const std::string& parameter_to_find) { On 2012/10/30 12:56:10, battre wrote: > I think this function does way more than necessary. Why don't you replace the > second parameter with parameter_name? Then you don't need to deal with "=" in > |parameter_to_find|. Also you can change the function signature to " > // Returns whether a URL parameter, |parameter| (e.g. foo=bar), has a specific > |name| (e.g. "foo"). > bool IsMatchingParameterName(const std::string& parameter, const std::string& > name) {" > (I don't like the "Check" so much because it does not tell me what the function > does.) What I was trying to accomplish was use as few constants as possible. I have the constant kConst = "foo=bar"; but in that function I'm trying to identify all parameters where "foo=somethingelse", which I did by searching for the = in "foo=bar" and then checking "foo=" that against "foo=somethingelse". Otherwise I could put two constants (for "foo" and "bar") or just hardcode the value foo in the call (but you would need to modify it in multiple places if it ever changes). Don't know which is the best option here http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:105: if (StartsWithASCII(parameter, parameter_prefix, false)) On 2012/10/30 12:56:10, battre wrote: > This does not work if the key in |parameter| is %xx encoded. Did you check > whether GURL resolves such encoding for you? Can you add a unit test for that or > check whether the RFC prevents this? RFC 3986 [1] states: URIs that differ in the replacement of an unreserved character with its corresponding percent-encoded US-ASCII octet are equivalent: they identify the same resource. However, URI comparison implementations do not always perform normalization prior to comparison (see Section 6). For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore (%5F), or tilde (%7E) should not be created by URI producers and, when found in a URI, should be decoded to their corresponding unreserved characters by URI normalizers. Therefore, the answer is that this function would not work with safe= written in percent-encoded form. However the end result would be that we would add safe=active in non-encoded form and keep the percent-encoded one but the one we added at the end would still be in effect. Since this is a corner case and I don't expect query keys in Google Web Search to be percent-encoded any time soon I don't think this is a major problem, but I can create a general solution if you think that it is worth it. [1] http://tools.ietf.org/html/rfc3986#page-14 http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:107: return false; On 2012/10/30 09:09:49, Joao da Silva wrote: > return StartsWithASCII(parameter, parameter_prefix, false); Done. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:113: std::string ExamineQueryForSafeSearch(const std::string& query) { On 2012/10/30 12:56:10, battre wrote: > How about "void AddSafeSearchParameters(const GURL& request, GURL* new_url)"? > ("Examine" is a bad word for a function in my opinion) Renamed, although the signature is the same as before (I don't think I need to interact with those objects in that function). Please advise if you think it should be changed. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:135: // it enforces that the SafeSearch query parameters are set to active On 2012/10/30 09:09:49, Joao da Silva wrote: > nit: terminate sentence Done. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:228: BooleanPrefMember* force_google_safesearch, On 2012/10/30 12:56:10, battre wrote: > if your functions use the spelling SafeSearch, I think this should be > safe_search. Yeah, SafeSearch was a bit tricky because it is officially written as SafeSearch, which is a single word, but I changed it to *_safe_search. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:98: void SetSafesearch(bool value) { On 2012/10/30 12:56:10, battre wrote: > In the chrome_network_delegate.cc you use the spelling SafeSearch Done. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:99: force_google_safesearch_.SetValue(value); On 2012/10/30 12:56:10, battre wrote: > if you use SafeSearch, this should be force_google_safe_search_ Done. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:124: // ? and #) of the url_string. On 2012/10/30 12:56:10, battre wrote: > This comment is incorrect. The function does not check that the part between ? > and # of *url_string* is equal to expected. Done. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:126: const std::string& expected) { On 2012/10/30 12:56:10, battre wrote: > nit: rename |expected| to |expected_query_parameter|? Done. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:142: BooleanPrefMember enable_referrers_; On 2012/10/30 12:56:10, battre wrote: > do you use the referrers anywhere? They are used in the delegate constructor which does not accept it as NULL (see [1]). I'm not sure what the logic behind that was exactly though, it was set in the other unit test as well. [1] http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_... http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:157: SetDelegate(delegate.get()); On 2012/10/30 12:56:10, battre wrote: > how about this? > net::NetworkDelegate delegate; > SetDelegate(&delegate); Could you explain a bit more here? Do you mean use a normal object instead of a scoped_ptr? I still have to call the ChromeNetworkDelegate constructor somehow don't I? http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:184: "q=google&" + kBothParameters); On 2012/10/30 12:56:10, battre wrote: > do the same thing with "safe" replaced with "%73afe" See comment in the chrome_network_delegate.cc file about this matter. Added a test with the expected result of adding the two parameters at the end. http://codereview.chromium.org/11186002/diff/24001/chrome/common/url_constant... File chrome/common/url_constants.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/common/url_constant... chrome/common/url_constants.cc:504: // Safesearch query parameters. On 2012/10/30 12:56:10, battre wrote: > nit: s/Safesearch/Google SafeSearch/ Done.
LGTM http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:100: const std::string& parameter_to_find) { On 2012/10/30 21:58:17, Sergiu wrote: > On 2012/10/30 12:56:10, battre wrote: > > I think this function does way more than necessary. Why don't you replace the > > second parameter with parameter_name? Then you don't need to deal with "=" in > > |parameter_to_find|. Also you can change the function signature to " > > // Returns whether a URL parameter, |parameter| (e.g. foo=bar), has a specific > > |name| (e.g. "foo"). > > bool IsMatchingParameterName(const std::string& parameter, const std::string& > > name) {" > > (I don't like the "Check" so much because it does not tell me what the > function > > does.) > > What I was trying to accomplish was use as few constants as possible. I have the > constant kConst = "foo=bar"; but in that function I'm trying to identify all > parameters where "foo=somethingelse", which I did by searching for the = in > "foo=bar" and then checking "foo=" that against "foo=somethingelse". Otherwise I > could put two constants (for "foo" and "bar") or just hardcode the value foo in > the call (but you would need to modify it in multiple places if it ever > changes). Don't know which is the best option here I like your new implementation. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:105: if (StartsWithASCII(parameter, parameter_prefix, false)) On 2012/10/30 21:58:17, Sergiu wrote: > On 2012/10/30 12:56:10, battre wrote: > > This does not work if the key in |parameter| is %xx encoded. Did you check > > whether GURL resolves such encoding for you? Can you add a unit test for that > or > > check whether the RFC prevents this? > > RFC 3986 [1] states: > URIs that differ in the replacement of an unreserved character with > its corresponding percent-encoded US-ASCII octet are equivalent: they > identify the same resource. However, URI comparison implementations > do not always perform normalization prior to comparison (see Section > 6). For consistency, percent-encoded octets in the ranges of ALPHA > (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), > underscore (%5F), or tilde (%7E) should not be created by URI > producers and, when found in a URI, should be decoded to their > corresponding unreserved characters by URI normalizers. > > Therefore, the answer is that this function would not work with safe= written in > percent-encoded form. However the end result would be that we would add > safe=active in non-encoded form and keep the percent-encoded one but the one we > added at the end would still be in effect. Since this is a corner case and I > don't expect query keys in Google Web Search to be percent-encoded any time soon > I don't think this is a major problem, but I can create a general solution if > you think that it is worth it. > > [1] http://tools.ietf.org/html/rfc3986#page-14 If the last stated parameter wins in case of two conflicting parameters in SafeSearch, I am fine with the current implementation and would not implement a general solution because it would be quite costly. If not, I don't consider this an unimportant corner case because people who want to work around security features are creative. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:113: std::string ExamineQueryForSafeSearch(const std::string& query) { On 2012/10/30 21:58:17, Sergiu wrote: > On 2012/10/30 12:56:10, battre wrote: > > How about "void AddSafeSearchParameters(const GURL& request, GURL* new_url)"? > > ("Examine" is a bad word for a function in my opinion) > > Renamed, although the signature is the same as before (I don't think I need to > interact with those objects in that function). Please advise if you think it > should be changed. SGTM http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:142: BooleanPrefMember enable_referrers_; On 2012/10/30 21:58:17, Sergiu wrote: > On 2012/10/30 12:56:10, battre wrote: > > do you use the referrers anywhere? > > They are used in the delegate constructor which does not accept it as NULL (see > [1]). I'm not sure what the logic behind that was exactly though, it was set in > the other unit test as well. > > [1] > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_... I think we should make enable_referrers in the ChromeNetworkDelegate optional as well. http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:157: SetDelegate(delegate.get()); On 2012/10/30 21:58:17, Sergiu wrote: > On 2012/10/30 12:56:10, battre wrote: > > how about this? > > net::NetworkDelegate delegate; > > SetDelegate(&delegate); > > Could you explain a bit more here? Do you mean use a normal object instead of a > scoped_ptr? I still have to call the ChromeNetworkDelegate constructor somehow > don't I? Oh, I am sorry, I did not notice this. This solution is good. http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:107: // Examines the query and adds the necessary parameters so that SafeSearch is nit: Examines the query parameter of a URL request and ... http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:139: request->url().spec()); how about moving lines 136 to 139 into google_util::SupportsSafeSearch?
http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:99: // We set the second parameter so it must have "=" in it. I don't follow this comment. Is it making assumptions about the caller? If so, might those be invalid for some hypothetical future caller? If so if so, the assumptions should be mentioned in the function comment. http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:113: std::string ssui_parameter = chrome::kSafeSearchSsuiParameter; Any particular reason you allocate these again, as opposed to just using the constants? http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:132: // it enforces that the SafeSearch query parameters are set to active. nit: drop "it" in "it enforces", and add a comma at the end of the previous line http://codereview.chromium.org/11186002/diff/22020/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/11186002/diff/22020/chrome/common/pref_names.c... chrome/common/pref_names.cc:842: // Boolean controlling whether SafeSearch is forced on users. Perhaps "required" or "mandatory" would be more diplomatic than "forced on users"?
http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:113: std::string ssui_parameter = chrome::kSafeSearchSsuiParameter; On 2012/10/31 13:50:35, Pam wrote: > Any particular reason you allocate these again, as opposed to just using the > constants? The strings are const char[] contants. You would instantiate them each time you call HasSameParameterKey(). StartsWithASCII is only defined on const std::string&
Adressed the last comments and also synced the tree to the latest version which led to some merges. My tests pass but can someone trigger the trybots for me to make sure that something else wasn't broken? Thanks, Sergiu http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): http://codereview.chromium.org/11186002/diff/24001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate_unittest.cc:142: BooleanPrefMember enable_referrers_; On 2012/10/31 13:16:55, battre wrote: > On 2012/10/30 21:58:17, Sergiu wrote: > > On 2012/10/30 12:56:10, battre wrote: > > > do you use the referrers anywhere? > > > > They are used in the delegate constructor which does not accept it as NULL > (see > > [1]). I'm not sure what the logic behind that was exactly though, it was set > in > > the other unit test as well. > > > > [1] > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/net/chrome_... > > I think we should make enable_referrers in the ChromeNetworkDelegate optional as > well. Since this is a long commit already with a lot of changes I'll make that as a separate commit. http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:99: // We set the second parameter so it must have "=" in it. On 2012/10/31 13:50:35, Pam wrote: > I don't follow this comment. Is it making assumptions about the caller? If so, > might those be invalid for some hypothetical future caller? If so if so, the > assumptions should be mentioned in the function comment. Rephrased it such that it is hopefully clearer now. http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:107: // Examines the query and adds the necessary parameters so that SafeSearch is On 2012/10/31 13:16:55, battre wrote: > nit: Examines the query parameter of a URL request and ... Done, rephrased. http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:132: // it enforces that the SafeSearch query parameters are set to active. On 2012/10/31 13:50:35, Pam wrote: > nit: drop "it" in "it enforces", and add a comma at the end of the previous line Rephrased. http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:139: request->url().spec()); On 2012/10/31 13:16:55, battre wrote: > how about moving lines 136 to 139 into google_util::SupportsSafeSearch? Done. http://codereview.chromium.org/11186002/diff/22020/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/11186002/diff/22020/chrome/common/pref_names.c... chrome/common/pref_names.cc:842: // Boolean controlling whether SafeSearch is forced on users. On 2012/10/31 13:50:35, Pam wrote: > Perhaps "required" or "mandatory" would be more diplomatic than "forced on > users"? Renamed, but the variable name is still force_* :)
As a general tip, if the CL is big and you sync during a code review, it may help to upload separate snapshots for syncing and making review changes. That way, reviewers can inspect the diff between patch sets to only see your changes, without the upstream changes. http://codereview.chromium.org/11186002/diff/45001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/45001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:138: if (google_util::SupportsSafeSearch(request->url().query())) { Early-return if the URL doesn't support SafeSearch? http://codereview.chromium.org/11186002/diff/45001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:345: void ChromeNetworkDelegate::ForceGoogleSafeSearch(net::URLRequest* request, Wait, we already have this method above ;-)
http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/22020/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:113: std::string ssui_parameter = chrome::kSafeSearchSsuiParameter; On 2012/10/31 14:06:08, battre wrote: > The strings are const char[] contants. Right. Ignore me. http://codereview.chromium.org/11186002/diff/45001/chrome/browser/google/goog... File chrome/browser/google/google_util.h (right): http://codereview.chromium.org/11186002/diff/45001/chrome/browser/google/goog... chrome/browser/google/google_util.h:99: // Search URL. This comment describes the implementation. More descriptive would be something like Returns true if the given url supports Google Safe Search parameters. Granted this is a bit redundant with the method name, but not entirely.
I thought that rebasing might break stuff up in codereview but I'm still not sure what the best way to do it is (you said something about snapshots). I'll ask more on Monday about this. http://codereview.chromium.org/11186002/diff/45001/chrome/browser/google/goog... File chrome/browser/google/google_util.h (right): http://codereview.chromium.org/11186002/diff/45001/chrome/browser/google/goog... chrome/browser/google/google_util.h:99: // Search URL. On 2012/11/02 14:00:35, Pam wrote: > This comment describes the implementation. More descriptive would be something > like > > Returns true if the given url supports Google Safe Search parameters. > > Granted this is a bit redundant with the method name, but not entirely. Done. http://codereview.chromium.org/11186002/diff/45001/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/45001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:138: if (google_util::SupportsSafeSearch(request->url().query())) { On 2012/11/02 13:54:34, Bernhard Bauer wrote: > Early-return if the URL doesn't support SafeSearch? Do you mean if (!SupportsSafeSearch(..)) return; //do work here instead of what's currently there? I've changed it to that version now. http://codereview.chromium.org/11186002/diff/45001/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:345: void ChromeNetworkDelegate::ForceGoogleSafeSearch(net::URLRequest* request, On 2012/11/02 13:54:34, Bernhard Bauer wrote: > Wait, we already have this method above ;-) /facepalm, rebasing fail
On 2012/11/02 20:38:43, Sergiu wrote: > I thought that rebasing might break stuff up in codereview but I'm still not > sure what the best way to do it is (you said something about snapshots). I'll > ask more on Monday about this. Not sure what you mean by breaking stuff up in codereview. With "uploading a snapshot" I meant uploading the current state of the CL as a new patch set. That means that you do the following: * You get a review on the CL (on patch set 1) * You sync to the newest version * You upload the synced version: `git cl upload -t sync` (patch set 2) * You make the review changes * You upload the CL again: `git cl upload -t review` (patch set 3) The reviewer can now look at the diff between patch sets 2 and 3 and see *only* the changes that you made, not the upstream changes that you merged in. http://codereview.chromium.org/11186002/diff/39004/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/39004/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:138: if (!google_util::SupportsSafeSearch(request->url().spec())) SupportsSafeSearch() takes a GURL as parameter, so you end up parsing the URL again (and writing the .spec() out is a bit ugly).
Drive-by: Please add a bug value to the BUG= line.
http://codereview.chromium.org/11186002/diff/39004/chrome/browser/google/goog... File chrome/browser/google/google_util.cc (right): http://codereview.chromium.org/11186002/diff/39004/chrome/browser/google/goog... chrome/browser/google/google_util.cc:326: return IsGoogleSearchUrl(url) || IsGoogleHomePageUrl(url); Don't add this. Have the lone caller check these two functions directly.
Got it, add a separate patch set for the merge, will do that from now on. http://codereview.chromium.org/11186002/diff/39004/chrome/browser/google/goog... File chrome/browser/google/google_util.cc (right): http://codereview.chromium.org/11186002/diff/39004/chrome/browser/google/goog... chrome/browser/google/google_util.cc:326: return IsGoogleSearchUrl(url) || IsGoogleHomePageUrl(url); On 2012/11/02 20:59:11, Peter Kasting wrote: > Don't add this. Have the lone caller check these two functions directly. Removed and added it back as direct call. http://codereview.chromium.org/11186002/diff/39004/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/39004/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:138: if (!google_util::SupportsSafeSearch(request->url().spec())) On 2012/11/02 20:56:46, Bernhard Bauer wrote: > SupportsSafeSearch() takes a GURL as parameter, so you end up parsing the URL > again (and writing the .spec() out is a bit ugly). Reverted to the original version as suggested by pkasting@.
LGTM!
lgtm
LGTM.
Still lgtm http://codereview.chromium.org/11186002/diff/46015/chrome/browser/net/chrome_... File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/11186002/diff/46015/chrome/browser/net/chrome_... chrome/browser/net/chrome_network_delegate.cc:7: #include <string> Already in .h
On 2012/11/05 15:43:16, Joao da Silva wrote: > Still lgtm > > http://codereview.chromium.org/11186002/diff/46015/chrome/browser/net/chrome_... > File chrome/browser/net/chrome_network_delegate.cc (right): > > http://codereview.chromium.org/11186002/diff/46015/chrome/browser/net/chrome_... > chrome/browser/net/chrome_network_delegate.cc:7: #include <string> > Already in .h Done, also after some discussions with Bernhard and Joao's help I changed the browsertest to reduce possible flakyness by failing requests. I think this is done, I'll submit tomorrow. Note to self: commit/upload after gsync if there's new stuff in there.
Thanks for fixing the flakiness. lgtm after a green bot run
Hopefully that's the end of the story for that test :-) Nice catch! lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11186002/49006
Presubmit check for 11186002-49006 failed and returned exit status 1.
Running presubmit commit checks ...
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 2.5s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/11186002/42012
Change committed as 166656
thakis@chromium.org:TBR for chrome/OWNERS.
Why is this done at the network level? I would have expected this code in chrome/browser (template_url.cc etc) and their clients.
We want this to apply for all search requests, not only for the ones made from the omnibox. Hooking into the network stack is the easiest way to make sure that we append the parameters to every request. On Thu, Nov 8, 2012 at 8:06 PM, <thakis@chromium.org> wrote: > Why is this done at the network level? I would have expected this code in > chrome/browser (template_url.cc etc) and their clients. > > https://chromiumcodereview.**appspot.com/11186002/<https://chromiumcodereview... >
On Thu, Nov 8, 2012 at 12:03 PM, Bernhard Bauer <bauerb@chromium.org> wrote: > We want this to apply for all search requests, not only for the ones made > from the omnibox. What other search requests do we do? > Hooking into the network stack is the easiest way to make > sure that we append the parameters to every request. > > > On Thu, Nov 8, 2012 at 8:06 PM, <thakis@chromium.org> wrote: >> >> Why is this done at the network level? I would have expected this code in >> chrome/browser (template_url.cc etc) and their clients. >> >> https://chromiumcodereview.appspot.com/11186002/ > >
Bernhard is referring to the requests that are done by typing "google.com" in the omnibox and searching. By doing this at the network level we know that we catch those as well and that you can't circumvent SafeSearch on Google Search. On Fri, Nov 9, 2012 at 6:56 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, Nov 8, 2012 at 12:03 PM, Bernhard Bauer <bauerb@chromium.org> > wrote: > > We want this to apply for all search requests, not only for the ones made > > from the omnibox. > > What other search requests do we do? > > > Hooking into the network stack is the easiest way to make > > sure that we append the parameters to every request. > > > > > > On Thu, Nov 8, 2012 at 8:06 PM, <thakis@chromium.org> wrote: > >> > >> Why is this done at the network level? I would have expected this code > in > >> chrome/browser (template_url.cc etc) and their clients. > >> > >> https://chromiumcodereview.appspot.com/11186002/ > > > > >
http://codereview.chromium.org/11186002/diff/42012/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): http://codereview.chromium.org/11186002/diff/42012/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:571: If you disable this setting, SafeSearch in Google Search is not enforced.''', Post-commit comment: This should probably be "If this setting is disabled or not set, [...]". |
