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

Issue 236713005: Use SizesAttributeParser to get the right srcset resource (Closed)

Created:
6 years, 8 months ago by Yoav Weiss
Modified:
6 years, 8 months ago
Reviewers:
eseidel
CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@sizes_parser8
Visibility:
Public.

Description

Use SizesAttributeParser to get the right srcset resource This CL connects the sizes parser to the extended srcset parser and makes sure the images are properly fetched and displayed. BUG=357586 Depends on http://crrev.com/242103008 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172004

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Added some tests and fixed a bug #

Patch Set 4 : Added and unified tests #

Patch Set 5 : Test fixups #

Patch Set 6 : Added curretSrc. Added some tests #

Patch Set 7 : rebase #

Patch Set 8 : Fixed layout tests and many bugs #

Patch Set 9 : Resolved images with no frame issues #

Patch Set 10 : Rebase #

Total comments: 4

Patch Set 11 : Fixed review bugs #

Patch Set 12 : Modified tests to point towards the hidpi folder #

Patch Set 13 : Added preloder and JS tests. Fixed bugs; #

Total comments: 10

Patch Set 14 : Fixed review comments #

Total comments: 2

Patch Set 15 : Fixed nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+504 lines, -69 lines) Patch
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +55 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +28 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-2x.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +59 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-2x-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +28 lines, -0 lines 0 comments Download
D LayoutTests/fast/dom/HTMLImageElement/image-sizes-invalid-length.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -14 lines 0 comments Download
D LayoutTests/fast/dom/HTMLImageElement/image-sizes-invalid-length-expected.txt View 1 2 3 4 5 6 7 1 chunk +0 lines, -4 lines 0 comments Download
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-js-change.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +32 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-js-change-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +7 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-js-innerhtml.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/fast/dom/HTMLImageElement/image-sizes-js-innerhtml-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
D LayoutTests/fast/dom/HTMLImageElement/image-sizes-simple.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -14 lines 0 comments Download
D LayoutTests/fast/dom/HTMLImageElement/image-sizes-simple-expected.txt View 1 2 3 4 5 6 7 1 chunk +0 lines, -4 lines 0 comments Download
A + LayoutTests/fast/dom/HTMLImageElement/resources/image-set-1x.png View 1 2 3 4 5 6 7 8 9 10 11 Binary file 0 comments Download
A + LayoutTests/fast/dom/HTMLImageElement/resources/image-set-2x.png View 1 2 3 4 5 6 7 8 9 10 11 Binary file 0 comments Download
A LayoutTests/http/tests/loading/preload-image-sizes.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +47 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-sizes-2x.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +51 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-sizes-2x-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +59 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-sizes-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +54 lines, -0 lines 0 comments Download
M Source/core/css/parser/MediaQueryTokenizer.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/css/parser/SizesAttributeParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +6 lines, -8 lines 0 comments Download
M Source/core/css/parser/SizesAttributeParserTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/html/HTMLImageElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +1 line, -1 line 0 comments Download
M Source/core/html/HTMLImageElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +10 lines, -5 lines 0 comments Download
M Source/core/html/HTMLImageElement.idl View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/parser/HTMLPreloadScanner.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +22 lines, -8 lines 0 comments Download
M Source/core/html/parser/HTMLSrcsetParser.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -3 lines 0 comments Download
M Source/core/html/parser/HTMLSrcsetParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +8 lines, -8 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
Yoav Weiss
6 years, 8 months ago (2014-04-17 12:51:38 UTC) #1
Yoav Weiss
I still need to add PreloadScanner layout tests and look into the layout test crashes, ...
6 years, 8 months ago (2014-04-17 15:07:19 UTC) #2
Yoav Weiss
On 2014/04/17 15:07:19, Yoav Weiss wrote: > I still need to add PreloadScanner layout tests ...
6 years, 8 months ago (2014-04-18 08:12:47 UTC) #3
eseidel
I think we need more testing per the bugs below: https://codereview.chromium.org/236713005/diff/160001/Source/core/css/parser/MediaQueryTokenizer.cpp File Source/core/css/parser/MediaQueryTokenizer.cpp (right): https://codereview.chromium.org/236713005/diff/160001/Source/core/css/parser/MediaQueryTokenizer.cpp#newcode216 ...
6 years, 8 months ago (2014-04-19 04:18:46 UTC) #4
Yoav Weiss
On 2014/04/19 04:18:46, eseidel wrote: > I think we need more testing per the bugs ...
6 years, 8 months ago (2014-04-19 04:46:47 UTC) #5
eseidel
https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html File LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html (right): https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html#newcode9 LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html:9: shouldBeTrue('document.getElementById("simple").clientWidth==(window.innerWidth)'); You wan "shouldBe()" then it will print nice ...
6 years, 8 months ago (2014-04-19 04:54:19 UTC) #6
Yoav Weiss
On 2014/04/19 04:54:19, eseidel wrote: > https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html > File LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html (right): > > https://codereview.chromium.org/236713005/diff/160001/LayoutTests/fast/dom/HTMLImageElement/image-sizes-1x.html#newcode9 > ...
6 years, 8 months ago (2014-04-20 20:57:46 UTC) #7
eseidel
https://codereview.chromium.org/236713005/diff/220001/Source/core/css/MediaValuesDynamic.h File Source/core/css/MediaValuesDynamic.h (left): https://codereview.chromium.org/236713005/diff/220001/Source/core/css/MediaValuesDynamic.h#oldcode36 Source/core/css/MediaValuesDynamic.h:36: Spurious. https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/MediaQueryTokenizer.cpp File Source/core/css/parser/MediaQueryTokenizer.cpp (right): https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/MediaQueryTokenizer.cpp#newcode216 Source/core/css/parser/MediaQueryTokenizer.cpp:216: if (string.isNull() ...
6 years, 8 months ago (2014-04-20 21:27:43 UTC) #8
Yoav Weiss
https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/SizesAttributeParserTest.cpp File Source/core/css/parser/SizesAttributeParserTest.cpp (right): https://codereview.chromium.org/236713005/diff/220001/Source/core/css/parser/SizesAttributeParserTest.cpp#newcode49 Source/core/css/parser/SizesAttributeParserTest.cpp:49: {"50vw", 250}, On 2014/04/20 21:27:43, eseidel wrote: > Is ...
6 years, 8 months ago (2014-04-20 21:48:02 UTC) #9
eseidel
lgtm https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/SizesAttributeParser.cpp File Source/core/css/parser/SizesAttributeParser.cpp (left): https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/SizesAttributeParser.cpp#oldcode30 Source/core/css/parser/SizesAttributeParser.cpp:30: if (!CSSPrimitiveValue::isLength(startToken->unitType())) I think this previous early-return based ...
6 years, 8 months ago (2014-04-20 22:06:20 UTC) #10
Yoav Weiss
On 2014/04/20 22:06:20, eseidel wrote: > lgtm > > https://codereview.chromium.org/236713005/diff/240001/Source/core/css/parser/SizesAttributeParser.cpp > File Source/core/css/parser/SizesAttributeParser.cpp (left): > ...
6 years, 8 months ago (2014-04-20 22:16:48 UTC) #11
Yoav Weiss
The CQ bit was checked by yoav@yoav.ws
6 years, 8 months ago (2014-04-20 22:17:15 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/236713005/260001
6 years, 8 months ago (2014-04-20 22:17:23 UTC) #13
commit-bot: I haz the power
6 years, 8 months ago (2014-04-21 01:13:08 UTC) #14
Message was sent while issue was closed.
Change committed as 172004

Powered by Google App Engine
This is Rietveld 408576698