Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(14)

Issue 23861003: Enable srcset support in HTMLImageElement (Closed)

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

Description

Enable srcset support in HTMLImageElement The `srcset` attribute enables authors to adapt image resources to the device's display characteristics. It is specified at http://www.whatwg.org/specs/web-apps/current-work/#attr-img-srcset This change is a port of WebKit's srcset implementation, comprised from patches by Romain Perier <romain.perier@gmail.com>;, Dean Jackson <dino@apple.com>; and myself. The implementation adds the srcset parsing algorithm to HTMLParserIdioms, adds preloading support to HTMLPreloadScanner and adds srcset support to HTMLImageElement. It implements only the 'x' qualifier of srcset, which handles DPR switching, and has rough consensus around it. BUG=284722 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158158

Patch Set 1 #

Total comments: 1

Patch Set 2 : Enable srcset support in HTMLImageElement #

Total comments: 3

Patch Set 3 : Enable srcset support in HTMLImageElement #

Patch Set 4 : Added a run-time flag, marking this feature as experimental #

Total comments: 7

Patch Set 5 : Fix issues raised in review #

Total comments: 21

Patch Set 6 : Addressed issues raised in review. Merged Romain Perier's new parser implementation. #

Total comments: 17

Patch Set 7 : Rewrote HTMLSrcsetParser, making it more efficient and readable. Addressed abarth's review comments. #

Total comments: 29

Patch Set 8 : Addressed latest review comments #

Total comments: 1

Patch Set 9 : Re-upload since binary files upload failed #

