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

Issue 2516383002: Embedding-CSP: Refactoring directive strings into enum (Closed)

Created:
4 years, 1 month ago by amalika
Modified:
4 years ago
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Embedding-CSP: Refactoring directive strings into enum. This is an effort as part of the experimental feature. Causing patch: https://codereview.chromium.org/2474903002 BUG=647588 Committed: https://crrev.com/a5e9944b7169dd3a3c9f2cb352aff3551b32cd12 Cr-Commit-Position: refs/heads/master@{#434474}

Patch Set 1 #

Total comments: 9

Patch Set 2 : Changing reportViolation*() to accepting DirectiveType + adding a unit tests #

Total comments: 9

Patch Set 3 : Addressing comments #

Total comments: 4

Patch Set 4 : Second inversion/fixing default for a switch #

Total comments: 3

Patch Set 5 : Rebasing #

Patch Set 6 : syncing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+335 lines, -233 lines) Patch
M third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.h View 1 3 chunks +9 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp View 1 28 chunks +115 lines, -94 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h View 1 2 3 4 5 chunks +30 lines, -40 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp View 1 2 3 12 chunks +129 lines, -90 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp View 1 2 3 4 1 chunk +50 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp View 1 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 38 (23 generated)
amalika
As per suggestion in Part 3.1 (https://codereview.chromium.org/2474903002), refactoring CSP to use enum instead of static ...
4 years, 1 month ago (2016-11-21 15:28:27 UTC) #3
Mike West
Looking good, thanks! https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2516383002/diff/1/third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp#newcode195 third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:195: ContentSecurityPolicy::BlockAllMixedContent), As noted above, I think ...
4 years ago (2016-11-23 08:58:16 UTC) #4
amalika
https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp#newcode1539 third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1539: default: Note that `Undefined` type will go to default. ...
4 years ago (2016-11-23 11:25:33 UTC) #6
Mike West
Thanks! https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/20001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp#newcode1539 third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1539: default: On 2016/11/23 at 11:25:33, amalika wrote: > ...
4 years ago (2016-11-23 11:38:34 UTC) #7
amalika
In part 3.1 https://codereview.chromium.org/2474903002, we need to iterate over all directives (line 1203 in CSPDirectiveList.cpp) ...
4 years ago (2016-11-23 12:42:15 UTC) #8
Mike West
LGTM % some nits, thanks! On 2016/11/23 at 12:42:15, amalika wrote: > In part 3.1 ...
4 years ago (2016-11-23 12:58:08 UTC) #11
amalika
A few changes! https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/40001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp#newcode1000 third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1000: effectiveType != ContentSecurityPolicy::DirectiveType::FrameSrc && On 2016/11/23 ...
4 years ago (2016-11-23 13:38:39 UTC) #15
Mike West
Thanks! Still LGTM % comments. https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2516383002/diff/80001/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp#newcode1545 third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:1545: return ""; On 2016/11/23 ...
4 years ago (2016-11-23 13:55:37 UTC) #16
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/2516383002/120001
4 years ago (2016-11-24 14:07:27 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_compile_dbg_ng/builds/304129)
4 years ago (2016-11-24 15:23:32 UTC) #24
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/2516383002/140001
4 years ago (2016-11-25 10:54:08 UTC) #31
commit-bot: I haz the power
Committed patchset #6 (id:140001)
4 years ago (2016-11-25 10:57:53 UTC) #33
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/a5e9944b7169dd3a3c9f2cb352aff3551b32cd12 Cr-Commit-Position: refs/heads/master@{#434474}
4 years ago (2016-11-25 11:00:03 UTC) #35
magjed_chromium
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2528133002/ by magjed@chromium.org. ...
4 years ago (2016-11-25 12:03:39 UTC) #36
magjed_chromium
4 years ago (2016-11-28 13:37:35 UTC) #38
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/2533683003/ by magjed@chromium.org.

The reason for reverting is: Causes ContentSecurityPolicyTest.DirectiveType to
fail in webkit_unit_tests:
C  143.019s Main  [FAIL] ContentSecurityPolicyTest.DirectiveType:
C  143.019s Main  [ RUN      ] ContentSecurityPolicyTest.DirectiveType
C  143.019s Main  
C  143.019s Main  [WARNING] ../../testing/gtest/src/gtest-death-test.cc:834::
Death tests use fork(), which is unsafe particularly in a threaded context. For
this test, Google Test detected 11 threads.
C  143.019s Main 
../../third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:950:
Failure
C  143.020s Main  Death test: ContentSecurityPolicy::getDirectiveName(
ContentSecurityPolicy::DirectiveType::Undefined)
C  143.020s Main      Result: failed to die.
C  143.020s Main   Error msg:
C  143.020s Main  [  DEATH   ] 
C  143.020s Main  [  FAILED  ] ContentSecurityPolicyTest.DirectiveType (24 ms)

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Andr....

Powered by Google App Engine
This is Rietveld 408576698