Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9720001:
Add a setting to CF to remove 'chromeframe' from the UserAgent on a per-pattern basis. (Closed)
|
Created:
8 years, 9 months ago by robertshield Modified:
8 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a setting to CF to remove 'chromeframe' from the UserAgent on a per-pattern basis.
Useful for testing and dealing with sites with broken UA parsing.
BUG=117157
TEST=chrome_frame_tests,chrome_frame_unittests,add a ExcludeUAFromDomain key to the CF settings, add some pattern values, observe that the UA string does not contain CF.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129985
Patch Set 1 #Patch Set 2 : Add missing files. #Patch Set 3 : Minor tweak to UA building. #
Total comments: 63
Patch Set 4 : Review feedback from Greg and Alex. #
Total comments: 38
Patch Set 5 : grt's feedback, test updates #
Total comments: 4
Patch Set 6 : In the end. #
Messages
Total messages: 14 (0 generated)
On 2012/03/19 14:28:29, robertshield wrote: The issue here is Chrome Frame UA tag 'chromeframe' raises false positives due to scripts looking form 'chrome'. If chrome frame UA was 'gcf/x.x.x.x' this would not be a problem at all. We should have done it much earlier but maybe we can add both for now and phase out the old one?
On 2012/03/19 16:43:50, amit wrote: > On 2012/03/19 14:28:29, robertshield wrote: > > The issue here is Chrome Frame UA tag 'chromeframe' raises false positives due > to scripts looking form 'chrome'. If chrome frame UA was 'gcf/x.x.x.x' this > would not be a problem at all. We should have done it much earlier but maybe we > can add both for now and phase out the old one? That is a much better long-term solution :-) This said, it will be difficult to get people to update their UA sniffing again making the phasing-out part time-consuming (I'm thinking it will take a couple of blog posts and a six months grace period). Having something like this as a workaround to unblock deployments in the mean time seems like a good idea.
http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc File chrome_frame/html_utils.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:379: DCHECK_EQ(false, StartsWithASCII(value, "User-Agent:", true)); What's the rationale for this check? Is it that we should only be dealing with the value, not the header name? If so, can we get a comment here quickly outlining the thinking/ http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:392: std::string ret(value, 0, cf_start - offset); can we get a check to make sure that cf_start - offset >= 0? http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... File chrome_frame/registry_list_preferences_holder.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:4: // clobber empty comment line http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc File chrome_frame/utils.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:763: kExcludeUAFromDomainList); My biggest concern here is that we'll provide some values to this as a form of compatibility list and then users will augment the list and on the next upgrade we'll be owned. Can we have our own (ostensibly) private key where we look for default values and then check this location for user-provided additions to the list?
http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc File chrome_frame/html_utils.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:381: std::string::size_type cf_start = value.find(kChromeFrameUserAgent); Chromium style says to use size_t instead of size_type http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:389: if (cf_start > 1 && value[cf_start - 1] == ' ') ++offset; nit: i find this single-line stuff foreign enough that it reduces readability. please wrap if you don't mind. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:395: (value[cf_start] == '/' || nit: order these clauses from most-likely-to-match to least-likely-to-match http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.cc File chrome_frame/http_negotiate.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:87: std::string ExtractUserAgentFromHeaders(LPCWSTR headers, the name of this method doesn't make its behavior obvious. does "extract" mean "remove from" or just "get"? would you add a comment? also, it appears to only consider |headers| if |additional_headers| doesn't contain a User-Agent field. seems that that should be mentioned in a comment, too. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:114: std::string FilterHeaders(const std::string& old_headers, i find "filter" a bit vague. how about ExcludeFieldFromHeaders(const std::string& headers, const char* field) also, is it required that |field| be lower-case? please document if so. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:123: new_headers += name + ": " + headers_iterator.values() + "\r\n"; these three uses of operator+ involve lots of temporaries. please use += or append as you see fit http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:147: std::string ascii_headers = WideToASCII(additional_headers); nit: use () rather than = http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:151: new_headers += "User-Agent: " + user_agent_value; i'd advocate for .append or += three times here, too http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:167: new_headers += "User-Agent: " + user_agent_value; use += instead of + http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:173: // |additional_headers| and and...? http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:195: new_headers += "User-Agent: " + user_agent_value; use += rather than + http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:261: HRESULT HttpNegotiatePatch::BeginningTransaction( is it possible to make a test for the changes of behavior in this function? http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:289: lstrcpyW(*additional_headers, ASCIIToWide(updated_headers).c_str()); the various UserAgentString functions appear to convert wide to ascii, and this converts the ascii back to wide. is there a reason to not do all manipulations on wide strings to avoid the pair of conversions? http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... File chrome_frame/registry_list_preferences_holder.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:4: // remove http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:11: using base::win::RegKey; remove this http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:20: RegKey config_key; i think you don't need config_key here: base::win::RegistryValueIterator string_list(hive, registry_path); http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:34: remove extra newline http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:38: if (MatchPattern(string, *iter)) { remove braces http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... File chrome_frame/registry_list_preferences_holder.h (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.h:10: #ifndef CHROME_FRAME_HTML_PREFERENCES_HOLDER_H_ CHROME_FRAME_REGISTRY_LIST_PREFERENCES_HOLDER_H_ http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.h:28: bool Valid() { return valid_; } Valid() const http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.h:30: bool ListMatches(const string16& string); ListMatches() const http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc File chrome_frame/utils.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcode49 chrome_frame/utils.cc:49: // Note that these values are all lower case and are compared to from the "no good deed goes unpunished" department: would you update this comment? not all values below are lower case, so it's not clear to me what "these values" corresponds to. since some of these constants are the names of registry values, perhaps make those ones kRegShadyGroove to group them together. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:722: static RegistryListPreferencesHolder render_type_for_url_holder; this introduces an exit-time call to a dtor. if leaking this object is okay (we're pinned anway), please use: CR_DEFINE_STATIC_LOCAL(RegistryListPreferencesHolder, render_type_for_url_holder, ()); also note that this isn't threadsafe. prefer LazyInstance<RegistryListPreferencesHolder>::Leaky if this could be called from multiple threads. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:725: if (!render_type_for_url_holder.Valid()) { i think this is equivalent, but smaller: if (!render_type_for_url_holder.Valid()) { const wchar_t* url_list_name = kRenderInGCFUrlList; render_in_cf_by_default = GetConfigInt(FALSE, kEnableGCFRendererByDefault); if (render_in_cf_by_default) { url_list_name = kRenderInHostUrlList; renderer_type = RENDERER_TYPE_CHROME_DEFAULT_RENDERER; } render_type_for_url_holder.Init(HKEY_CURRENT_USER, kChromeFrameConfigKey, url_list_name); } http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:759: static RegistryListPreferencesHolder user_agent_filter_holder; see note above about static dtors and threadsafety http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:766: if (user_agent_filter_holder.ListMatches(url)) { nit: remove braces http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.h File chrome_frame/utils.h (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.h#newcode284 chrome_frame/utils.h:284: UserAgentAction ShouldRemoveUAForUrl(const string16& url); since there are only two options and the function is called "ShouldBlahBlah," i find it more intuitive for the return value to be a boolean. if (ShouldBlahBlah()) is more readable.
http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc File chrome_frame/html_utils.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:379: DCHECK_EQ(false, StartsWithASCII(value, "User-Agent:", true)); On 2012/03/20 15:31:56, slightlyoff1 wrote: > What's the rationale for this check? Is it that we should only be dealing with > the value, not the header name? If so, can we get a comment here quickly > outlining the thinking/ I was following the tradition established in AddChromeFrameToUserAgentValue of ensuring that this function is only called on an actual User-Agent header string. This isn't documented in the header comments though and seems generally unnecessary, so I'm removing it. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:381: std::string::size_type cf_start = value.find(kChromeFrameUserAgent); On 2012/03/20 17:27:45, grt wrote: > Chromium style says to use size_t instead of size_type Fixed here and above. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:389: if (cf_start > 1 && value[cf_start - 1] == ' ') ++offset; On 2012/03/20 17:27:45, grt wrote: > nit: i find this single-line stuff foreign enough that it reduces readability. > please wrap if you don't mind. Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:392: std::string ret(value, 0, cf_start - offset); On 2012/03/20 15:31:56, slightlyoff1 wrote: > can we get a check to make sure that cf_start - offset >= 0? Sure, added. Note that since cf_start is a positive integer, it can be proven that the three lines above guarantee that cf_cstart - offset >= 0. Check added for clarity anyway. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/html_utils.cc#n... chrome_frame/html_utils.cc:395: (value[cf_start] == '/' || On 2012/03/20 17:27:45, grt wrote: > nit: order these clauses from most-likely-to-match to least-likely-to-match Indeed! http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.cc File chrome_frame/http_negotiate.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:87: std::string ExtractUserAgentFromHeaders(LPCWSTR headers, On 2012/03/20 17:27:45, grt wrote: > the name of this method doesn't make its behavior obvious. does "extract" mean > "remove from" or just "get"? would you add a comment? also, it appears to only > consider |headers| if |additional_headers| doesn't contain a User-Agent field. > seems that that should be mentioned in a comment, too. Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:114: std::string FilterHeaders(const std::string& old_headers, On 2012/03/20 17:27:45, grt wrote: > i find "filter" a bit vague. how about ExcludeFieldFromHeaders(const > std::string& headers, const char* field) > also, is it required that |field| be lower-case? please document if so. Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:123: new_headers += name + ": " + headers_iterator.values() + "\r\n"; On 2012/03/20 17:27:45, grt wrote: > these three uses of operator+ involve lots of temporaries. please use += or > append as you see fit Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:147: std::string ascii_headers = WideToASCII(additional_headers); On 2012/03/20 17:27:45, grt wrote: > nit: use () rather than = Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:151: new_headers += "User-Agent: " + user_agent_value; On 2012/03/20 17:27:45, grt wrote: > i'd advocate for .append or += three times here, too Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:167: new_headers += "User-Agent: " + user_agent_value; On 2012/03/20 17:27:45, grt wrote: > use += instead of + Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:173: // |additional_headers| and On 2012/03/20 17:27:45, grt wrote: > and...? ... and this is where I fell asleep :-) http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:195: new_headers += "User-Agent: " + user_agent_value; On 2012/03/20 17:27:45, grt wrote: > use += rather than + Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:261: HRESULT HttpNegotiatePatch::BeginningTransaction( On 2012/03/20 17:27:45, grt wrote: > is it possible to make a test for the changes of behavior in this function? It is! Test added. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/http_negotiate.... chrome_frame/http_negotiate.cc:289: lstrcpyW(*additional_headers, ASCIIToWide(updated_headers).c_str()); On 2012/03/20 17:27:45, grt wrote: > the various UserAgentString functions appear to convert wide to ascii, and this > converts the ascii back to wide. is there a reason to not do all manipulations > on wide strings to avoid the pair of conversions? The functions in this file use UA code from \net (specifically the HeadersIterator) which is ascii only. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... File chrome_frame/registry_list_preferences_holder.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:4: // On 2012/03/20 15:31:56, slightlyoff1 wrote: > clobber empty comment line Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:4: // On 2012/03/20 17:27:45, grt wrote: > remove Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:11: using base::win::RegKey; On 2012/03/20 17:27:45, grt wrote: > remove this Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:20: RegKey config_key; On 2012/03/20 17:27:45, grt wrote: > i think you don't need config_key here: > base::win::RegistryValueIterator string_list(hive, registry_path); Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:34: On 2012/03/20 17:27:45, grt wrote: > remove extra newline Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.cc:38: if (MatchPattern(string, *iter)) { On 2012/03/20 17:27:45, grt wrote: > remove braces Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... File chrome_frame/registry_list_preferences_holder.h (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.h:10: #ifndef CHROME_FRAME_HTML_PREFERENCES_HOLDER_H_ On 2012/03/20 17:27:45, grt wrote: > CHROME_FRAME_REGISTRY_LIST_PREFERENCES_HOLDER_H_ Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.h:28: bool Valid() { return valid_; } On 2012/03/20 17:27:45, grt wrote: > Valid() const Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/registry_list_p... chrome_frame/registry_list_preferences_holder.h:30: bool ListMatches(const string16& string); On 2012/03/20 17:27:45, grt wrote: > ListMatches() const Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc File chrome_frame/utils.cc (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcode49 chrome_frame/utils.cc:49: // Note that these values are all lower case and are compared to On 2012/03/20 17:27:45, grt wrote: > from the "no good deed goes unpunished" department: would you update this > comment? not all values below are lower case, so it's not clear to me what > "these values" corresponds to. since some of these constants are the names of > registry values, perhaps make those ones kRegShadyGroove to group them together. Reordered and updated the comment. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:722: static RegistryListPreferencesHolder render_type_for_url_holder; On 2012/03/20 17:27:45, grt wrote: > this introduces an exit-time call to a dtor. if leaking this object is okay > (we're pinned anway), please use: > CR_DEFINE_STATIC_LOCAL(RegistryListPreferencesHolder, > render_type_for_url_holder, ()); > also note that this isn't threadsafe. prefer > LazyInstance<RegistryListPreferencesHolder>::Leaky if this could be called from > multiple threads. Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:725: if (!render_type_for_url_holder.Valid()) { On 2012/03/20 17:27:45, grt wrote: > i think this is equivalent, but smaller: > if (!render_type_for_url_holder.Valid()) { > const wchar_t* url_list_name = kRenderInGCFUrlList; > render_in_cf_by_default = GetConfigInt(FALSE, kEnableGCFRendererByDefault); > if (render_in_cf_by_default) { > url_list_name = kRenderInHostUrlList; > renderer_type = RENDERER_TYPE_CHROME_DEFAULT_RENDERER; > } > render_type_for_url_holder.Init(HKEY_CURRENT_USER, kChromeFrameConfigKey, > url_list_name); > } Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:759: static RegistryListPreferencesHolder user_agent_filter_holder; On 2012/03/20 17:27:45, grt wrote: > see note above about static dtors and threadsafety Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:763: kExcludeUAFromDomainList); On 2012/03/20 15:31:56, slightlyoff1 wrote: > My biggest concern here is that we'll provide some values to this as a form of > compatibility list and then users will augment the list and on the next upgrade > we'll be owned. Can we have our own (ostensibly) private key where we look for > default values and then check this location for user-provided additions to the > list? That sounds reasonable, but I'd like to tackle that in a different CL if that's okay with you. This CL doesn't provide for a default list and several other changes will also be needed to make that happen; I'd rather make the change you suggest then. I've left a TODO in place. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.cc#newcod... chrome_frame/utils.cc:766: if (user_agent_filter_holder.ListMatches(url)) { On 2012/03/20 17:27:45, grt wrote: > nit: remove braces Done. http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.h File chrome_frame/utils.h (right): http://codereview.chromium.org/9720001/diff/5001/chrome_frame/utils.h#newcode284 chrome_frame/utils.h:284: UserAgentAction ShouldRemoveUAForUrl(const string16& url); On 2012/03/20 17:27:45, grt wrote: > since there are only two options and the function is called "ShouldBlahBlah," i > find it more intuitive for the return value to be a boolean. if > (ShouldBlahBlah()) is more readable. Done.
lookin' good. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... File chrome_frame/html_utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:370: // add comment text or remove // https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:372: if (value.empty()) { i'm not sure what value the log message adds. if it's useful, how about replacing 372 through 375 with: DLOG_IF(WARNING, value.empty()) << "Empty user agent value."; since lines 377 through 380 will yield the same results? https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:387: if (cf_start > 2 && value[cf_start - 2] == ';') to match the insertion code, i think this really needs to be: if (cf_start > 3 && value[cf_start - 2] == ';' && isalnum(value[cf_start - 3])) https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:399: ret += value.substr(cf_start); avoid a temporary and extra memcpy with: ret.append(value, cf_start, std::string::npos); https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... File chrome_frame/http_negotiate.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:123: HttpUtil::HeadersIterator headers_iterator(old_headers.begin(), assuming that the field to be excluded doesn't account for the majority of old_headers' size, we can save on reallocs and memcpys by calling new_headers.reserve(old_headers.size()) somewhere up here. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:125: while (headers_iterator.GetNext()) { this loop is hit often, no? if so, i think it's worth avoiding the extra strings and memcpys involved with name() and values(): if (!LowerCaseEqualsASCII(headers_iterator.name_begin(), headers_iterator.name_end(), field)) { ... new_headers.append(headers_iterator.values_begin(), headers_iterator.values_end()); ... } https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:138: std::string AppendCFUserAgentString(LPCWSTR headers, this differs from RemoveCFUserAgentString by only one line of code. i think it's worth combining them into one function with an extra argument indicating whether CF should be added or removed. if it's simpler, this one method could be a *Impl method, with Append and Remove being one-liners that call the impl with true/false as appropriate. then again, they're called in either side of a boolean if() else clause, so maybe it's convenient for it to be just one function there, too. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:153: std::string new_headers; replace everything here on down with: return ReplaceOrAddUserAgent(additional_headers, user_agent_value); https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:200: std::string new_headers; replace everything here on down with: return ReplaceOrAddUserAgent(additional_headers, user_agent_value); https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/regist... File chrome_frame/registry_list_preferences_holder.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/regist... chrome_frame/registry_list_preferences_holder.cc:17: registry_path); can this be un-wrapped? https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/regist... chrome_frame/registry_list_preferences_holder.cc:18: while (string_list.Valid()) { ? for (; string_list.Valid(); ++string_list) values_push_back(string_list.Name()); ? https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/test/h... File chrome_frame/test/http_negotiate_unittest.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/test/h... chrome_frame/test/http_negotiate_unittest.cc:145: nit: remove blank line https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/test/h... chrome_frame/test/http_negotiate_unittest.cc:160: { ua_url, un-indent two spaces https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.cc File chrome_frame/utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:733: if (!g_render_type_for_url_holder.Get().Valid()) { cache the result of .Get() for use throughout the function https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:735: if (IsGcfDefaultRenderer()) { two comments here: 1. you lost the assignment to render_in_cf_by_default, which is needed down below. 2. i think the use of IsGcfDefaultRenderer is not desired, as it mixes in policy settings. i think policy checking is handled at the top of this function, so the stuff down here shouldn't be considering policy again. do you disagree? render_in_cf_by_default = GetConfigInt(FALSE, kEnableGCFRendererByDefault); if (render_in_cf_by_default) { https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:737: renderer_type = RENDERER_TYPE_CHROME_DEFAULT_RENDERER; oops, i think this assignment needs to take place outside of the initialization clause so that it happens each time through the function. otherwise the CHROME_DEFAULT_RENDERER won't be returned when it should for non-matching URLs. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:741: kChromeFrameConfigKey, nit: indentation https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:760: if (!g_user_agent_filter_holder.Get().Valid()) { cache the result of Get() https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.h File chrome_frame/utils.h (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.h:50: nit: too many blank lines
Thanks Greg for the performance tips, comments addressed, plus I added a little bit of plumbing to make the tests happy with how the registry settings are no longer re-read on each operation. PTAL https://chromiumcodereview.appspot.com/9720001/diff/5001/chrome_frame/registr... File chrome_frame/registry_list_preferences_holder.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/5001/chrome_frame/registr... chrome_frame/registry_list_preferences_holder.cc:20: RegKey config_key; On 2012/03/26 02:43:33, robertshield wrote: > On 2012/03/20 17:27:45, grt wrote: > > i think you don't need config_key here: > > base::win::RegistryValueIterator string_list(hive, registry_path); > > Done. And slightly modified to avoid a regression caused by dropping list_name. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... File chrome_frame/html_utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:370: // On 2012/03/26 17:48:41, grt wrote: > add comment text or remove // Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:372: if (value.empty()) { On 2012/03/26 17:48:41, grt wrote: > i'm not sure what value the log message adds. if it's useful, how about > replacing 372 through 375 with: > DLOG_IF(WARNING, value.empty()) << "Empty user agent value."; > > since lines 377 through 380 will yield the same results? Some monkey-seeing here. Removing the block altogether, the log statement is unlikely to ever be viewed by humans. Removed just the log statement in AddCFToUAV above. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:387: if (cf_start > 2 && value[cf_start - 2] == ';') On 2012/03/26 17:48:41, grt wrote: > to match the insertion code, i think this really needs to be: > if (cf_start > 3 && value[cf_start - 2] == ';' && isalnum(value[cf_start - 3])) Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/html_u... chrome_frame/html_utils.cc:399: ret += value.substr(cf_start); On 2012/03/26 17:48:41, grt wrote: > avoid a temporary and extra memcpy with: > ret.append(value, cf_start, std::string::npos); Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... File chrome_frame/http_negotiate.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:123: HttpUtil::HeadersIterator headers_iterator(old_headers.begin(), On 2012/03/26 17:48:41, grt wrote: > assuming that the field to be excluded doesn't account for the majority of > old_headers' size, we can save on reallocs and memcpys by calling > new_headers.reserve(old_headers.size()) somewhere up here. Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:125: while (headers_iterator.GetNext()) { On 2012/03/26 17:48:41, grt wrote: > this loop is hit often, no? if so, i think it's worth avoiding the extra > strings and memcpys involved with name() and values(): > if (!LowerCaseEqualsASCII(headers_iterator.name_begin(), > headers_iterator.name_end(), field)) { > ... > new_headers.append(headers_iterator.values_begin(), > headers_iterator.values_end()); > ... > } Yes indeed, thanks! https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:138: std::string AppendCFUserAgentString(LPCWSTR headers, On 2012/03/26 17:48:41, grt wrote: > this differs from RemoveCFUserAgentString by only one line of code. i think > it's worth combining them into one function with an extra argument indicating > whether CF should be added or removed. if it's simpler, this one method could > be a *Impl method, with Append and Remove being one-liners that call the impl > with true/false as appropriate. then again, they're called in either side of a > boolean if() else clause, so maybe it's convenient for it to be just one > function there, too. Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:153: std::string new_headers; On 2012/03/26 17:48:41, grt wrote: > replace everything here on down with: > return ReplaceOrAddUserAgent(additional_headers, user_agent_value); Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/http_n... chrome_frame/http_negotiate.cc:200: std::string new_headers; On 2012/03/26 17:48:41, grt wrote: > replace everything here on down with: > return ReplaceOrAddUserAgent(additional_headers, user_agent_value); Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/regist... File chrome_frame/registry_list_preferences_holder.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/regist... chrome_frame/registry_list_preferences_holder.cc:17: registry_path); On 2012/03/26 17:48:41, grt wrote: > can this be un-wrapped? Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/regist... chrome_frame/registry_list_preferences_holder.cc:18: while (string_list.Valid()) { On 2012/03/26 17:48:41, grt wrote: > ? > for (; string_list.Valid(); ++string_list) > values_push_back(string_list.Name()); > ? Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/test/h... File chrome_frame/test/http_negotiate_unittest.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/test/h... chrome_frame/test/http_negotiate_unittest.cc:145: On 2012/03/26 17:48:41, grt wrote: > nit: remove blank line Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/test/h... chrome_frame/test/http_negotiate_unittest.cc:160: { ua_url, On 2012/03/26 17:48:41, grt wrote: > un-indent two spaces Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.cc File chrome_frame/utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:733: if (!g_render_type_for_url_holder.Get().Valid()) { On 2012/03/26 17:48:41, grt wrote: > cache the result of .Get() for use throughout the function Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:735: if (IsGcfDefaultRenderer()) { On 2012/03/26 17:48:41, grt wrote: > two comments here: > 1. you lost the assignment to render_in_cf_by_default, which is needed down > below. Done. > 2. i think the use of IsGcfDefaultRenderer is not desired, as it mixes in policy > settings. i think policy checking is handled at the top of this function, so > the stuff down here shouldn't be considering policy again. do you disagree? In this case, I do disagree. I believe it is desirable to mix in the policy settings in the initialization block here. I believe it was a bug that we were ignoring the policy settings in the earlier code. I also think it would be less readable to set render_in_cf_by_default in two places. What say you? > > render_in_cf_by_default = GetConfigInt(FALSE, kEnableGCFRendererByDefault); > if (render_in_cf_by_default) { https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:737: renderer_type = RENDERER_TYPE_CHROME_DEFAULT_RENDERER; On 2012/03/26 17:48:41, grt wrote: > oops, i think this assignment needs to take place outside of the initialization > clause so that it happens each time through the function. otherwise the > CHROME_DEFAULT_RENDERER won't be returned when it should for non-matching URLs. oops indeed, thanks! https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:741: kChromeFrameConfigKey, On 2012/03/26 17:48:41, grt wrote: > nit: indentation Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.cc:760: if (!g_user_agent_filter_holder.Get().Valid()) { On 2012/03/26 17:48:41, grt wrote: > cache the result of Get() Done. https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.h File chrome_frame/utils.h (right): https://chromiumcodereview.appspot.com/9720001/diff/13001/chrome_frame/utils.... chrome_frame/utils.h:50: On 2012/03/26 17:48:41, grt wrote: > nit: too many blank lines Done.
LGTM w/ two trivial formatting tips. Awesome change! https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/html_u... File chrome_frame/html_utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/html_u... chrome_frame/html_utils.cc:383: ++offset; nit: braces due to multi-line conditional https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/utils.cc File chrome_frame/utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/utils.... chrome_frame/utils.cc:773: kChromeFrameConfigKey, nit: de-indent 8 spaces
Thanks! https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/html_u... File chrome_frame/html_utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/html_u... chrome_frame/html_utils.cc:383: ++offset; On 2012/03/27 13:06:40, grt wrote: > nit: braces due to multi-line conditional Done. https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/utils.cc File chrome_frame/utils.cc (right): https://chromiumcodereview.appspot.com/9720001/diff/18002/chrome_frame/utils.... chrome_frame/utils.cc:773: kChromeFrameConfigKey, On 2012/03/27 13:06:40, grt wrote: > nit: de-indent 8 spaces Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robertshield@chromium.org/9720001/27001
Try job failure for 9720001-27001 (retry) on win_rel for step "browser_tests". It's a second try, previously, steps "ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robertshield@chromium.org/9720001/27001
Change committed as 129985 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