Patch Set 10 : Fixed a flaky test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1208 lines, -44 lines) Patch
A LayoutTests/fast/hidpi/dpr-setting.html View 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/dpr-setting-expected.txt View 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-1x.html View 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-1x-expected.txt View 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x.html View 1 chunk +33 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x-expected.txt View 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-data-src.html View 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-data-src-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-data-srcset.html View 1 2 3 4 5 1 chunk +20 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-data-srcset-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-dpr-zoom.html View 1 1 chunk +35 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-dpr-zoom-expected.txt View 1 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-fraction.html View 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-fraction-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-invalid-inputs.html View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src.html View 1 2 3 4 5 6 1 chunk +21 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-invalid-inputs-except-one.html View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-invalid-inputs-except-one-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-invalid-inputs-expected.txt View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-nomodifier.html View 1 2 3 4 5 1 chunk +20 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-nomodifier-expected.txt View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-only-src-attribute.html View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-only-src-attribute-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-remove-dynamically-from-js.html View 1 chunk +31 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-remove-dynamically-from-js-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-same-alternative-for-both-attributes.html View 1 chunk +25 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-same-alternative-for-both-attributes-expected.txt View 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-simple-1x.html View 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-simple-1x-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-simple-2x.html View 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-simple-2x-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-src-selection-1x.html View 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-src-selection-1x-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-src-selection-2x.html View 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-src-selection-2x-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-viewport-modifiers.html View 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-viewport-modifiers-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/resources/image-set-1x.png View Binary file 0 comments Download
A LayoutTests/fast/hidpi/resources/image-set-2x.png View Binary file 0 comments Download
A LayoutTests/fast/hidpi/resources/srcset-helper.js View 1 chunk +29 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/dont-preload-non-img-srcset.html View 1 2 3 4 5 6 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/dont-preload-non-img-srcset-expected.txt View 1 2 3 4 5 6 1 chunk +25 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-src.html View 1 2 3 4 5 6 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-src-expected.txt View 1 2 3 4 5 6 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset.html View 1 2 3 4 5 6 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-2x.html View 1 2 3 4 5 6 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-2x-expected.txt View 1 2 3 4 5 6 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-duplicate.html View 1 2 3 4 5 6 1 chunk +31 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-duplicate-expected.txt View 1 2 3 4 5 6 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-expected.txt View 1 2 3 4 5 6 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-reverse-order.html View 1 2 3 4 5 6 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-reverse-order-expected.txt View 1 2 3 4 5 6 1 chunk +16 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-src-preloaded.html View 1 2 3 4 5 6 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-src-preloaded-expected.txt View 1 2 3 4 5 6 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-src-preloaded-reverse-order.html View 1 2 3 4 5 6 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/preload-image-srcset-src-preloaded-reverse-order-expected.txt View 1 2 3 4 5 6 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/loading/resources/base-image1.png View 1 2 3 4 5 Binary file 0 comments Download
A LayoutTests/http/tests/loading/resources/base-image2.png View 1 2 3 4 5 6 Binary file 0 comments Download
A LayoutTests/http/tests/loading/resources/base-image3.png View 1 2 3 4 5 6 Binary file 0 comments Download
A LayoutTests/http/tests/loading/resources/dup-image1.png View 1 2 3 4 5 6 Binary file 0 comments Download
A LayoutTests/http/tests/loading/resources/dup-image2.png View 1 2 3 4 5 6 Binary file 0 comments Download
A LayoutTests/http/tests/loading/resources/dup-image3.png View 1 2 3 4 5 6 Binary file 0 comments Download
A + LayoutTests/http/tests/loading/resources/empty.html View 1 2 3 4 5 0 chunks +-1 lines, --1 lines 0 comments Download
A + LayoutTests/http/tests/loading/resources/empty.js View 1 2 3 4 5 0 chunks +-1 lines, --1 lines 0 comments Download
A + LayoutTests/http/tests/loading/resources/image1.png View 1 2 3 4 5 Binary file 0 comments Download
A + LayoutTests/http/tests/loading/resources/image2.png View 1 2 3 4 5 Binary file 0 comments Download
A LayoutTests/http/tests/loading/resources/preloaded.js View 1 2 3 4 5 6 1 chunk +20 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/resources/srcset-helper.js View 1 2 3 4 5 6 1 chunk +29 lines, -0 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/dom/Document.h View 1 2 3 4 5 6 7 1 chunk +6 lines, -4 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/html/HTMLAttributeNames.in View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLImageElement.h View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/html/HTMLImageElement.cpp View 1 2 3 4 5 6 3 chunks +11 lines, -1 line 0 comments Download
M Source/core/html/HTMLImageElement.idl View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/parser/HTMLDocumentParser.cpp View 1 2 3 4 5 6 7 3 chunks +3 lines, -3 lines 0 comments Download
M Source/core/html/parser/HTMLPreloadScanner.h View 1 2 3 4 5 6 7 3 chunks +3 lines, -2 lines 0 comments Download
M Source/core/html/parser/HTMLPreloadScanner.cpp View 1 2 3 4 5 6 7 8 chunks +40 lines, -12 lines 0 comments Download
A + Source/core/html/parser/HTMLSrcsetParser.h View 1 2 3 4 5 6 7 1 chunk +32 lines, -24 lines 0 comments Download
A Source/core/html/parser/HTMLSrcsetParser.cpp View 1 2 3 4 5 6 7 1 chunk +171 lines, -0 lines 0 comments Download
M Source/core/page/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/wtf/text/StringView.h View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 49 (0 generated)
Yoav Weiss
Following the recent discussion on Blink-dev, I've ported the srcset code from WebKit. (with some ...
5 years, 7 months ago (2013-09-03 20:34:53 UTC) #1
cbiesinger
On 2013/09/03 20:34:53, Yoav Weiss wrote: > Following the recent discussion on Blink-dev, I've ported ...
5 years, 7 months ago (2013-09-03 23:56:59 UTC) #2
johnme
https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageElement.cpp File Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageElement.cpp#newcode124 Source/core/html/HTMLImageElement.cpp:124: deviceScaleFactor = page->deviceScaleFactor(); As discussed on blink-dev, you probably ...
5 years, 7 months ago (2013-09-04 09:50:22 UTC) #3
Yoav Weiss
On 2013/09/04 09:50:22, johnme wrote: > https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageElement.cpp > File Source/core/html/HTMLImageElement.cpp (right): > > https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageElement.cpp#newcode124 > ...
5 years, 7 months ago (2013-09-05 00:30:57 UTC) #4
cbiesinger
https://codereview.chromium.org/23861003/diff/9001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/23861003/diff/9001/Source/core/dom/Document.h#newcode469 Source/core/dom/Document.h:469: Page* page() const; // can be null Why the ...
5 years, 7 months ago (2013-09-06 22:45:35 UTC) #5
Yoav Weiss
On 2013/09/06 22:45:35, cbiesinger wrote: > https://codereview.chromium.org/23861003/diff/9001/Source/core/dom/Document.h > File Source/core/dom/Document.h (right): > > https://codereview.chromium.org/23861003/diff/9001/Source/core/dom/Document.h#newcode469 > ...
5 years, 7 months ago (2013-09-07 08:07:40 UTC) #6
Yoav Weiss
> https://codereview.chromium.org/23861003/diff/9001/Source/core/dom/Document.h#newcode469 > > Source/core/dom/Document.h:469: Page* page() const; // can be null > > Why ...
5 years, 7 months ago (2013-09-07 09:45:57 UTC) #7
Yoav Weiss
Following the discussion on https://groups.google.com/a/chromium.org/d/msg/blink-dev/SVQxmsJRa9c/qfd55y_86HEJ, I've updated the CL to include a runtime flag.
5 years, 7 months ago (2013-09-10 06:46:06 UTC) #8
ojan
On 2013/09/10 06:46:06, Yoav Weiss wrote: > Following the discussion on > https://groups.google.com/a/chromium.org/d/msg/blink-dev/SVQxmsJRa9c/qfd55y_86HEJ, > I've ...
5 years, 7 months ago (2013-09-11 21:16:01 UTC) #9
do-not-use
https://codereview.chromium.org/23861003/diff/24001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/23861003/diff/24001/Source/core/dom/Document.cpp#newcode5322 Source/core/dom/Document.cpp:5322: float devicePixelRatio = 1.0; nit: return m_frame ? m_frame->devicePixelRatio() ...
5 years, 7 months ago (2013-09-12 08:23:53 UTC) #10
Yoav Weiss
https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImageElement.cpp File Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImageElement.cpp#newcode113 Source/core/html/HTMLImageElement.cpp:113: return m_bestFitImageURL.isEmpty() ? fastGetAttribute(srcAttr) : m_bestFitImageURL; On 2013/09/12 08:23:53, ...
5 years, 7 months ago (2013-09-12 09:30:20 UTC) #11
Yoav Weiss
On 2013/09/11 21:16:01, ojan wrote: > On 2013/09/10 06:46:06, Yoav Weiss wrote: > > Following ...
5 years, 7 months ago (2013-09-12 09:52:05 UTC) #12
do-not-use
https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImageElement.cpp File Source/core/html/HTMLImageElement.cpp (right): https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImageElement.cpp#newcode113 Source/core/html/HTMLImageElement.cpp:113: return m_bestFitImageURL.isEmpty() ? fastGetAttribute(srcAttr) : m_bestFitImageURL; On 2013/09/12 09:30:20, ...
5 years, 7 months ago (2013-09-12 09:56:37 UTC) #13
do-not-use
On 2013/09/12 09:56:37, Christophe Dumez wrote: > https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImageElement.cpp > File Source/core/html/HTMLImageElement.cpp (right): > > https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImageElement.cpp#newcode113 ...
5 years, 7 months ago (2013-09-12 09:57:28 UTC) #14
Yoav Weiss
> > > > To be clear the default constructor for String creates a NULL ...
5 years, 7 months ago (2013-09-12 17:52:34 UTC) #15
cbiesinger
On 2013/09/07 08:07:40, Yoav Weiss wrote: > https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HTMLParserIdioms.cpp#newcode343 > > Source/core/html/parser/HTMLParserIdioms.cpp:343: image.imageURL = > > ...
5 years, 7 months ago (2013-09-12 19:31:19 UTC) #16
ojan
This mostly looks good. I'm not too familiar with the html/parser code though. Adam or ...
5 years, 7 months ago (2013-09-12 23:26:37 UTC) #17
abarth-chromium
not lgtm This looks like a good start, but there are a number of issues ...
5 years, 7 months ago (2013-09-13 05:54:06 UTC) #18
Yoav Weiss
On 2013/09/12 19:31:19, cbiesinger wrote: > On 2013/09/07 08:07:40, Yoav Weiss wrote: > > > ...
5 years, 7 months ago (2013-09-13 06:17:23 UTC) #19
Yoav Weiss
On 2013/09/13 05:54:06, abarth wrote: > not lgtm > > This looks like a good ...
5 years, 7 months ago (2013-09-13 06:18:32 UTC) #20
Yoav Weiss
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/HTMLParserIdioms.cpp File Source/core/html/parser/HTMLParserIdioms.cpp (right): https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/HTMLParserIdioms.cpp#newcode315 Source/core/html/parser/HTMLParserIdioms.cpp:315: String bestFitSourceForImageAttributes(float deviceScaleFactor, const String& srcAttribute, const String& srcSetAttribute) ...
5 years, 7 months ago (2013-09-13 07:50:58 UTC) #21
Yoav Weiss
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/HTMLPreloadScanner.cpp File Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/HTMLPreloadScanner.cpp#newcode126 Source/core/html/parser/HTMLPreloadScanner.cpp:126: applySrcset(); On 2013/09/13 05:54:07, abarth wrote: > Instead of ...
5 years, 7 months ago (2013-09-13 10:04:18 UTC) #22
cbiesinger
On 2013/09/13 06:17:23, Yoav Weiss wrote: > On 2013/09/12 19:31:19, cbiesinger wrote: > > On ...
5 years, 7 months ago (2013-09-13 16:06:29 UTC) #23
Yoav Weiss
You're right, of course. I've fixed it locally by incorporating the newer parsing algorithm from ...
5 years, 7 months ago (2013-09-13 17:44:01 UTC) #24
abarth-chromium
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/HTMLParserIdioms.cpp File Source/core/html/parser/HTMLParserIdioms.cpp (right): https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/HTMLParserIdioms.cpp#newcode315 Source/core/html/parser/HTMLParserIdioms.cpp:315: String bestFitSourceForImageAttributes(float deviceScaleFactor, const String& srcAttribute, const String& srcSetAttribute) ...
5 years, 7 months ago (2013-09-13 17:58:12 UTC) #25
Yoav Weiss
I've addressed most issues raised in the review. Merging in large parts of Romain Perier's ...
5 years, 7 months ago (2013-09-13 21:45:15 UTC) #26
abarth-chromium
https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/HTMLParserIdioms.cpp File Source/core/html/parser/HTMLParserIdioms.cpp (right): https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/HTMLParserIdioms.cpp#newcode299 Source/core/html/parser/HTMLParserIdioms.cpp:299: There's no need for this blank line. https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/HTMLPreloadScanner.cpp File ...
5 years, 7 months ago (2013-09-13 22:24:05 UTC) #27
Yoav Weiss
> > https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/HTMLPreloadScanner.cpp#newcode129 > Source/core/html/parser/HTMLPreloadScanner.cpp:129: applySrcset(); > As I wrote before, we don't want to ...
5 years, 7 months ago (2013-09-14 11:46:20 UTC) #28
abarth-chromium
On 2013/09/14 11:46:20, Yoav Weiss wrote: > Assuming that the m_srcSetAttribute is only set when ...
5 years, 7 months ago (2013-09-14 14:47:17 UTC) #29
Yoav Weiss
On 2013/09/14 14:47:17, abarth wrote: > On 2013/09/14 11:46:20, Yoav Weiss wrote: > > Assuming ...
5 years, 7 months ago (2013-09-14 14:54:37 UTC) #30
Yoav Weiss
> > https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/HTMLSrcsetParser.cpp#newcode124 > Source/core/html/parser/HTMLSrcsetParser.cpp:124: } > This parsing algorithm is too hard to read. ...
5 years, 7 months ago (2013-09-15 13:29:18 UTC) #31
abarth-chromium
On 2013/09/15 13:29:18, Yoav Weiss wrote: > Sounds good. Would you consider HTMLParserIdioms a good ...
5 years, 7 months ago (2013-09-15 15:28:44 UTC) #32
Yoav Weiss
I've uploaded a new patch that addresses the previous review comments: * Rebased patch * ...
5 years, 7 months ago (2013-09-18 09:38:50 UTC) #33
abarth-chromium
This looks much better. We mostly just need to clean up the license blocks and ...
5 years, 7 months ago (2013-09-19 18:09:51 UTC) #34
cbiesinger
https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLSrcsetParser.cpp File Source/core/html/parser/HTMLSrcsetParser.cpp (right): https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLSrcsetParser.cpp#newcode113 Source/core/html/parser/HTMLSrcsetParser.cpp:113: return srcAttribute; On 2013/09/19 18:09:52, abarth wrote: > Or ...
5 years, 7 months ago (2013-09-19 23:01:43 UTC) #35
Yoav Weiss
https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLParserIdioms.h File Source/core/html/parser/HTMLParserIdioms.h (right): https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLParserIdioms.h#newcode105 Source/core/html/parser/HTMLParserIdioms.h:105: } On 2013/09/19 18:09:52, abarth wrote: > Why do ...
5 years, 7 months ago (2013-09-20 04:54:26 UTC) #36
Yoav Weiss
https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLSrcsetParser.cpp File Source/core/html/parser/HTMLSrcsetParser.cpp (right): https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLSrcsetParser.cpp#newcode40 Source/core/html/parser/HTMLSrcsetParser.cpp:40: skipWhile<CharType, isNotHTMLSpace<CharType> >(position, attributeEnd); On 2013/09/19 18:09:52, abarth wrote: ...
5 years, 7 months ago (2013-09-20 07:31:35 UTC) #37
abarth-chromium
https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLParserIdioms.h File Source/core/html/parser/HTMLParserIdioms.h (right): https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLParserIdioms.h#newcode105 Source/core/html/parser/HTMLParserIdioms.h:105: } On 2013/09/20 04:54:27, Yoav Weiss wrote: > On ...
5 years, 7 months ago (2013-09-20 16:08:06 UTC) #38
abarth-chromium
https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLSrcsetParser.cpp File Source/core/html/parser/HTMLSrcsetParser.cpp (right): https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/HTMLSrcsetParser.cpp#newcode113 Source/core/html/parser/HTMLSrcsetParser.cpp:113: return srcAttribute; On 2013/09/20 16:08:07, abarth wrote: > On ...
5 years, 7 months ago (2013-09-20 16:08:57 UTC) #39
Yoav Weiss
> > https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/ParsingUtilities.h > File Source/core/platform/ParsingUtilities.h (right): > > https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/ParsingUtilities.h#newcode18 > Source/core/platform/ParsingUtilities.h:18: template<typename CharType, bool ...
5 years, 7 months ago (2013-09-20 21:00:10 UTC) #40
abarth-chromium
lgtm https://codereview.chromium.org/23861003/diff/80001/Source/wtf/text/StringView.h File Source/wtf/text/StringView.h (right): https://codereview.chromium.org/23861003/diff/80001/Source/wtf/text/StringView.h#newcode95 Source/wtf/text/StringView.h:95: return StringImpl::create(characters16(), m_length); We should probably add an ...
5 years, 7 months ago (2013-09-20 21:13:33 UTC) #41
abarth-chromium
Thanks for iterating on this CL. We can optimize StringView::toString in a followup CL.
5 years, 7 months ago (2013-09-20 21:14:12 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/23861003/80001
5 years, 7 months ago (2013-09-20 21:14:33 UTC) #43
commit-bot: I haz the power
Can't process patch for file LayoutTests/http/tests/loading/resources/base-image1.png. Binary file is empty. Maybe the file wasn't uploaded ...
5 years, 7 months ago (2013-09-20 21:14:57 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/23861003/91001
5 years, 7 months ago (2013-09-20 21:36:31 UTC) #45
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=5505
5 years, 7 months ago (2013-09-20 23:47:26 UTC) #46
Yoav Weiss
On 2013/09/20 23:47:26, I haz the power (commit-bot) wrote: > Retried try job too often ...
5 years, 7 months ago (2013-09-21 03:51:35 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/23861003/109001
5 years, 7 months ago (2013-09-21 03:52:08 UTC) #48
commit-bot: I haz the power
5 years, 7 months ago (2013-09-21 06:13:53 UTC) #49
Message was sent while issue was closed.
Change committed as 158158

Powered by Google App Engine
This is Rietveld 408576698