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

Issue 2452903004: Part 2.2: Is policy list subsumed under subsuming policy? (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

Part 2.2: Is policy list subsumed under subsuming policy? This is part of an experimental feature Embedding-CSP. In particular this is the second CL devoted to implementing *3.3. Is policy list subsumed under subsuming policy? Here we try to find whether a source list directive is subsuming to a vector of source list directives. Note: right now we only consider non-special keyword cases, i.e. CSPSources. BUG=647588 Committed: https://crrev.com/7650479aed19de17e02478abd1845ffea51e7dbf Cr-Commit-Position: refs/heads/master@{#431549}

Patch Set 1 : SourceListDirective #

Patch Set 2 : Separating normalization into a separate patch (namely, Part 2.1) #

Patch Set 3 : Adding more tests #

Patch Set 4 : After rebasing on part2.1 #

Total comments: 17

Patch Set 5 : Addressing comments #

Total comments: 1

Patch Set 6 : Rebasing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+228 lines, -0 lines) Patch
M third_party/WebKit/Source/core/frame/csp/CSPSource.h View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/CSPSource.cpp View 1 2 3 4 1 chunk +21 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/CSPSourceTest.cpp View 1 2 3 4 1 chunk +95 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/SourceListDirective.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp View 1 2 3 4 5 1 chunk +14 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp View 1 2 3 4 1 chunk +90 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 29 (17 generated)
amalika
4 years, 1 month ago (2016-10-27 14:08:54 UTC) #4
amalika
Adding jochen@ Splitting this patch into normalization and subsumption. This patch is now only devoted ...
4 years, 1 month ago (2016-11-02 10:36:47 UTC) #7
amalika
+ More tests. Note that subsumption direction here changes: isSubsumingOf instead of isSubsumedBy. it might ...
4 years, 1 month ago (2016-11-03 10:55:42 UTC) #9
amalika
Rebasing on part2.1
4 years, 1 month ago (2016-11-07 12:58:29 UTC) #11
Mike West
Looking good! I've left a number of comments, but they're mostly nits. :) https://codereview.chromium.org/2452903004/diff/120001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp File ...
4 years, 1 month ago (2016-11-10 15:04:55 UTC) #15
amalika
Source lists with wildcards will be considered separately. This patch now only deals with CSPSources.
4 years, 1 month ago (2016-11-10 20:03:34 UTC) #19
Mike West
On 2016/11/10 at 20:03:34, amalika wrote: > Source lists with wildcards will be considered > ...
4 years, 1 month ago (2016-11-11 07:52:14 UTC) #20
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/2452903004/180001
4 years, 1 month ago (2016-11-11 11:04:21 UTC) #23
commit-bot: I haz the power
Committed patchset #6 (id:180001)
4 years, 1 month ago (2016-11-11 12:43:01 UTC) #24
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/7650479aed19de17e02478abd1845ffea51e7dbf Cr-Commit-Position: refs/heads/master@{#431549}
4 years, 1 month ago (2016-11-11 12:45:40 UTC) #26
amalika
This CL is part of an experimental feature and has been already committed, but we ...
4 years ago (2016-12-01 15:32:03 UTC) #28
sof
4 years ago (2016-12-01 15:47:31 UTC) #29
Message was sent while issue was closed.
On 2016/12/01 15:32:03, amalika wrote:
> This CL is part of an experimental feature and has been already committed,
> but we wanted to clarify the usage of `HeapVector<Member<CSPSource>>`.
> 
> For example, `SourceListDirective::subsumes` function
> returns `HeapVector<Member<SourceListDirective>>`. There is no conversion
> `HeapVector<Member<CSPSource>>` to `HeapVector<CSPSource*>`, so we left it
> as it is. Is it an allowed usage of HeapVectors and Members?
> 
> Thank you!

Yes; I don't see the use of a return type like that here, but wrapping up
garbage collected types in a HeapVector with a Member<>, is indeed how to use
HeapVector<> -- static asserts will prevent the use of a bare pointer type like
CSPSource* in (Heap)Vector.

Wouldn't it be better if subsumes() had the signature

  bool subsumes(const HeapVector<Member<CSPSource>>& other) const;

?

Powered by Google App Engine
This is Rietveld 408576698