|
|
Created:
3 years, 8 months ago by Marc Treib Modified:
3 years, 6 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLocal NTP: Deploy strict-dynamic CSP
BUG=704518
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2805133004
Cr-Commit-Position: refs/heads/master@{#466959}
Committed: https://chromium.googlesource.com/chromium/src/+/3be61a94164073f03be243df0f388de723d3274f
Patch Set 1 #
Total comments: 22
Patch Set 2 : review #Patch Set 3 : shave ALL the yaks! #Patch Set 4 : fix #
Total comments: 9
Patch Set 5 : review #Patch Set 6 : remove GoogleSearchProviderService #Patch Set 7 : . #Patch Set 8 : rebase #Patch Set 9 : rebase #
Total comments: 3
Messages
Total messages: 65 (42 generated)
Description was changed from ========== Local NTP: Deploy strict-dynamic CSP BUG=704518 ========== to ========== Local NTP: Deploy strict-dynamic CSP BUG=704518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
treib@chromium.org changed reviewers: + sfiera@chromium.org
PTAL! By itself, this doesn't do anything useful, but it's a prerequisite for Doodle and OGB support. And I think it'd be nice to have this part in one version earlier than the actual functionality, to make sure it doesn't break anything. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.html (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.html:10: integrity="{{CONFIG_INTEGRITY}}"></script> Kinda ugly, but similar to this: https://cs.chromium.org/chromium/src/chrome/browser/resources/local_ntp/most_... https://cs.chromium.org/chromium/src/chrome/browser/search/iframe_source.cc?r... https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. This is somewhat inconvenient. The alternative would be to dynamically compute and insert the checksum, like for config.js. We'd buy some development convenience with a runtime cost though, so I kinda prefer the way it is. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:205: // config hash. ...which would be somewhat annoying in terms of plumbing. Since I've never actually used this stuff, I'm kinda tempted to just rip it out. WDYT? https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:279: return "script-src 'strict-dynamic' " Could hide this behind a (default-enabled) feature, just to be sure...?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/07 16:46:15, Marc Treib wrote: > This is somewhat inconvenient. The alternative would be to dynamically compute > and insert the checksum, like for config.js. We'd buy some development > convenience with a runtime cost though, so I kinda prefer the way it is. Is there a test that will fail straightforwardly if someone forgets? https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:205: // config hash. On 2017/04/07 16:46:15, Marc Treib wrote: > ...which would be somewhat annoying in terms of plumbing. Since I've never > actually used this stuff, I'm kinda tempted to just rip it out. WDYT? What's it actually for? Live-editing the files in the src/ tree? Can we just disable integrity-checking in that case? Simpler than making it work. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:279: return "script-src 'strict-dynamic' " On 2017/04/07 16:46:15, Marc Treib wrote: > Could hide this behind a (default-enabled) feature, just to be sure...? Right now, users only land on the local NTP if they're offline, right? Do we know how often that is? My inclination would be to not worry about it. (obviously if we run an experiment and detect problems, we can pull the plug on the experiment) https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:281: GetIntegritySha256Value(GetConfigData(profile_)) + Are we going to be computing this hash multiple times on each NTP load? https://codereview.chromium.org/2805133004/diff/1/chrome/browser/ui/search/lo... File chrome/browser/ui/search/local_ntp_browsertest.cc (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:376: base::MakeUnique<content::ConsoleObserverDelegate>(active_tab, "*"); Is there a reason to use a unique_ptr here instead of the object directly? https://codereview.chromium.org/2805133004/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:384: ASSERT_TRUE(search::IsInstantNTP(active_tab)); Can we also assert that this is the local NTP?
Comments addressed. NewTabPageInterceptorTest is still failing; I'll fix that soon. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 08:53:21, sfiera wrote: > On 2017/04/07 16:46:15, Marc Treib wrote: > > This is somewhat inconvenient. The alternative would be to dynamically compute > > and insert the checksum, like for config.js. We'd buy some development > > convenience with a runtime cost though, so I kinda prefer the way it is. > > Is there a test that will fail straightforwardly if someone forgets? Yup, that's what the newly added LocalNTPSmokeTest is for, mostly. It's in interactive_ui_tests, so it's somewhat debatable how "straightforward" it is, but there you go. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:205: // config hash. On 2017/04/10 08:53:21, sfiera wrote: > On 2017/04/07 16:46:15, Marc Treib wrote: > > ...which would be somewhat annoying in terms of plumbing. Since I've never > > actually used this stuff, I'm kinda tempted to just rip it out. WDYT? > > What's it actually for? Live-editing the files in the src/ tree? Can we just > disable integrity-checking in that case? Simpler than making it work. Yup, it's for live-editing. Good idea, I'll turn off the CSP if that switch is set. There's still the hash in the html file itself, I guess you'll just have to manually remove or update that. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:279: return "script-src 'strict-dynamic' " On 2017/04/10 08:53:21, sfiera wrote: > On 2017/04/07 16:46:15, Marc Treib wrote: > > Could hide this behind a (default-enabled) feature, just to be sure...? > > Right now, users only land on the local NTP if they're offline, right? Do we > know how often that is? My inclination would be to not worry about it. Even while offline, there should usually be a cached version of the remote NTP. However, metrics say that something like 10% of all NTP loads are on the local NTP, so it's not all that rare. > (obviously if we run an experiment and detect problems, we can pull the plug on > the experiment) My main worry would be that not everything can be put behind the experiment in a straightforward way - e.g. the hashes in local_ntp.html wouldn't be trivial to take out. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:281: GetIntegritySha256Value(GetConfigData(profile_)) + On 2017/04/10 08:53:21, sfiera wrote: > Are we going to be computing this hash multiple times on each NTP load? Yes; twice at the very least, probably even more often. Unfortunately, it's not entirely trivial to cache, or more precisely, when to invalidate the cache. Are you worried about performance? https://codereview.chromium.org/2805133004/diff/1/chrome/browser/ui/search/lo... File chrome/browser/ui/search/local_ntp_browsertest.cc (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:376: base::MakeUnique<content::ConsoleObserverDelegate>(active_tab, "*"); On 2017/04/10 08:53:21, sfiera wrote: > Is there a reason to use a unique_ptr here instead of the object directly? Nope, I don't think so. That's what you get for copy-pasting. Done. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/ui/search/lo... chrome/browser/ui/search/local_ntp_browsertest.cc:384: ASSERT_TRUE(search::IsInstantNTP(active_tab)); On 2017/04/10 08:53:22, sfiera wrote: > Can we also assert that this is the local NTP? Good idea, done.
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 10:11:18, Marc Treib wrote: > On 2017/04/10 08:53:21, sfiera wrote: > > On 2017/04/07 16:46:15, Marc Treib wrote: > > > This is somewhat inconvenient. The alternative would be to dynamically > compute > > > and insert the checksum, like for config.js. We'd buy some development > > > convenience with a runtime cost though, so I kinda prefer the way it is. > > > > Is there a test that will fail straightforwardly if someone forgets? > > Yup, that's what the newly added LocalNTPSmokeTest is for, mostly. > It's in interactive_ui_tests, so it's somewhat debatable how "straightforward" > it is, but there you go. What does a failure look like? I would hope for something like “Expected: kLocalNTPJSHash equals "sha-256-BaSe64=="; actual: "sha-256-bAsE64=="”, so if everything is fine, you can copy the actual value and it's clear where to copy it. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:281: GetIntegritySha256Value(GetConfigData(profile_)) + On 2017/04/10 10:11:18, Marc Treib wrote: > On 2017/04/10 08:53:21, sfiera wrote: > > Are we going to be computing this hash multiple times on each NTP load? > > Yes; twice at the very least, probably even more often. Unfortunately, it's not > entirely trivial to cache, or more precisely, when to invalidate the cache. I was thinking about computing once when the HTML is loaded, since the HTML would be loaded first (I assume), and the hashes in the CSP need to match those in the HTML file. > Are you worried about performance? That was my thought, but I guess this is Desktop-only.
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 10:28:53, sfiera wrote: > On 2017/04/10 10:11:18, Marc Treib wrote: > > On 2017/04/10 08:53:21, sfiera wrote: > > > On 2017/04/07 16:46:15, Marc Treib wrote: > > > > This is somewhat inconvenient. The alternative would be to dynamically > > compute > > > > and insert the checksum, like for config.js. We'd buy some development > > > > convenience with a runtime cost though, so I kinda prefer the way it is. > > > > > > Is there a test that will fail straightforwardly if someone forgets? > > > > Yup, that's what the newly added LocalNTPSmokeTest is for, mostly. > > It's in interactive_ui_tests, so it's somewhat debatable how "straightforward" > > it is, but there you go. > > What does a failure look like? I would hope for something like “Expected: > kLocalNTPJSHash equals "sha-256-BaSe64=="; actual: "sha-256-bAsE64=="”, so if > everything is fine, you can copy the actual value and it's clear where to copy > it. "Failed to find a valid digest in the 'integrity' attribute for resource 'chrome-search://local-ntp/local-ntp.js' with computed SHA-256 integrity 'g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0='. The resource has been blocked." So yup, should be good. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/search/local... chrome/browser/search/local_ntp_source.cc:281: GetIntegritySha256Value(GetConfigData(profile_)) + On 2017/04/10 10:28:53, sfiera wrote: > On 2017/04/10 10:11:18, Marc Treib wrote: > > On 2017/04/10 08:53:21, sfiera wrote: > > > Are we going to be computing this hash multiple times on each NTP load? > > > > Yes; twice at the very least, probably even more often. Unfortunately, it's > not > > entirely trivial to cache, or more precisely, when to invalidate the cache. > > I was thinking about computing once when the HTML is loaded, since the HTML > would be loaded first (I assume), and the hashes in the CSP need to match those > in the HTML file. The CSP is served through HTTP headers, so that actually comes first. Also it seems that GetContentSecurityPolicyScriptSrc is called multiple times during a page load, I don't know why. But we can't really assume any relation between HTML load and CSP query. > > Are you worried about performance? > > That was my thought, but I guess this is Desktop-only. Yup. Still shouldn't ignore performance of course...
LGTM https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 11:44:03, Marc Treib wrote: > On 2017/04/10 10:28:53, sfiera wrote: > > On 2017/04/10 10:11:18, Marc Treib wrote: > > > On 2017/04/10 08:53:21, sfiera wrote: > > > > On 2017/04/07 16:46:15, Marc Treib wrote: > > > > > This is somewhat inconvenient. The alternative would be to dynamically > > > compute > > > > > and insert the checksum, like for config.js. We'd buy some development > > > > > convenience with a runtime cost though, so I kinda prefer the way it is. > > > > > > > > Is there a test that will fail straightforwardly if someone forgets? > > > > > > Yup, that's what the newly added LocalNTPSmokeTest is for, mostly. > > > It's in interactive_ui_tests, so it's somewhat debatable how > "straightforward" > > > it is, but there you go. > > > > What does a failure look like? I would hope for something like “Expected: > > kLocalNTPJSHash equals "sha-256-BaSe64=="; actual: "sha-256-bAsE64=="”, so if > > everything is fine, you can copy the actual value and it's clear where to copy > > it. > > "Failed to find a valid digest in the 'integrity' attribute for resource > 'chrome-search://local-ntp/local-ntp.js' with computed SHA-256 integrity > 'g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0='. The resource has been blocked." > > So yup, should be good. To be sure, the printed value is the actual value, not the expected, right?
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 12:08:34, sfiera wrote: > On 2017/04/10 11:44:03, Marc Treib wrote: > > On 2017/04/10 10:28:53, sfiera wrote: > > > On 2017/04/10 10:11:18, Marc Treib wrote: > > > > On 2017/04/10 08:53:21, sfiera wrote: > > > > > On 2017/04/07 16:46:15, Marc Treib wrote: > > > > > > This is somewhat inconvenient. The alternative would be to dynamically > > > > compute > > > > > > and insert the checksum, like for config.js. We'd buy some development > > > > > > convenience with a runtime cost though, so I kinda prefer the way it > is. > > > > > > > > > > Is there a test that will fail straightforwardly if someone forgets? > > > > > > > > Yup, that's what the newly added LocalNTPSmokeTest is for, mostly. > > > > It's in interactive_ui_tests, so it's somewhat debatable how > > "straightforward" > > > > it is, but there you go. > > > > > > What does a failure look like? I would hope for something like “Expected: > > > kLocalNTPJSHash equals "sha-256-BaSe64=="; actual: "sha-256-bAsE64=="”, so > if > > > everything is fine, you can copy the actual value and it's clear where to > copy > > > it. > > > > "Failed to find a valid digest in the 'integrity' attribute for resource > > 'chrome-search://local-ntp/local-ntp.js' with computed SHA-256 integrity > > 'g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0='. The resource has been > blocked." > > > > So yup, should be good. > > To be sure, the printed value is the actual value, not the expected, right? It's the value you have to paste in to make things work, so I guess "expected". Which is what you'd want, no?
On 2017/04/10 12:16:18, Marc Treib wrote: > https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... > File chrome/browser/resources/local_ntp/local_ntp.js (right): > > https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/lo... > chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in > LocalNtpSource::GetContentSecurityPolicyScriptSrc. > On 2017/04/10 12:08:34, sfiera wrote: > > On 2017/04/10 11:44:03, Marc Treib wrote: > > > On 2017/04/10 10:28:53, sfiera wrote: > > > > On 2017/04/10 10:11:18, Marc Treib wrote: > > > > > On 2017/04/10 08:53:21, sfiera wrote: > > > > > > On 2017/04/07 16:46:15, Marc Treib wrote: > > > > > > > This is somewhat inconvenient. The alternative would be to > dynamically > > > > > compute > > > > > > > and insert the checksum, like for config.js. We'd buy some > development > > > > > > > convenience with a runtime cost though, so I kinda prefer the way it > > is. > > > > > > > > > > > > Is there a test that will fail straightforwardly if someone forgets? > > > > > > > > > > Yup, that's what the newly added LocalNTPSmokeTest is for, mostly. > > > > > It's in interactive_ui_tests, so it's somewhat debatable how > > > "straightforward" > > > > > it is, but there you go. > > > > > > > > What does a failure look like? I would hope for something like “Expected: > > > > kLocalNTPJSHash equals "sha-256-BaSe64=="; actual: "sha-256-bAsE64=="”, so > > if > > > > everything is fine, you can copy the actual value and it's clear where to > > copy > > > > it. > > > > > > "Failed to find a valid digest in the 'integrity' attribute for resource > > > 'chrome-search://local-ntp/local-ntp.js' with computed SHA-256 integrity > > > 'g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0='. The resource has been > > blocked." > > > > > > So yup, should be good. > > > > To be sure, the printed value is the actual value, not the expected, right? > > It's the value you have to paste in to make things work, so I guess "expected". > Which is what you'd want, no? I'm thinking along the lines of "EXPECT_THAT(actual, Eq(expected))". It tells you the actual value, you paste it over the expected value.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL again! The good news is that those failing tests pointed at an actual problem. The bad news is that fixing it involved two fun topics, threads and lifetime, and a whole lot of yak shaving. More detail: GetContentSecurityPolicyChildSrc is called on the IO thread, meaning it can't do all the TemplateURL crap to find out if Google is the default search engine. Also, LocalNtpSource might outlive TemplateURLService, so it can't observe it directly, because it doesn't get notified if the service goes away.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... File chrome/browser/search/google_search_provider_service.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... chrome/browser/search/google_search_provider_service.h:17: // Tracks whether the default search provider is Google. Comments? Tests? It's not clear from the header why this class exists, what you gain by using it over observing TemplateURLService directly, or if it delivers on it. https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... File chrome/browser/search/google_search_provider_service_observer.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... chrome/browser/search/google_search_provider_service_observer.h:8: class GoogleSearchProviderServiceObserver { Comments? https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/l... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/l... chrome/browser/search/local_ntp_source.cc:295: default_search_provider_is_google_ = is_google; What thread is this method called from? If it's the UI thread, and we're reading it from (only?) the IO thread, shouldn't we send the write to the IO thread too?
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for the missing tests and comments! I guess I was too happy that I finally got it working... PTAL again! Not urgent though; I'm not planning to land this in M59 anymore. It's kinda gotten too hairy to drop a day before BP. https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... File chrome/browser/search/google_search_provider_service.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... chrome/browser/search/google_search_provider_service.h:17: // Tracks whether the default search provider is Google. On 2017/04/11 09:45:17, sfiera wrote: > Comments? Tests? It's not clear from the header why this class exists, what you > gain by using it over observing TemplateURLService directly, or if it delivers > on it. Two things: 1) It abstracts away the kinda yucky TemplateURL wrangling. I think this should be clear from the new comments. 2) It notifies observers when it's shutting down, so they can de-register. I've added a comment to Add/RemoveObserver. (Of course, this functionality could be added to TemplateURLService too.) https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... File chrome/browser/search/google_search_provider_service_observer.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... chrome/browser/search/google_search_provider_service_observer.h:8: class GoogleSearchProviderServiceObserver { On 2017/04/11 09:45:17, sfiera wrote: > Comments? Done. https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/l... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/l... chrome/browser/search/local_ntp_source.cc:295: default_search_provider_is_google_ = is_google; On 2017/04/11 09:45:17, sfiera wrote: > What thread is this method called from? If it's the UI thread, and we're reading > it from (only?) the IO thread, shouldn't we send the write to the IO thread too? Threading in URLDataSource is kind of a mess :( The ctor and StartDataRequest are called on the UI thread, most of the other stuff on the IO thread. I've added a bunch of DCHECK_CURRENTLY_ONs to make things clearer to the reader. This method is called on the UI thread, but the bool is read on both UI and IO thread, which is not quite safe :-/ I've added a copy of the variable for the IO thread. I'm not particularly happy with this, it feels super fragile... an std::atomic<bool> might have been nice, but that's banned (and also atomics are generally discouraged). Better ideas welcome.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... File chrome/browser/search/google_search_provider_service.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... chrome/browser/search/google_search_provider_service.h:17: // Tracks whether the default search provider is Google. On 2017/04/11 12:37:59, Marc Treib wrote: > On 2017/04/11 09:45:17, sfiera wrote: > > Comments? Tests? It's not clear from the header why this class exists, what > you > > gain by using it over observing TemplateURLService directly, or if it delivers > > on it. > > Two things: > 1) It abstracts away the kinda yucky TemplateURL wrangling. I think this should > be clear from the new comments. On its own, I'm not sure that motivates this class to exist. Exposing DefaultSearchProviderIsGoogleImpl() would serve as well, or better. > 2) It notifies observers when it's shutting down, so they can de-register. I've > added a comment to Add/RemoveObserver. (Of course, this functionality could be > added to TemplateURLService too.) Can we add a TODO or a bug to add shutdown to its observer interface? https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/l... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/l... chrome/browser/search/local_ntp_source.cc:295: default_search_provider_is_google_ = is_google; On 2017/04/11 12:37:59, Marc Treib wrote: > On 2017/04/11 09:45:17, sfiera wrote: > > What thread is this method called from? If it's the UI thread, and we're > reading > > it from (only?) the IO thread, shouldn't we send the write to the IO thread > too? > > Threading in URLDataSource is kind of a mess :( The ctor and StartDataRequest > are called on the UI thread, most of the other stuff on the IO thread. I've > added a bunch of DCHECK_CURRENTLY_ONs to make things clearer to the reader. > > This method is called on the UI thread, but the bool is read on both UI and IO > thread, which is not quite safe :-/ I've added a copy of the variable for the > IO thread. I'm not particularly happy with this, it feels super fragile... an > std::atomic<bool> might have been nice, but that's banned (and also atomics are > generally discouraged). Better ideas welcome. Alright, thanks. This is ugly, but feels like the right thing to do given the circumstances.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... File chrome/browser/search/google_search_provider_service.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/g... chrome/browser/search/google_search_provider_service.h:17: // Tracks whether the default search provider is Google. On 2017/04/11 14:07:35, sfiera wrote: > On 2017/04/11 12:37:59, Marc Treib wrote: > > On 2017/04/11 09:45:17, sfiera wrote: > > > Comments? Tests? It's not clear from the header why this class exists, what > > you > > > gain by using it over observing TemplateURLService directly, or if it > delivers > > > on it. > > > > Two things: > > 1) It abstracts away the kinda yucky TemplateURL wrangling. I think this > should > > be clear from the new comments. > > On its own, I'm not sure that motivates this class to exist. Exposing > DefaultSearchProviderIsGoogleImpl() would serve as well, or better. > > > 2) It notifies observers when it's shutting down, so they can de-register. > I've > > added a comment to Add/RemoveObserver. (Of course, this functionality could be > > added to TemplateURLService too.) > > Can we add a TODO or a bug to add shutdown to its observer interface? You're right, this isn't really worth a new service. I've added an OnShutdown method to TemplateURLServiceObserver (in a separate CL) and removed the new service again. Let's see what the TemplateURLService owners say.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All ready now! Do you want to take a last look at the diff between patch set 5 and 8?
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2805133004/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493125168770420, "parent_rev": "a5bdc7b47269c5ba3b9a5fa75c42935c7fffec2d", "commit_rev": "3be61a94164073f03be243df0f388de723d3274f"}
Message was sent while issue was closed.
Description was changed from ========== Local NTP: Deploy strict-dynamic CSP BUG=704518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Local NTP: Deploy strict-dynamic CSP BUG=704518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2805133004 Cr-Commit-Position: refs/heads/master@{#466959} Committed: https://chromium.googlesource.com/chromium/src/+/3be61a94164073f03be243df0f38... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/3be61a94164073f03be243df0f38...
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... chrome/browser/search/local_ntp_source.cc:346: "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; is there perhaps a more dynamic way to do this?
Message was sent while issue was closed.
https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... chrome/browser/search/local_ntp_source.cc:346: "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; On 2017/05/30 17:09:52, Dan Beam wrote: > is there perhaps a more dynamic way to do this? Marc's OOO this week. Doing it at runtime would be easy, but undesirable because it would slow down NTP loads. Doing it at compile time might be possible; I guess we'd have to generate a header?
Message was sent while issue was closed.
https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... chrome/browser/search/local_ntp_source.cc:346: "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; On 2017/05/31 08:22:39, sfiera wrote: > On 2017/05/30 17:09:52, Dan Beam wrote: > > is there perhaps a more dynamic way to do this? > > Marc's OOO this week. > > Doing it at runtime would be easy, but undesirable because it would slow down > NTP loads. Doing it at compile time might be possible; I guess we'd have to > generate a header? yes, generating at compile time would be great
Message was sent while issue was closed.
On 2017/05/31 17:49:23, Dan Beam wrote: > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... > File chrome/browser/search/local_ntp_source.cc (right): > > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... > chrome/browser/search/local_ntp_source.cc:346: > "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; > On 2017/05/31 08:22:39, sfiera wrote: > > On 2017/05/30 17:09:52, Dan Beam wrote: > > > is there perhaps a more dynamic way to do this? > > > > Marc's OOO this week. > > > > Doing it at runtime would be easy, but undesirable because it would slow down > > NTP loads. Doing it at compile time might be possible; I guess we'd have to > > generate a header? > > yes, generating at compile time would be great Indeed it would. It's complicated by the fact that the checksum is needed both in the .cc and in the .html, so just a header won't do. Any good ideas on how to get it into the .html?
Message was sent while issue was closed.
On 2017/06/07 08:51:40, Marc Treib (OOO till June 7) wrote: > On 2017/05/31 17:49:23, Dan Beam wrote: > > > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... > > File chrome/browser/search/local_ntp_source.cc (right): > > > > > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... > > chrome/browser/search/local_ntp_source.cc:346: > > "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; > > On 2017/05/31 08:22:39, sfiera wrote: > > > On 2017/05/30 17:09:52, Dan Beam wrote: > > > > is there perhaps a more dynamic way to do this? > > > > > > Marc's OOO this week. > > > > > > Doing it at runtime would be easy, but undesirable because it would slow > down > > > NTP loads. Doing it at compile time might be possible; I guess we'd have to > > > generate a header? > > > > yes, generating at compile time would be great > > Indeed it would. It's complicated by the fact that the checksum is needed both > in the .cc and in the .html, so just a header won't do. > Any good ideas on how to get it into the .html? what about with $i18n{}?
Message was sent while issue was closed.
On 2017/06/07 19:00:30, Dan Beam wrote: > On 2017/06/07 08:51:40, Marc Treib (OOO till June 7) wrote: > > On 2017/05/31 17:49:23, Dan Beam wrote: > > > > > > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... > > > File chrome/browser/search/local_ntp_source.cc (right): > > > > > > > > > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/... > > > chrome/browser/search/local_ntp_source.cc:346: > > > "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; > > > On 2017/05/31 08:22:39, sfiera wrote: > > > > On 2017/05/30 17:09:52, Dan Beam wrote: > > > > > is there perhaps a more dynamic way to do this? > > > > > > > > Marc's OOO this week. > > > > > > > > Doing it at runtime would be easy, but undesirable because it would slow > > down > > > > NTP loads. Doing it at compile time might be possible; I guess we'd have > to > > > > generate a header? > > > > > > yes, generating at compile time would be great > > > > Indeed it would. It's complicated by the fact that the checksum is needed both > > in the .cc and in the .html, so just a header won't do. > > Any good ideas on how to get it into the .html? > > what about with $i18n{}? Thanks, I didn't think of that! Filed crbug.com/731027 |