Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
440423003:
Clean content_settings_pattern_parser.* from unnecessary dependencies. (Closed)
|
Created:
6 years, 4 months ago by vasilii Modified:
6 years, 3 months ago Reviewers:
Bernhard Bauer, Devlin, jochen (gone - plz use gerrit), vabr (Chromium), Joao da Silva, markusheintz_, James Hawkins, blundell CC:
chromium-apps-reviews_chromium.org, chromium-reviews, dbeam+watch-options_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, miu+watch_chromium.org, posciak+watch_chromium.org, stuartmorgan+watch_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionClean content_settings_pattern_parser.* from unnecessary dependencies on extensions/ and net/.
BUG=387079
Committed: https://crrev.com/94a1f29d656ac654ac0248e00f2d47fe2da7d446
Cr-Commit-Position: refs/heads/master@{#292906}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Removed the clients #
Total comments: 3
Patch Set 3 : compilation #Patch Set 4 : Implement global variable #Patch Set 5 : WIn compilation #Patch Set 6 : Fix unit tests on linux #Patch Set 7 : Fix CrOS tests #
Total comments: 10
Patch Set 8 : try global init #Patch Set 9 : no DCHECK #
Total comments: 16
Patch Set 10 : @vabr's comments addressed #
Total comments: 4
Patch Set 11 : @vabr and @bauerb comments addressed #
Total comments: 11
Patch Set 12 : answered @blundell comments #Patch Set 13 : Remove embedder_variables.* #
Total comments: 4
Patch Set 14 : Reorder methods in the correct order #
Total comments: 4
Patch Set 15 : @jochen comments #Patch Set 16 : rm content_settings_initializer #Messages
Total messages: 55 (2 generated)
Hi guys, please review this componentisation effort. Markus: content_settings* rdevlin.cronin@ : c/b/extensions/ stevenjb@chromium.org: chrome/browser/notifications/message_center_settings_controller.cc bauerb@chromium.org: c/b/ui/webui/ and chrome/browser/plugins/plugin_info_message_filter_unittest.cc blundell@chromium.org: components/ joaodasilva@chromium.org: chrome/browser/policy/policy_browsertest.cc sergeyu@chromium.org: chrome/browser/media/media_stream_devices_controller.cc jochen@chromium.org: chrome/common/render_messages.cc and chrome/renderer/content_settings_observer_browsertest.cc
Hi Vasilii, Currently not LGTM, because we cannot have the client living in common. If there is no way to have kExtensionScheme in the component, then I think hard-coding it's value, as you observed was done elsewhere, is the most sensible approach right now. Happy to talk offline as well. Cheers, Vaclav https://codereview.chromium.org/440423003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/content_settings_details.h (right): https://codereview.chromium.org/440423003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/content_settings_details.h:42: return primary_pattern_.ToString(&client).empty() && There should be a way to tell if the pattern is empty without converting it to a string before. That should be possible without the client. https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... chrome/common/content_settings_pattern.cc:91: // Overrides BuilderInterface nit: Overrides -> Implements ? https://codereview.chromium.org/440423003/diff/1/components/content_settings.... File components/content_settings.gypi (right): https://codereview.chromium.org/440423003/diff/1/components/content_settings.... components/content_settings.gypi:18: 'content_settings/core/common/content_settings_client.cc', I don't think we can have the client in common. Once it is properly introduced as a WebContentUserData and created in the browser code, there is no way to pass this to renderer code. https://codereview.chromium.org/440423003/diff/1/components/content_settings/... File components/content_settings/core/common/content_settings_client.cc (left): https://codereview.chromium.org/440423003/diff/1/components/content_settings/... components/content_settings/core/common/content_settings_client.cc:5: // This file exists only to mark the Play Store string resources as used. nit: Please try to tune the "git cl upload --similarity" flag to mark this as a brand new addition. Currently, git thinks it is a fork of remoting/resources/play_store_resources.cc, which makes for a funny history. https://codereview.chromium.org/440423003/diff/1/components/content_settings/... File components/content_settings/core/common/content_settings_client.h (right): https://codereview.chromium.org/440423003/diff/1/components/content_settings/... components/content_settings/core/common/content_settings_client.h:19: virtual bool IsExtensionScheme(const std::string& scheme); The way this method is used, I would rather call it DoesSchemeUsePorts, and modify it to return true for both extensions and file schemes. If the notion of the extension scheme is something we want to have in a method name in the component, then we probably might as well hard-code the value in here.
Colin, is there way to have an interface in components/ and the implementation available in renderer?
policy_browsertest.cc lgtm
Hi Vasilii, Thanks for this work! +1 to vabr's comment: the client here is going to be a purely browser-side concept here. I would suggest doing whichever of the following you like better: - hardcode the extensions scheme - pass in the extensions scheme as a string to the functions that need to compare against it That should avoid the need to reference the client in the renderer. https://codereview.chromium.org/440423003/diff/1/chrome/common/chrome_content... File chrome/common/chrome_content_settings_client.h (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/chrome_content... chrome/common/chrome_content_settings_client.h:11: namespace content_settings { nit: code in //chrome shouldn't be namespaced.
Once the layering is figured out, extensions lgtm. (Please ping if anything significant changes.)
https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... chrome/common/content_settings_pattern_parser.cc:13: const char* kDomainWildcard = "[*.]"; If you're touching this code anyway: const char kConstantName[] = "..." would be better. https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... File chrome/common/content_settings_pattern_parser.h (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... chrome/common/content_settings_pattern_parser.h:19: static void Parse(ContentSettingsClient* client, If you make these methods take the client as a const ref, you can pass it in inline instead of having to construct it beforehand.
Guys, please review again. https://codereview.chromium.org/440423003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/content_settings_details.h (right): https://codereview.chromium.org/440423003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/content_settings_details.h:42: return primary_pattern_.ToString(&client).empty() && On 2014/08/06 15:14:00, vabr (OOO until 19 Aug) wrote: > There should be a way to tell if the pattern is empty without converting it to a > string before. That should be possible without the client. Markus was against changing this. https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... chrome/common/content_settings_pattern.cc:91: // Overrides BuilderInterface On 2014/08/06 15:14:00, vabr (OOO until 19 Aug) wrote: > nit: Overrides -> Implements ? Done. https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... chrome/common/content_settings_pattern_parser.cc:13: const char* kDomainWildcard = "[*.]"; On 2014/08/11 08:43:50, Bernhard Bauer wrote: > If you're touching this code anyway: const char kConstantName[] = "..." would be > better. DomainWildcard is used in Builder too. Below we have also kHostWildcard, kPathWildcard, kPortWildcard. What do you want to rename? https://codereview.chromium.org/440423003/diff/1/components/content_settings/... File components/content_settings/core/common/content_settings_client.h (right): https://codereview.chromium.org/440423003/diff/1/components/content_settings/... components/content_settings/core/common/content_settings_client.h:19: virtual bool IsExtensionScheme(const std::string& scheme); On 2014/08/06 15:14:01, vabr (OOO until 19 Aug) wrote: > The way this method is used, I would rather call it DoesSchemeUsePorts, and > modify it to return true for both extensions and file schemes. > > If the notion of the extension scheme is something we want to have in a method > name in the component, then we probably might as well hard-code the value in > here. url::kFileScheme is used separately in the parser.
https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... chrome/common/content_settings_pattern_parser.cc:13: const char* kDomainWildcard = "[*.]"; On 2014/08/11 13:57:18, vasilii wrote: > On 2014/08/11 08:43:50, Bernhard Bauer wrote: > > If you're touching this code anyway: const char kConstantName[] = "..." would > be > > better. > > DomainWildcard is used in Builder too. Below we have also kHostWildcard, > kPathWildcard, kPortWildcard. What do you want to rename? Declare it as an array of characters, not as a pointer. https://codereview.chromium.org/440423003/diff/20001/chrome/common/content_se... File chrome/common/content_settings_pattern.h (right): https://codereview.chromium.org/440423003/diff/20001/chrome/common/content_se... chrome/common/content_settings_pattern.h:153: const char* extension_scheme, You still have the notion of an extension scheme in the interface. I don't think this is what vabr@ meant.
Hi Vasilii, Could you add some more context to the CL description listing the dependencies that you're abstracting? Thanks!
I changed the CL description. https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/1/chrome/common/content_settin... chrome/common/content_settings_pattern_parser.cc:13: const char* kDomainWildcard = "[*.]"; On 2014/08/11 14:06:43, Bernhard Bauer wrote: > On 2014/08/11 13:57:18, vasilii wrote: > > On 2014/08/11 08:43:50, Bernhard Bauer wrote: > > > If you're touching this code anyway: const char kConstantName[] = "..." > would > > be > > > better. > > > > DomainWildcard is used in Builder too. Below we have also kHostWildcard, > > kPathWildcard, kPortWildcard. What do you want to rename? > > Declare it as an array of characters, not as a pointer. Done. https://codereview.chromium.org/440423003/diff/20001/chrome/common/content_se... File chrome/common/content_settings_pattern.h (right): https://codereview.chromium.org/440423003/diff/20001/chrome/common/content_se... chrome/common/content_settings_pattern.h:153: const char* extension_scheme, On 2014/08/11 14:06:44, Bernhard Bauer wrote: > You still have the notion of an extension scheme in the interface. I don't think > this is what vabr@ meant. Vaclav's proposal was to hardcode the constant in the component. I don't think that it's the right way. I defer to Colin because Vaclav is on vacation.
https://codereview.chromium.org/440423003/diff/20001/chrome/common/content_se... File chrome/common/content_settings_pattern.h (right): https://codereview.chromium.org/440423003/diff/20001/chrome/common/content_se... chrome/common/content_settings_pattern.h:153: const char* extension_scheme, On 2014/08/11 16:37:47, vasilii wrote: > On 2014/08/11 14:06:44, Bernhard Bauer wrote: > > You still have the notion of an extension scheme in the interface. I don't > think > > this is what vabr@ meant. > > Vaclav's proposal was to hardcode the constant in the component. I don't think > that it's the right way. I defer to Colin because Vaclav is on vacation. There are two things I don't like here: One is the layering violation, the other is the fact that you have to pass in the extension scheme to every single call to one of these methods. That seems like a relatively large cost for what is basically an edge case. If we want to have the embedder inject the extension scheme, could we simply store it in a global variable and set it once? Components don't usually have to deal with multiple simultaneous embedders, so the fact that it's global shouldn't be a problem.
The layering violation will exist regardless of the solution we choose. If we don't want to pass this string to every method, maybe we can just hardcode it.
On 2014/08/12 09:29:08, vasilii wrote: > The layering violation will exist regardless of the solution we choose. If we > don't want to pass this string to every method, maybe we can just hardcode it. I would be okay with that, TBH.
Assuming that we're componentizing most/all of the code that this CL is touching in //chrome/browser/content_settings, then we'll have to deal with this problem again at that point :|. That suggests that either the hardcoding or global variable approach would be more practical.
we should probably (similar to what we do for e.g. GURL) add a static method to register special schemes that the embedder (chrome) will have to call during startup: https://code.google.com/p/chromium/codesearch#chromium/src/url/url_util.h&l=45
Bernhard and Vaclav, do you agree with the implemented solution suggested by Jochen?
https://codereview.chromium.org/440423003/diff/120001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser_unittest.cc (right): https://codereview.chromium.org/440423003/diff/120001/chrome/common/content_s... chrome/common/content_settings_pattern_parser_unittest.cc:31: InitContentSettingsComponent(); I would add this to ChromeTestSuite::Initialize() instead of every single test that uses content settings. https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:14: extension_scheme = scheme; DCHECK that this isn't called more than once (with the same scheme)? https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.h:15: // extensions::kExtensionScheme). It's a bit weird to assume there is only ever going to be one scheme that doesn't support ports. Could you make this a set? Also, please remove the mention of kExtensionScheme from the comment. Comments can be layering violations too :)
https://codereview.chromium.org/440423003/diff/120001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser_unittest.cc (right): https://codereview.chromium.org/440423003/diff/120001/chrome/common/content_s... chrome/common/content_settings_pattern_parser_unittest.cc:31: InitContentSettingsComponent(); On 2014/08/18 15:55:13, Bernhard Bauer wrote: > I would add this to ChromeTestSuite::Initialize() instead of every single test > that uses content settings. Done. https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:14: extension_scheme = scheme; On 2014/08/18 15:55:13, Bernhard Bauer wrote: > DCHECK that this isn't called more than once (with the same scheme)? Doesn't work for browser tests. It's called twice (from ChromeTestSuite::Initialize() and ChromeMainDelegate::BasicStartupComplete()). https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.h:15: // extensions::kExtensionScheme). On 2014/08/18 15:55:13, Bernhard Bauer wrote: > It's a bit weird to assume there is only ever going to be one scheme that > doesn't support ports. Could you make this a set? > > Also, please remove the mention of kExtensionScheme from the comment. Comments > can be layering violations too :) We don't need this complexity now. The implementation in url_util.cc involves a global vector, annotated memory leaks, global shutdown.
Thanks Vasilii, LGTM with comments. I'm glad you got rid of the client at this point. The code currently checks for the file: and chrome-extensions: schemes at different places, but it looks to me like these schemes are both not supporting domain and port wildcards, so whenever IsNonPortScheme(str) is called, it should return true both if str is "file:" or "chrome-extensions:". So I suggest IsNonPortScheme checking for equality to "file:" directly, and for equality to "chrome-extensions:" through extension_scheme. That would make the callsite of IsNonPortScheme more readable. Also, SetNonPortScheme would need to be called SetExtensionScheme instead -- that's what it does already, taking into account the static variable name extension_scheme. I'm happy to talk more about this. Cheers, Vaclav https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.h:15: // extensions::kExtensionScheme). On 2014/08/20 08:44:06, vasilii wrote: > On 2014/08/18 15:55:13, Bernhard Bauer wrote: > > It's a bit weird to assume there is only ever going to be one scheme that > > doesn't support ports. Could you make this a set? > > > > Also, please remove the mention of kExtensionScheme from the comment. Comments > > can be layering violations too :) > > We don't need this complexity now. The implementation in url_util.cc involves a > global vector, annotated memory leaks, global shutdown. I support the API accepting a single scheme as not using port if it is true (see my e-mail for file:, though), but please mention that this is for code simplicity reasons, and if there is need to have more such schemes in the future, there is no logical problem in adding support for that. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_component.h (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_component.h:8: void InitContentSettingsComponent(); nit: Please consider adding a comment explaining when to call this (also -- is it once per run, once per profile?), and what does it do. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern.h (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern.h:161: std::string ToString() const; Please keep the return type const. This makes the compiler screan if somebody mistakenly writes pattern.ToString() = "abc" instead of pattern.ToString() == "abc" (see also http://crbug.com/393155#c4) https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser.cc (left): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_parser.cc:190: // static The header still declares this method as static. Please put the header and the cc file back in sync on that. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_parser.cc:209: if (content_settings::IsNonPortScheme(parts.scheme)) { I believe this path should work for file: as well. Could you confirm, possibly with Markus? https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_serializer.h (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_serializer.h:19: // Serializes the pattern to an IPC message or deserializes it. nit: Separate the comments and place each of them in front of the respective method. https://codereview.chromium.org/440423003/diff/160001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/160001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:19: DCHECK(extension_scheme); optional nit: This DCHECK enforces that SetNonPortScheme is called before, but the comment for SetNonPortScheme only says "should be called". I suggest phrasing that comment as "Needs to be called with a non-NULL pointer" or similarly. https://codereview.chromium.org/440423003/diff/160001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/160001/components/content_sett... components/content_settings/core/common/embedder_variables.h:17: void SetNonPortScheme(const char* scheme); nit: The comment speaks also about domain, but the name does not. Could you improve? (Also below.)
Markus, we are curious can we handle file theme and extension theme in the same way (see Vaclav's mail). Is it a good idea? Please review. https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.h:15: // extensions::kExtensionScheme). On 2014/08/20 09:52:22, vabr (Chromium) wrote: > On 2014/08/20 08:44:06, vasilii wrote: > > On 2014/08/18 15:55:13, Bernhard Bauer wrote: > > > It's a bit weird to assume there is only ever going to be one scheme that > > > doesn't support ports. Could you make this a set? > > > > > > Also, please remove the mention of kExtensionScheme from the comment. > Comments > > > can be layering violations too :) > > > > We don't need this complexity now. The implementation in url_util.cc involves > a > > global vector, annotated memory leaks, global shutdown. > > I support the API accepting a single scheme as not using port if it is true (see > my e-mail for file:, though), but please mention that this is for code > simplicity reasons, and if there is need to have more such schemes in the > future, there is no logical problem in adding support for that. Done. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_component.h (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_component.h:8: void InitContentSettingsComponent(); On 2014/08/20 09:52:22, vabr (Chromium) wrote: > nit: Please consider adding a comment explaining when to call this (also -- is > it once per run, once per profile?), and what does it do. Done. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern.h (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern.h:161: std::string ToString() const; On 2014/08/20 09:52:22, vabr (Chromium) wrote: > Please keep the return type const. This makes the compiler screan if somebody > mistakenly writes > pattern.ToString() = "abc" > instead of > pattern.ToString() == "abc" > > (see also http://crbug.com/393155#c4) It won't compile any way because pattern.ToString() == "abc" is boolean and pattern.ToString() = "abc" is string. We also have Wparentheses warning. Using 'const string' prevents C++11 compiler from reusing the temporary object in the expression "pattern.ToString() + "abc"". Thus, it's obsolete. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser.cc (left): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_parser.cc:190: // static On 2014/08/20 09:52:22, vabr (Chromium) wrote: > The header still declares this method as static. Please put the header and the > cc file back in sync on that. Done. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_parser.cc:209: if (content_settings::IsNonPortScheme(parts.scheme)) { On 2014/08/20 09:52:22, vabr (Chromium) wrote: > I believe this path should work for file: as well. Could you confirm, possibly > with Markus? It's handled above. The file path can be wildcard. Also for empty extension path we add '/'. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_serializer.h (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_serializer.h:19: // Serializes the pattern to an IPC message or deserializes it. On 2014/08/20 09:52:22, vabr (Chromium) wrote: > nit: Separate the comments and place each of them in front of the respective > method. Done. https://codereview.chromium.org/440423003/diff/160001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/160001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:19: DCHECK(extension_scheme); On 2014/08/20 09:52:22, vabr (Chromium) wrote: > optional nit: This DCHECK enforces that SetNonPortScheme is called before, but > the comment for SetNonPortScheme only says "should be called". I suggest > phrasing that comment as "Needs to be called with a non-NULL pointer" or > similarly. Done. https://codereview.chromium.org/440423003/diff/160001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/160001/components/content_sett... components/content_settings/core/common/embedder_variables.h:17: void SetNonPortScheme(const char* scheme); On 2014/08/20 09:52:23, vabr (Chromium) wrote: > nit: The comment speaks also about domain, but the name does not. Could you > improve? (Also below.) Done.
https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:14: extension_scheme = scheme; On 2014/08/20 08:44:06, vasilii wrote: > On 2014/08/18 15:55:13, Bernhard Bauer wrote: > > DCHECK that this isn't called more than once (with the same scheme)? > > Doesn't work for browser tests. It's called twice (from > ChromeTestSuite::Initialize() and ChromeMainDelegate::BasicStartupComplete()). OK, then can you at least DCHECK that if it's called more than once, it's always with the same scheme? https://codereview.chromium.org/440423003/diff/180001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/180001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:11: // The component supports only one theme for simplicity. s/theme/scheme/?
Thanks, Vasilii. Still LGTM. Cheers, Vaclav https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_parser.cc:209: if (content_settings::IsNonPortScheme(parts.scheme)) { On 2014/08/21 14:58:02, vasilii wrote: > On 2014/08/20 09:52:22, vabr (Chromium) wrote: > > I believe this path should work for file: as well. Could you confirm, possibly > > with Markus? > > It's handled above. The file path can be wildcard. Also for empty extension path > we add '/'. Ah, I see. Special casing the file URL here makes sense. On other places, a file: URL is treated the same as the extension scheme URLs, and in those cases, having IsNonWildcardDomainNonPortScheme match also file: would make sense. If we go that way, we should comment here on line 195, that the special-case of file: needs to come before the case of IsNonWildcardDomainNonPortScheme. However, this is only an optional comment. I don't want to diverge too much from the original goal of the CL -- removing dependencies. Even the current state is acceptable for me. https://codereview.chromium.org/440423003/diff/180001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/180001/components/content_sett... components/content_settings/core/common/embedder_variables.h:16: // |scheme| can't be NULL. The caller is responsible for its lifetime. nit: Please join the last two sentences into: "|scheme| can't be NULL, and the pointed string must remain alive until the Chrome terminates." if that's what you wanted to say. Currently, it's not clear what "responsible for its lifetime" means.
On 2014/08/21 15:31:41, vabr (Chromium) wrote: > Thanks, Vasilii. > Still LGTM. > Cheers, > Vaclav > > https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... > File chrome/common/content_settings_pattern_parser.cc (right): > > https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... > chrome/common/content_settings_pattern_parser.cc:209: if > (content_settings::IsNonPortScheme(parts.scheme)) { > On 2014/08/21 14:58:02, vasilii wrote: > > On 2014/08/20 09:52:22, vabr (Chromium) wrote: > > > I believe this path should work for file: as well. Could you confirm, > possibly > > > with Markus? > > > > It's handled above. The file path can be wildcard. Also for empty extension > path > > we add '/'. > > Ah, I see. Special casing the file URL here makes sense. On other places, a > file: URL is treated the same as the extension scheme URLs, and in those cases, > having IsNonWildcardDomainNonPortScheme match also file: would make sense. If we > go that way, we should comment here on line 195, that the special-case of file: > needs to come before the case of IsNonWildcardDomainNonPortScheme. > > However, this is only an optional comment. I don't want to diverge too much from > the original goal of the CL -- removing dependencies. Even the current state is > acceptable for me. > > https://codereview.chromium.org/440423003/diff/180001/components/content_sett... > File components/content_settings/core/common/embedder_variables.h (right): > > https://codereview.chromium.org/440423003/diff/180001/components/content_sett... > components/content_settings/core/common/embedder_variables.h:16: // |scheme| > can't be NULL. The caller is responsible for its lifetime. > nit: Please join the last two sentences into: > "|scheme| can't be NULL, and the pointed string must remain alive until the > Chrome terminates." > if that's what you wanted to say. Currently, it's not clear what "responsible > for its lifetime" means. content settings parts LGTM
https://codereview.chromium.org/440423003/diff/120001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/120001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:14: extension_scheme = scheme; On 2014/08/21 15:05:54, Bernhard Bauer wrote: > On 2014/08/20 08:44:06, vasilii wrote: > > On 2014/08/18 15:55:13, Bernhard Bauer wrote: > > > DCHECK that this isn't called more than once (with the same scheme)? > > > > Doesn't work for browser tests. It's called twice (from > > ChromeTestSuite::Initialize() and ChromeMainDelegate::BasicStartupComplete()). > > OK, then can you at least DCHECK that if it's called more than once, it's always > with the same scheme? Done. https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... File chrome/common/content_settings_pattern_parser.cc (right): https://codereview.chromium.org/440423003/diff/160001/chrome/common/content_s... chrome/common/content_settings_pattern_parser.cc:209: if (content_settings::IsNonPortScheme(parts.scheme)) { On 2014/08/21 15:31:41, vabr (Chromium) wrote: > On 2014/08/21 14:58:02, vasilii wrote: > > On 2014/08/20 09:52:22, vabr (Chromium) wrote: > > > I believe this path should work for file: as well. Could you confirm, > possibly > > > with Markus? > > > > It's handled above. The file path can be wildcard. Also for empty extension > path > > we add '/'. > > Ah, I see. Special casing the file URL here makes sense. On other places, a > file: URL is treated the same as the extension scheme URLs, and in those cases, > having IsNonWildcardDomainNonPortScheme match also file: would make sense. If we > go that way, we should comment here on line 195, that the special-case of file: > needs to come before the case of IsNonWildcardDomainNonPortScheme. > > However, this is only an optional comment. I don't want to diverge too much from > the original goal of the CL -- removing dependencies. Even the current state is > acceptable for me. I don't know if there are other places where the difference matters. There is a risk that I'll be reverting it back again. https://codereview.chromium.org/440423003/diff/180001/components/content_sett... File components/content_settings/core/common/embedder_variables.cc (right): https://codereview.chromium.org/440423003/diff/180001/components/content_sett... components/content_settings/core/common/embedder_variables.cc:11: // The component supports only one theme for simplicity. On 2014/08/21 15:05:54, Bernhard Bauer wrote: > s/theme/scheme/? Done. https://codereview.chromium.org/440423003/diff/180001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/180001/components/content_sett... components/content_settings/core/common/embedder_variables.h:16: // |scheme| can't be NULL. The caller is responsible for its lifetime. On 2014/08/21 15:31:41, vabr (Chromium) wrote: > nit: Please join the last two sentences into: > "|scheme| can't be NULL, and the pointed string must remain alive until the > Chrome terminates." > if that's what you wanted to say. Currently, it's not clear what "responsible > for its lifetime" means. Done.
lgtm
jam@chromium.org: Please review changes in chrome/app/chrome_main_delegate.cc chrome/browser/notifications/desktop_notification_profile_util.h chrome/common/render_messages.cc chrome/test/base/chrome_test_suite.cc
On 2014/08/22 13:16:58, vasilii wrote: > mailto:jam@chromium.org: Please review changes in > > chrome/app/chrome_main_delegate.cc > chrome/browser/notifications/desktop_notification_profile_util.h > chrome/common/render_messages.cc > chrome/test/base/chrome_test_suite.cc sorry I'm very overloaded with the CY, please pick another OWNER from src/chrome/OWNERS, thanks.
James, please review chrome/app/chrome_main_delegate.cc chrome/browser/notifications/desktop_notification_profile_util.h chrome/common/render_messages.cc chrome/test/base/chrome_test_suite.cc
I'm on vacation, so the earliest I can get to this is Wednesday.
Wednesday is fine. Colin, please review too.
https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... File chrome/common/content_settings_component.h (right): https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... chrome/common/content_settings_component.h:5: #ifndef CHROME_COMMON_CONTENT_SETTINGS_COMPONENT_H_ I would call this content_settings_initializer.*. https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... chrome/common/content_settings_pattern.cc:484: // For chrome extensions URLs ignore the port. This comment should be updated. https://codereview.chromium.org/440423003/diff/200001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/200001/components/content_sett... components/content_settings/core/common/embedder_variables.h:16: // |scheme| can't be NULL, and the pointed string must remain alive until the Is there a reason not to have a global variable internally, pass the string by const ref, and copy it? That seems like it would be a friendlier API. https://codereview.chromium.org/440423003/diff/200001/components/content_sett... components/content_settings/core/common/embedder_variables.h:21: bool IsNonWildcardDomainNonPortScheme(const std::string& scheme); is this function used outside of ContentSettingsPattern? If not, I'd be inclined to just put SetNonWildcardDomainNonPortScheme into ContentSettingsPattern and have ContentSettingsPattern use the global directly internally.
https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... File chrome/common/content_settings_component.h (right): https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... chrome/common/content_settings_component.h:5: #ifndef CHROME_COMMON_CONTENT_SETTINGS_COMPONENT_H_ On 2014/08/26 09:16:57, blundell wrote: > I would call this content_settings_initializer.*. Done. https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... File chrome/common/content_settings_pattern.cc (right): https://codereview.chromium.org/440423003/diff/200001/chrome/common/content_s... chrome/common/content_settings_pattern.cc:484: // For chrome extensions URLs ignore the port. On 2014/08/26 09:16:57, blundell wrote: > This comment should be updated. Done. https://codereview.chromium.org/440423003/diff/200001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/200001/components/content_sett... components/content_settings/core/common/embedder_variables.h:16: // |scheme| can't be NULL, and the pointed string must remain alive until the On 2014/08/26 09:16:57, blundell wrote: > Is there a reason not to have a global variable internally, pass the string by > const ref, and copy it? That seems like it would be a friendlier API. The reason for that is we dislike global objects with the destructor https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/FM8C-qPm5fk We also have a warning for this 'enable_wexit_time_destructors'. For some reason it's off in componenets/. I included it for the content_settings component. https://codereview.chromium.org/440423003/diff/200001/components/content_sett... components/content_settings/core/common/embedder_variables.h:21: bool IsNonWildcardDomainNonPortScheme(const std::string& scheme); On 2014/08/26 09:16:57, blundell wrote: > is this function used outside of ContentSettingsPattern? If not, I'd be inclined > to just put SetNonWildcardDomainNonPortScheme into ContentSettingsPattern and > have ContentSettingsPattern use the global directly internally. In content_settings_pattern_parser.cc. I see that kExtensionScheme is used at least in cookie_settings.cc, host_content_settings_map.cc. They are to be componentised.
https://codereview.chromium.org/440423003/diff/200001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/200001/components/content_sett... components/content_settings/core/common/embedder_variables.h:21: bool IsNonWildcardDomainNonPortScheme(const std::string& scheme); On 2014/08/26 12:18:18, vasilii wrote: > On 2014/08/26 09:16:57, blundell wrote: > > is this function used outside of ContentSettingsPattern? If not, I'd be > inclined > > to just put SetNonWildcardDomainNonPortScheme into ContentSettingsPattern and > > have ContentSettingsPattern use the global directly internally. > > In content_settings_pattern_parser.cc. I see that kExtensionScheme is used at > least in cookie_settings.cc, host_content_settings_map.cc. They are to be > componentised. Hmm, is the usage in the latter two files the same semantically, i.e., will it make sense to replace it by a call to IsNonWildcardDomainNonPortScheme()? I have no problem with the existence of this file but I don't like the name embedder_variables.*. I was having trouble trying to think of a better name so thought maybe we could just get rid of the file altogether :).
https://codereview.chromium.org/440423003/diff/200001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/200001/components/content_sett... components/content_settings/core/common/embedder_variables.h:16: // |scheme| can't be NULL, and the pointed string must remain alive until the On 2014/08/26 12:18:19, vasilii wrote: > On 2014/08/26 09:16:57, blundell wrote: > > Is there a reason not to have a global variable internally, pass the string by > > const ref, and copy it? That seems like it would be a friendlier API. > > The reason for that is we dislike global objects with the destructor > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/FM8C-qPm5fk > We also have a warning for this 'enable_wexit_time_destructors'. For some reason > it's off in componenets/. I included it for the content_settings component. FWIW, you could still leak the string at shutdown (either via base::LazyInstance<>::Leaky or a plain std::string*).
On 2014/08/26 12:21:38, blundell wrote: > https://codereview.chromium.org/440423003/diff/200001/components/content_sett... > File components/content_settings/core/common/embedder_variables.h (right): > > https://codereview.chromium.org/440423003/diff/200001/components/content_sett... > components/content_settings/core/common/embedder_variables.h:21: bool > IsNonWildcardDomainNonPortScheme(const std::string& scheme); > On 2014/08/26 12:18:18, vasilii wrote: > > On 2014/08/26 09:16:57, blundell wrote: > > > is this function used outside of ContentSettingsPattern? If not, I'd be > > inclined > > > to just put SetNonWildcardDomainNonPortScheme into ContentSettingsPattern > and > > > have ContentSettingsPattern use the global directly internally. > > > > In content_settings_pattern_parser.cc. I see that kExtensionScheme is used at > > least in cookie_settings.cc, host_content_settings_map.cc. They are to be > > componentised. > > Hmm, is the usage in the latter two files the same semantically, i.e., will it > make sense to replace it by a call to IsNonWildcardDomainNonPortScheme()? > > I have no problem with the existence of this file but I don't like the name > embedder_variables.*. I was having trouble trying to think of a better name so > thought maybe we could just get rid of the file altogether :). I think we'll replace it with IsNonWildcardDomainNonPortScheme. Is there another choice? Do you still want to move the function in this case?
On 2014/08/26 12:43:19, vasilii wrote: > On 2014/08/26 12:21:38, blundell wrote: > > > https://codereview.chromium.org/440423003/diff/200001/components/content_sett... > > File components/content_settings/core/common/embedder_variables.h (right): > > > > > https://codereview.chromium.org/440423003/diff/200001/components/content_sett... > > components/content_settings/core/common/embedder_variables.h:21: bool > > IsNonWildcardDomainNonPortScheme(const std::string& scheme); > > On 2014/08/26 12:18:18, vasilii wrote: > > > On 2014/08/26 09:16:57, blundell wrote: > > > > is this function used outside of ContentSettingsPattern? If not, I'd be > > > inclined > > > > to just put SetNonWildcardDomainNonPortScheme into ContentSettingsPattern > > and > > > > have ContentSettingsPattern use the global directly internally. > > > > > > In content_settings_pattern_parser.cc. I see that kExtensionScheme is used > at > > > least in cookie_settings.cc, host_content_settings_map.cc. They are to be > > > componentised. > > > > Hmm, is the usage in the latter two files the same semantically, i.e., will it > > make sense to replace it by a call to IsNonWildcardDomainNonPortScheme()? > > > > I have no problem with the existence of this file but I don't like the name > > embedder_variables.*. I was having trouble trying to think of a better name so > > thought maybe we could just get rid of the file altogether :). > > I think we'll replace it with IsNonWildcardDomainNonPortScheme. Is there another > choice? For host_content_settings_map.cc, I think it will make sense to abstract out the usage of the extension scheme into the embedder via the client. For CookieSettings we could probably do the same thing. > Do you still want to move the function in this case? Yes, I think we can just move it to live in content_settings_pattern. Thanks!
https://codereview.chromium.org/440423003/diff/200001/components/content_sett... File components/content_settings/core/common/embedder_variables.h (right): https://codereview.chromium.org/440423003/diff/200001/components/content_sett... components/content_settings/core/common/embedder_variables.h:21: bool IsNonWildcardDomainNonPortScheme(const std::string& scheme); On 2014/08/26 12:21:38, blundell wrote: > On 2014/08/26 12:18:18, vasilii wrote: > > On 2014/08/26 09:16:57, blundell wrote: > > > is this function used outside of ContentSettingsPattern? If not, I'd be > > inclined > > > to just put SetNonWildcardDomainNonPortScheme into ContentSettingsPattern > and > > > have ContentSettingsPattern use the global directly internally. > > > > In content_settings_pattern_parser.cc. I see that kExtensionScheme is used at > > least in cookie_settings.cc, host_content_settings_map.cc. They are to be > > componentised. > > Hmm, is the usage in the latter two files the same semantically, i.e., will it > make sense to replace it by a call to IsNonWildcardDomainNonPortScheme()? > > I have no problem with the existence of this file but I don't like the name > embedder_variables.*. I was having trouble trying to think of a better name so > thought maybe we could just get rid of the file altogether :). Removed the files.
lgtm, thanks! https://codereview.chromium.org/440423003/diff/240001/chrome/common/content_s... File chrome/common/content_settings_pattern.h (right): https://codereview.chromium.org/440423003/diff/240001/chrome/common/content_s... chrome/common/content_settings_pattern.h:182: // Chrome terminates. s/Chrome/app https://codereview.chromium.org/440423003/diff/240001/chrome/common/content_s... chrome/common/content_settings_pattern.h:183: static void SetNonWildcardDomainNonPortScheme(const char* scheme); nit: These should be up with the other static methods.
https://codereview.chromium.org/440423003/diff/240001/chrome/common/content_s... File chrome/common/content_settings_pattern.h (right): https://codereview.chromium.org/440423003/diff/240001/chrome/common/content_s... chrome/common/content_settings_pattern.h:182: // Chrome terminates. On 2014/08/27 12:32:19, blundell wrote: > s/Chrome/app Done. https://codereview.chromium.org/440423003/diff/240001/chrome/common/content_s... chrome/common/content_settings_pattern.h:183: static void SetNonWildcardDomainNonPortScheme(const char* scheme); On 2014/08/27 12:32:19, blundell wrote: > nit: These should be up with the other static methods. Done.
James?
James, do you have time to review? chrome/app/chrome_main_delegate.cc chrome/browser/notifications/desktop_notification_profile_util.h chrome/common/render_messages.cc chrome/test/base/chrome_test_suite.cc
On 2014/09/01 08:34:17, vasilii wrote: > James, do you have time to review? (Today is a US holiday, so I wouldn't expect a reply.)
jochen@chromium.org changed reviewers: + joaodasilva@chromium.org, jochen@chromium.org
https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... File chrome/browser/notifications/desktop_notification_profile_util.h (right): https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... chrome/browser/notifications/desktop_notification_profile_util.h:8: #include "base/macros.h" why do you include base/macros.h everywhere? this file strikes me as unrelated to your change? https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... File chrome/common/content_settings_initializer.h (right): https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... chrome/common/content_settings_initializer.h:10: void InitContentSettingsComponent(); nit. Initialize.* it's a bit generic name, however, why not just RegisterAdditionalContentSettingURLSchemes() or similar? Why an extra file and not just inline it in the ChromeMainDelegate? If you want an extra file, why not put it in a chrome/common/content_settings/ subdir?
https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... File chrome/browser/notifications/desktop_notification_profile_util.h (right): https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... chrome/browser/notifications/desktop_notification_profile_util.h:8: #include "base/macros.h" On 2014/09/02 08:15:50, jochen wrote: > why do you include base/macros.h everywhere? this file strikes me as unrelated > to your change? I removed some includes from content_settings_pattern.h. These files stopped compiling. https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... File chrome/common/content_settings_initializer.h (right): https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... chrome/common/content_settings_initializer.h:10: void InitContentSettingsComponent(); On 2014/09/02 08:15:50, jochen wrote: > nit. Initialize.* > > it's a bit generic name, however, why not just > RegisterAdditionalContentSettingURLSchemes() or similar? Why an extra file and > not just inline it in the ChromeMainDelegate? > > If you want an extra file, why not put it in a chrome/common/content_settings/ > subdir? Presubmit wasn't happy: Illegal include: "extensions/common/constants.h". Rename - done. There is no chrome/common/content_settings/. All content_settings* files are in chrome/common
On 2014/09/02 08:50:37, vasilii wrote: > https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... > File chrome/browser/notifications/desktop_notification_profile_util.h (right): > > https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... > chrome/browser/notifications/desktop_notification_profile_util.h:8: #include > "base/macros.h" > On 2014/09/02 08:15:50, jochen wrote: > > why do you include base/macros.h everywhere? this file strikes me as unrelated > > to your change? > > I removed some includes from content_settings_pattern.h. These files stopped > compiling. > > https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... > File chrome/common/content_settings_initializer.h (right): > > https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... > chrome/common/content_settings_initializer.h:10: void > InitContentSettingsComponent(); > On 2014/09/02 08:15:50, jochen wrote: > > nit. Initialize.* > > > > it's a bit generic name, however, why not just > > RegisterAdditionalContentSettingURLSchemes() or similar? Why an extra file and > > not just inline it in the ChromeMainDelegate? > > > > If you want an extra file, why not put it in a chrome/common/content_settings/ > > subdir? > > Presubmit wasn't happy: Illegal include: "extensions/common/constants.h". Rename > - done. There is no chrome/common/content_settings/. All content_settings* files > are in chrome/common I would guess it's fine to add that DEPS to //chrome/app, fwiw.
On 2014/09/02 at 08:53:18, blundell wrote: > On 2014/09/02 08:50:37, vasilii wrote: > > https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... > > File chrome/browser/notifications/desktop_notification_profile_util.h (right): > > > > https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... > > chrome/browser/notifications/desktop_notification_profile_util.h:8: #include > > "base/macros.h" > > On 2014/09/02 08:15:50, jochen wrote: > > > why do you include base/macros.h everywhere? this file strikes me as unrelated > > > to your change? > > > > I removed some includes from content_settings_pattern.h. These files stopped > > compiling. > > > > https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... > > File chrome/common/content_settings_initializer.h (right): > > > > https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... > > chrome/common/content_settings_initializer.h:10: void > > InitContentSettingsComponent(); > > On 2014/09/02 08:15:50, jochen wrote: > > > nit. Initialize.* > > > > > > it's a bit generic name, however, why not just > > > RegisterAdditionalContentSettingURLSchemes() or similar? Why an extra file and > > > not just inline it in the ChromeMainDelegate? > > > > > > If you want an extra file, why not put it in a chrome/common/content_settings/ > > > subdir? > > > > Presubmit wasn't happy: Illegal include: "extensions/common/constants.h". Rename > > - done. There is no chrome/common/content_settings/. All content_settings* files > > are in chrome/common > > I would guess it's fine to add that DEPS to //chrome/app, fwiw. yes, you could just whitelist the one header you need.
On 2014/09/02 08:54:53, jochen wrote: > On 2014/09/02 at 08:53:18, blundell wrote: > > On 2014/09/02 08:50:37, vasilii wrote: > > > > https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... > > > File chrome/browser/notifications/desktop_notification_profile_util.h > (right): > > > > > > > https://codereview.chromium.org/440423003/diff/260001/chrome/browser/notifica... > > > chrome/browser/notifications/desktop_notification_profile_util.h:8: #include > > > "base/macros.h" > > > On 2014/09/02 08:15:50, jochen wrote: > > > > why do you include base/macros.h everywhere? this file strikes me as > unrelated > > > > to your change? > > > > > > I removed some includes from content_settings_pattern.h. These files stopped > > > compiling. > > > > > > > https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... > > > File chrome/common/content_settings_initializer.h (right): > > > > > > > https://codereview.chromium.org/440423003/diff/260001/chrome/common/content_s... > > > chrome/common/content_settings_initializer.h:10: void > > > InitContentSettingsComponent(); > > > On 2014/09/02 08:15:50, jochen wrote: > > > > nit. Initialize.* > > > > > > > > it's a bit generic name, however, why not just > > > > RegisterAdditionalContentSettingURLSchemes() or similar? Why an extra file > and > > > > not just inline it in the ChromeMainDelegate? > > > > > > > > If you want an extra file, why not put it in a > chrome/common/content_settings/ > > > > subdir? > > > > > > Presubmit wasn't happy: Illegal include: "extensions/common/constants.h". > Rename > > > - done. There is no chrome/common/content_settings/. All content_settings* > files > > > are in chrome/common > > > > I would guess it's fine to add that DEPS to //chrome/app, fwiw. > > yes, you could just whitelist the one header you need. Done.
lgtm
Hooray!
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vasilii@chromium.org/440423003/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as 77e83962ccd5d993c4f6c025af45b2562dc396b6
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/94a1f29d656ac654ac0248e00f2d47fe2da7d446 Cr-Commit-Position: refs/heads/master@{#292906} |
