Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(10)

Issue 2805133004: Local NTP: Deploy strict-dynamic CSP (Closed)

Created:
3 years, 8 months ago by Marc Treib
Modified:
3 years, 6 months ago
Reviewers:
sfiera, Dan Beam
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.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+335 lines, -60 lines) Patch
M chrome/browser/resources/local_ntp/local_ntp.html View 1 2 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/resources/local_ntp/local_ntp.js View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/resources/local_ntp/most_visited_single.js View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/search/local_ntp_source.h View 1 2 3 4 5 3 chunks +17 lines, -0 lines 0 comments Download
M chrome/browser/search/local_ntp_source.cc View 1 2 3 4 5 6 12 chunks +151 lines, -27 lines 3 comments Download
M chrome/browser/ui/search/local_ntp_browsertest.cc View 1 5 chunks +137 lines, -3 lines 0 comments Download
M chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc View 1 2 3 4 5 6 7 8 2 chunks +20 lines, -27 lines 0 comments Download

Messages

Total messages: 65 (42 generated)
Marc Treib
PTAL! By itself, this doesn't do anything useful, but it's a prerequisite for Doodle and ...
3 years, 8 months ago (2017-04-07 16:46:15 UTC) #5
sfiera
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js#newcode7 chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/07 16:46:15, Marc ...
3 years, 8 months ago (2017-04-10 08:53:22 UTC) #8
Marc Treib
Comments addressed. NewTabPageInterceptorTest is still failing; I'll fix that soon. https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js#newcode7 ...
3 years, 8 months ago (2017-04-10 10:11:18 UTC) #9
sfiera
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js#newcode7 chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 10:11:18, Marc ...
3 years, 8 months ago (2017-04-10 10:28:54 UTC) #10
Marc Treib
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js#newcode7 chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 10:28:53, sfiera ...
3 years, 8 months ago (2017-04-10 11:44:03 UTC) #11
sfiera
LGTM https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js#newcode7 chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 11:44:03, ...
3 years, 8 months ago (2017-04-10 12:08:34 UTC) #12
Marc Treib
https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js#newcode7 chrome/browser/resources/local_ntp/local_ntp.js:7: // local_ntp.html and in LocalNtpSource::GetContentSecurityPolicyScriptSrc. On 2017/04/10 12:08:34, sfiera ...
3 years, 8 months ago (2017-04-10 12:16:18 UTC) #13
sfiera
On 2017/04/10 12:16:18, Marc Treib wrote: > https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js > File chrome/browser/resources/local_ntp/local_ntp.js (right): > > https://codereview.chromium.org/2805133004/diff/1/chrome/browser/resources/local_ntp/local_ntp.js#newcode7 ...
3 years, 8 months ago (2017-04-10 12:22:51 UTC) #14
Marc Treib
PTAL again! The good news is that those failing tests pointed at an actual problem. ...
3 years, 8 months ago (2017-04-10 15:29:45 UTC) #19
sfiera
https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/google_search_provider_service.h File chrome/browser/search/google_search_provider_service.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/google_search_provider_service.h#newcode17 chrome/browser/search/google_search_provider_service.h:17: // Tracks whether the default search provider is Google. ...
3 years, 8 months ago (2017-04-11 09:45:17 UTC) #22
Marc Treib
Sorry for the missing tests and comments! I guess I was too happy that I ...
3 years, 8 months ago (2017-04-11 12:37:59 UTC) #25
sfiera
LGTM https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/google_search_provider_service.h File chrome/browser/search/google_search_provider_service.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/google_search_provider_service.h#newcode17 chrome/browser/search/google_search_provider_service.h:17: // Tracks whether the default search provider is ...
3 years, 8 months ago (2017-04-11 14:07:35 UTC) #28
Marc Treib
https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/google_search_provider_service.h File chrome/browser/search/google_search_provider_service.h (right): https://codereview.chromium.org/2805133004/diff/60001/chrome/browser/search/google_search_provider_service.h#newcode17 chrome/browser/search/google_search_provider_service.h:17: // Tracks whether the default search provider is Google. ...
3 years, 8 months ago (2017-04-12 11:53:46 UTC) #31
Marc Treib
All ready now! Do you want to take a last look at the diff between ...
3 years, 8 months ago (2017-04-13 08:30:15 UTC) #40
sfiera
LGTM
3 years, 8 months ago (2017-04-13 08:53:30 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2805133004/180001
3 years, 7 months ago (2017-04-25 12:59:43 UTC) #55
commit-bot: I haz the power
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/3be61a94164073f03be243df0f388de723d3274f
3 years, 7 months ago (2017-04-25 13:03:51 UTC) #58
Dan Beam
https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc#newcode346 chrome/browser/search/local_ntp_source.cc:346: "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; is there perhaps a more dynamic way to ...
3 years, 6 months ago (2017-05-30 17:09:52 UTC) #60
sfiera
https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc#newcode346 chrome/browser/search/local_ntp_source.cc:346: "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; On 2017/05/30 17:09:52, Dan Beam wrote: > is ...
3 years, 6 months ago (2017-05-31 08:22:39 UTC) #61
Dan Beam
https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc#newcode346 chrome/browser/search/local_ntp_source.cc:346: "'sha256-g38WaUaxnOIWY7E2LtLZ5ff9r5sn1dBj80jevt/kmx0=';"; On 2017/05/31 08:22:39, sfiera wrote: > On 2017/05/30 ...
3 years, 6 months ago (2017-05-31 17:49:23 UTC) #62
Marc Treib
On 2017/05/31 17:49:23, Dan Beam wrote: > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc > File chrome/browser/search/local_ntp_source.cc (right): > > https://codereview.chromium.org/2805133004/diff/180001/chrome/browser/search/local_ntp_source.cc#newcode346 ...
3 years, 6 months ago (2017-06-07 08:51:40 UTC) #63
Dan Beam
On 2017/06/07 08:51:40, Marc Treib (OOO till June 7) wrote: > On 2017/05/31 17:49:23, Dan ...
3 years, 6 months ago (2017-06-07 19:00:30 UTC) #64
Marc Treib
3 years, 6 months ago (2017-06-08 09:40:20 UTC) #65
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

Powered by Google App Engine
This is Rietveld 408576698