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

Issue 889883002: Add a parser for Link headers (Closed)

Created:
5 years, 10 months ago by Yoav Weiss
Modified:
5 years, 10 months ago
Reviewers:
pmeenan, Mike West
CC:
blink-reviews, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Add a parser for Link headers This CL adds a parser for Link headers, that would enable to support them for various features that can benefit from such support. Resource Hints (and the non-standard hints: dns-prefetch, prefetch, etc) are likely to be the first users. BUG=58456 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189352

Patch Set 1 #

Patch Set 2 : Self review nits #

Patch Set 3 : Handle null headers #

Total comments: 3

Patch Set 4 : Refactored, added tests and fixed a bug #

Patch Set 5 : Self review + added expectations #

Total comments: 12

Patch Set 6 : review comments #

Patch Set 7 : Removed CharType #

Total comments: 3

Patch Set 8 : proper expected files. Less polluting tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+337 lines, -1 line) Patch
A + LayoutTests/http/tests/linkHeader/link-header-no-crash-empty.php View 1 2 3 4 5 6 7 1 chunk +4 lines, -1 line 0 comments Download
A LayoutTests/http/tests/linkHeader/link-header-no-crash-empty-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/http/tests/linkHeader/link-header-no-crash-space-in-url.php View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/linkHeader/link-header-no-crash-space-in-url-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/http/tests/linkHeader/link-header-no-crash-space-invalid.php View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/linkHeader/link-header-no-crash-space-invalid-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A LayoutTests/http/tests/linkHeader/link-header-no-crash-valid.php View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/linkHeader/link-header-no-crash-valid-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/core.gypi View 2 chunks +2 lines, -0 lines 0 comments Download
A Source/core/loader/LinkHeader.h View 1 2 3 4 5 1 chunk +37 lines, -0 lines 0 comments Download
A Source/core/loader/LinkHeader.cpp View 1 2 3 4 5 6 1 chunk +198 lines, -0 lines 0 comments Download
A Source/core/loader/LinkHeaderTest.cpp View 1 2 3 1 chunk +65 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (3 generated)
Yoav Weiss
This adds a parser for link headers. It has no users yet, but it is ...
5 years, 10 months ago (2015-01-30 11:24:15 UTC) #3
Mike West
https://codereview.chromium.org/889883002/diff/40001/Source/core/loader/LinkHeader.cpp File Source/core/loader/LinkHeader.cpp (right): https://codereview.chromium.org/889883002/diff/40001/Source/core/loader/LinkHeader.cpp#newcode19 Source/core/loader/LinkHeader.cpp:19: template < typename CharType> Nit: No space after '<'. ...
5 years, 10 months ago (2015-01-30 13:55:13 UTC) #4
Yoav Weiss
On 2015/01/30 13:55:13, Mike West wrote: > https://codereview.chromium.org/889883002/diff/40001/Source/core/loader/LinkHeader.cpp > File Source/core/loader/LinkHeader.cpp (right): > > https://codereview.chromium.org/889883002/diff/40001/Source/core/loader/LinkHeader.cpp#newcode19 ...
5 years, 10 months ago (2015-02-02 12:53:11 UTC) #5
Mike West
Thanks! This is _significantly_ easier to review now... I appreciate it, and I have comments! ...
5 years, 10 months ago (2015-02-02 13:14:54 UTC) #6
Yoav Weiss
On 2015/02/02 13:14:54, Mike West wrote: > https://codereview.chromium.org/889883002/diff/80001/Source/core/loader/LinkHeader.cpp#newcode79 > Source/core/loader/LinkHeader.cpp:79: isValidDelimiter = false; > You ...
5 years, 10 months ago (2015-02-02 13:38:09 UTC) #7
Mike West
On 2015/02/02 13:38:09, Yoav Weiss wrote: > On 2015/02/02 13:14:54, Mike West wrote: > > ...
5 years, 10 months ago (2015-02-02 14:35:16 UTC) #8
Yoav Weiss
OK, uploaded a new patch that covers all the comments. (and you're right, it's much ...
5 years, 10 months ago (2015-02-02 15:46:46 UTC) #9
Mike West
This is a lot nicer! Thanks for taking another pass. LGTM. https://codereview.chromium.org/889883002/diff/120001/Source/core/loader/LinkHeader.cpp File Source/core/loader/LinkHeader.cpp (right): ...
5 years, 10 months ago (2015-02-02 15:56:30 UTC) #10
Yoav Weiss
On 2015/02/02 15:56:30, Mike West wrote: > This is a lot nicer! Thanks for taking ...
5 years, 10 months ago (2015-02-02 16:17:00 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/889883002/140001
5 years, 10 months ago (2015-02-02 17:21:23 UTC) #13
commit-bot: I haz the power
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189352
5 years, 10 months ago (2015-02-02 18:25:29 UTC) #14
perezju
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/876633009/ by perezju@chromium.org. ...
5 years, 10 months ago (2015-02-03 14:58:57 UTC) #15
Yoav Weiss
5 years, 10 months ago (2015-02-03 17:02:02 UTC) #16
Message was sent while issue was closed.
On 2015/02/03 14:58:57, perezju wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/876633009/ by mailto:perezju@chromium.org.
> 
> The reason for reverting is: A memory leak was detected by the asan tool..

Thanks for reverting! I'm able to recreate the issue, understand it, and will
fix shortly.

Powered by Google App Engine
This is Rietveld 408576698