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

Issue 184523003: Fix two srcset related bugs (Closed)

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

Description

Fix two srcset related bugs Two problems with the current srcset parsing algorithm are fixed here: 1. When a set of descriptors contained only invalid descriptors, the resource wasn't ignored. 2. When more than 1 resource shared a single DPR value, the last one was taken into account, rather than the first one. I've also added test and fixed current ones to prevent this from regressing in the future. BUG=347998 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168241

Patch Set 1 #

Patch Set 2 : Removed a useless if #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -17 lines) Patch
M LayoutTests/fast/hidpi/image-srcset-invalid-descriptor.html View 1 chunk +4 lines, -2 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-invalid-descriptor-expected.txt View 1 chunk +3 lines, -2 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src.html View 1 chunk +1 line, -1 line 0 comments Download
A + LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both.html View 2 chunks +5 lines, -4 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-src-selection-1x-both-expected.txt View 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/html/parser/HTMLSrcsetParser.cpp View 1 3 chunks +16 lines, -8 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Yoav Weiss
Following the beta release I've got a bug report on twitter (https://twitter.com/benfosterdev/status/439365333372391424) with an example. ...
6 years, 9 months ago (2014-02-28 15:46:52 UTC) #1
abarth-chromium
LGTM Is this issue important enough to warrant merging to the beta branch?
6 years, 9 months ago (2014-03-01 06:08:51 UTC) #2
abarth-chromium
The CQ bit was checked by abarth@chromium.org
6 years, 9 months ago (2014-03-01 06:08:56 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/184523003/20001
6 years, 9 months ago (2014-03-01 06:09:11 UTC) #4
Yoav Weiss
On 2014/03/01 06:08:51, abarth wrote: > LGTM > > Is this issue important enough to ...
6 years, 9 months ago (2014-03-01 06:16:09 UTC) #5
abarth-chromium
On 2014/03/01 06:16:09, Yoav Weiss wrote: > I believe it is. Otherwise, people experimenting with ...
6 years, 9 months ago (2014-03-01 07:09:02 UTC) #6
commit-bot: I haz the power
6 years, 9 months ago (2014-03-01 08:47:40 UTC) #7
Message was sent while issue was closed.
Change committed as 168241

Powered by Google App Engine
This is Rietveld 408576698