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

Issue 2456493002: Splitting *Matches functions in CSPSource (Closed)

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

Description

Change to String-based CSPSource matching functions. Currently, CSPSource's matching functions match the relevant component against a KURL. We need to match against Strings in order to implement some new features, so this patch converts the comparisons in preparation for that work. BUG=647588 Committed: https://crrev.com/8b7fe99a65e3478fed6f8a4a6f06213b18147c93 Cr-Commit-Position: refs/heads/master@{#427997}

Patch Set 1 : Removing equalIgnoringCase #

Total comments: 3

Patch Set 2 : Removing KURL equivalents #

Total comments: 1

Patch Set 3 : Removing KURL equivalents #

Total comments: 2

Patch Set 4 : Adding DCHECKs for lower case on protocol/host #

Total comments: 2

Patch Set 5 : Adding a comment #

Total comments: 1

Patch Set 6 : Removing lower and dcheck for host #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -31 lines) Patch
M third_party/WebKit/Source/core/frame/csp/CSPSource.h View 1 2 3 4 1 chunk +5 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/frame/csp/CSPSource.cpp View 1 2 3 4 5 4 chunks +23 lines, -27 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 33 (18 generated)
amalika
https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode65 third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:65: bool equalHosts = equalIgnoringCase(host, m_host); Can we assume hosts ...
4 years, 1 month ago (2016-10-26 12:18:08 UTC) #3
Mike West
On 2016/10/26 at 12:18:08, amalika wrote: > https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp > File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): > > https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode65 ...
4 years, 1 month ago (2016-10-26 12:20:59 UTC) #6
Mike West
Thanks! https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/20001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode24 third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:24: m_scheme(scheme), I'm actually wrong about parsing here: we ...
4 years, 1 month ago (2016-10-26 12:25:16 UTC) #7
Mike West
Also: please change the CL description to fit the general pattern: ``` Title of ~60 ...
4 years, 1 month ago (2016-10-26 12:28:17 UTC) #8
amalika
https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode58 third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:58: bool equalHosts = (m_host == host.lower()); it seems like ...
4 years, 1 month ago (2016-10-26 13:03:33 UTC) #13
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/2456493002/60001
4 years, 1 month ago (2016-10-26 13:03:49 UTC) #14
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
4 years, 1 month ago (2016-10-26 13:03:50 UTC) #16
Mike West
On 2016/10/26 at 13:03:33, amalika wrote: > https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp > File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): > > https://codereview.chromium.org/2456493002/diff/40001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode58 ...
4 years, 1 month ago (2016-10-26 13:57:45 UTC) #19
Mike West
https://codereview.chromium.org/2456493002/diff/60001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/60001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode45 third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:45: bool CSPSource::schemeMatches(const String& protocol) const { On 2016/10/26 at ...
4 years, 1 month ago (2016-10-26 13:57:48 UTC) #20
amalika
Dry run passed! https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode55 third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:55: DCHECK_EQ(host, host.lower()); Actually, I conducted an ...
4 years, 1 month ago (2016-10-26 16:27:40 UTC) #24
Mike West
https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Source/core/frame/csp/CSPSource.h File third_party/WebKit/Source/core/frame/csp/CSPSource.h (right): https://codereview.chromium.org/2456493002/diff/80001/third_party/WebKit/Source/core/frame/csp/CSPSource.h#newcode41 third_party/WebKit/Source/core/frame/csp/CSPSource.h:41: bool portMatches(int, const String&) const; Nit: Can you add ...
4 years, 1 month ago (2016-10-26 16:29:23 UTC) #26
Mike West
+csharrison@, as https://codereview.chromium.org/2447293002 is relevant here. :) https://codereview.chromium.org/2456493002/diff/100001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp File third_party/WebKit/Source/core/frame/csp/CSPSource.cpp (right): https://codereview.chromium.org/2456493002/diff/100001/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp#newcode55 third_party/WebKit/Source/core/frame/csp/CSPSource.cpp:55: DCHECK_EQ(host, host.lower()); ...
4 years, 1 month ago (2016-10-27 07:45:59 UTC) #27
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/2456493002/120001
4 years, 1 month ago (2016-10-27 09:30:09 UTC) #30
commit-bot: I haz the power
Committed patchset #6 (id:120001)
4 years, 1 month ago (2016-10-27 11:30:12 UTC) #31
commit-bot: I haz the power
4 years, 1 month ago (2016-10-27 11:32:06 UTC) #33
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/8b7fe99a65e3478fed6f8a4a6f06213b18147c93
Cr-Commit-Position: refs/heads/master@{#427997}

Powered by Google App Engine
This is Rietveld 408576698