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

Issue 1186333004: Add support for `width` based Width Client Hint (Closed)

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

Description

Add support for `width` based Width Client Hint In order to increase the cases in which we can send an accurate `Width` hint, this CL sends them when `sizes` is absent, but `width` is present (when `width`'s value is not '%' based) BUG=458983 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197374

Patch Set 1 #

Patch Set 2 : extra test case #

Patch Set 3 : Fixed tests #

Total comments: 22

Patch Set 4 : Review comments #

Total comments: 1

Patch Set 5 : Moar comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+128 lines, -20 lines) Patch
M LayoutTests/http/tests/misc/client-hints-accept.php View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/http/tests/misc/client-hints-accept-meta.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/http/tests/misc/client-hints-accept-meta-preloader.html View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
A + LayoutTests/http/tests/misc/client-hints-rw-width.html View 1 2 3 1 chunk +8 lines, -4 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLImageElement.h View 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/html/HTMLImageElement.cpp View 1 2 3 4 1 chunk +37 lines, -4 lines 0 comments Download
A Source/core/html/HTMLImageElementTest.cpp View 1 2 3 4 1 chunk +45 lines, -0 lines 0 comments Download
M Source/core/html/parser/HTMLPreloadScanner.cpp View 5 chunks +14 lines, -0 lines 0 comments Download
M Source/core/html/parser/HTMLPreloadScannerTest.cpp View 1 2 chunks +14 lines, -4 lines 0 comments Download
M Source/core/loader/ImageLoader.cpp View 1 chunk +2 lines, -6 lines 0 comments Download

Messages

Total messages: 18 (4 generated)
Yoav Weiss
Yo, Mike :) Can you take a look?
5 years, 6 months ago (2015-06-17 12:29:50 UTC) #2
igrigorik
https://codereview.chromium.org/1186333004/diff/40001/Source/core/html/HTMLImageElement.cpp File Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/1186333004/diff/40001/Source/core/html/HTMLImageElement.cpp#newcode405 Source/core/html/HTMLImageElement.cpp:405: // TODO(yoav): That seems buggy when width is provided ...
5 years, 6 months ago (2015-06-17 17:26:28 UTC) #4
Yoav Weiss
On 2015/06/17 17:26:28, igrigorik wrote: > https://codereview.chromium.org/1186333004/diff/40001/Source/core/html/HTMLImageElement.cpp > File Source/core/html/HTMLImageElement.cpp (right): > > https://codereview.chromium.org/1186333004/diff/40001/Source/core/html/HTMLImageElement.cpp#newcode405 > ...
5 years, 6 months ago (2015-06-17 17:28:21 UTC) #5
igrigorik
On 2015/06/17 17:28:21, Yoav Weiss wrote: > True, but % values are supported, even if ...
5 years, 6 months ago (2015-06-17 21:07:36 UTC) #6
Mike West
A few comments. I don't really understand the layout test changes... This is probably because ...
5 years, 6 months ago (2015-06-18 06:20:04 UTC) #7
Yoav Weiss
Thanks for reviewing! :) Answers below https://codereview.chromium.org/1186333004/diff/40001/LayoutTests/http/tests/misc/client-hints-accept-meta.html File LayoutTests/http/tests/misc/client-hints-accept-meta.html (right): https://codereview.chromium.org/1186333004/diff/40001/LayoutTests/http/tests/misc/client-hints-accept-meta.html#newcode15 LayoutTests/http/tests/misc/client-hints-accept-meta.html:15: img.sizes = '100vw'; ...
5 years, 6 months ago (2015-06-18 06:30:22 UTC) #8
Mike West
https://codereview.chromium.org/1186333004/diff/40001/LayoutTests/http/tests/misc/client-hints-accept.php File LayoutTests/http/tests/misc/client-hints-accept.php (right): https://codereview.chromium.org/1186333004/diff/40001/LayoutTests/http/tests/misc/client-hints-accept.php#newcode17 LayoutTests/http/tests/misc/client-hints-accept.php:17: img.width = '500'; On 2015/06/18 at 06:30:21, Yoav Weiss ...
5 years, 6 months ago (2015-06-18 06:34:35 UTC) #9
Yoav Weiss
> On 2015/06/18 at 06:30:22, Yoav Weiss wrote: > > On 2015/06/18 06:20:04, Mike West ...
5 years, 6 months ago (2015-06-18 06:43:00 UTC) #10
Mike West
On 2015/06/18 at 06:43:00, yoav wrote: > > On 2015/06/18 at 06:30:22, Yoav Weiss wrote: ...
5 years, 6 months ago (2015-06-18 06:57:35 UTC) #11
Yoav Weiss
https://codereview.chromium.org/1186333004/diff/40001/Source/core/html/HTMLImageElement.cpp File Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/1186333004/diff/40001/Source/core/html/HTMLImageElement.cpp#newcode405 Source/core/html/HTMLImageElement.cpp:405: // TODO(yoav): That seems buggy when width is provided ...
5 years, 6 months ago (2015-06-18 09:19:28 UTC) #12
Yoav Weiss
On 2015/06/18 06:57:35, Mike West wrote: > On 2015/06/18 at 06:43:00, yoav wrote: > > ...
5 years, 6 months ago (2015-06-18 13:21:40 UTC) #13
Mike West
LGTM yay. https://codereview.chromium.org/1186333004/diff/60001/Source/core/html/HTMLImageElementTest.cpp File Source/core/html/HTMLImageElementTest.cpp (right): https://codereview.chromium.org/1186333004/diff/60001/Source/core/html/HTMLImageElementTest.cpp#newcode31 Source/core/html/HTMLImageElementTest.cpp:31: EXPECT_EQ(500, image->resourceWidth().width); Maybe make this a const ...
5 years, 6 months ago (2015-06-18 14:44:49 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1186333004/80001
5 years, 6 months ago (2015-06-18 16:33:05 UTC) #17
commit-bot: I haz the power
5 years, 6 months ago (2015-06-18 17:10:12 UTC) #18
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197374

Powered by Google App Engine
This is Rietveld 408576698