Enable srcset support in HTMLImageElement

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

[Yoav Weiss, 2013-09-03 20:34:53]

Following the recent discussion on Blink-dev, I've ported the srcset code from
WebKit. (with some fixes and an additional test)

[cbiesinger, 2013-09-03 23:56:59]

On 2013/09/03 20:34:53, Yoav Weiss wrote:
> Following the recent discussion on Blink-dev, I've ported the srcset code from
> WebKit. (with some fixes and an additional test)

Thanks for doing this work!

I think this includes both http://trac.webkit.org/changeset/153624 and
http://trac.webkit.org/changeset/153733, is that correct?

I'll take a closer look at the patch tomorrow, but I'm not an actual Blink
reviewer, so someone else will have to give the actual LGTM.

[johnme, 2013-09-04 09:50:22]

https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageEle...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageEle...
Source/core/html/HTMLImageElement.cpp:124: deviceScaleFactor =
page->deviceScaleFactor();
As discussed on blink-dev, you probably want:

float devicePixelRatio = 1.0;
if (Frame* frame = document()->frame())
    devicePixelRatio = frame->devicePixelRatio();

instead.

See https://codereview.chromium.org/23000005 and
https://codereview.chromium.org/23192002 for the context behind these.

[Yoav Weiss, 2013-09-05 00:30:57]

On 2013/09/04 09:50:22, johnme wrote:
>
https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageEle...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/23861003/diff/1/Source/core/html/HTMLImageEle...
> Source/core/html/HTMLImageElement.cpp:124: deviceScaleFactor =
> page->deviceScaleFactor();
> As discussed on blink-dev, you probably want:
> 
> float devicePixelRatio = 1.0;
> if (Frame* frame = document()->frame())
>     devicePixelRatio = frame->devicePixelRatio();
> 
> instead.
> 
> See https://codereview.chromium.org/23000005 and
> https://codereview.chromium.org/23192002 for the context behind these.

Fixed by adding a devicePixelRatio() method to Document, and using that in all 3
places the DPR is needed (HTMLImageElement and HTMLDocumentParser).
Added a test to enforce that behavior.

[cbiesinger, 2013-09-06 22:45:35]

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...
Source/core/dom/Document.h:469: Page* page() const; // can be null
Why the various changes up here?

https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
Source/core/html/parser/HTMLParserIdioms.cpp:343: image.imageURL =
decodeURLEscapeSequences(data[0]);
why decode them?

https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
Source/core/html/parser/HTMLParserIdioms.cpp:352: image.imageURL =
decodeURLEscapeSequences(src);
same

[Yoav Weiss, 2013-09-07 08:07:40]

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...
> Source/core/dom/Document.h:469: Page* page() const; // can be null
> Why the various changes up here?

Since I changed something in Document.h, the style check errored on the "NULL"s,
insisting that they'd be changed to "null".
The reordering of function declaration order wasn't something I did on purpose.
I can reverse it if it's a problem.

> 
>
https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
> File Source/core/html/parser/HTMLParserIdioms.cpp (right):
> 
>
https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
> Source/core/html/parser/HTMLParserIdioms.cpp:343: image.imageURL =
> decodeURLEscapeSequences(data[0]);
> why decode them?

The reasoning is at:
https://bugs.webkit.org/attachment.cgi?id=207963&action=review
To quote the review there:
"""
Darin Adler:
This seems like a strange thing to do. Why would we decode URL escape sequences
here? Is there a test case that demonstrates the benefit of this?
Dean Jackson:
Yes, it's the test case fast/hidpi/image-srcset-data-srcset.html
Since srcset requires comma separated values, a base64 encoded url breaks the
parsing unless it is also % encoded (which is a pain that I've reported to Ted
to see if it can be avoided in the spec).
"""

Looking the comment on the implementation of decodeURLEscapeSequences at
weborigin/KURL.cpp (and the various FIXMEs there), there might be better ways to
achieve the same functionality in Blink.
Is it better to use
core/platform/text/DecodeEscapSequences.h:decodeEscapeSequences ? A comment on
XSSAuditor.cpp indicates so.

> 
>
https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
> Source/core/html/parser/HTMLParserIdioms.cpp:352: image.imageURL =
> decodeURLEscapeSequences(src);
> same

[Yoav Weiss, 2013-09-07 09:45:57]

>
https://codereview.chromium.org/23861003/diff/9001/Source/core/dom/Document.h...
> > Source/core/dom/Document.h:469: Page* page() const; // can be null
> > Why the various changes up here?
> 
> Since I changed something in Document.h, the style check errored on the
"NULL"s,
> insisting that they'd be changed to "null".
> The reordering of function declaration order wasn't something I did on
purpose.
> I can reverse it if it's a problem.
> 
Reversed it.

> Looking the comment on the implementation of decodeURLEscapeSequences at
> weborigin/KURL.cpp (and the various FIXMEs there), there might be better ways
to
> achieve the same functionality in Blink.
> Is it better to use
> core/platform/text/DecodeEscapSequences.h:decodeEscapeSequences ? A comment on
> XSSAuditor.cpp indicates so.

Updated the patch to use decodeEscapeSequences

[Yoav Weiss, 2013-09-10 06:46:06]

Following the discussion on 
https://groups.google.com/a/chromium.org/d/msg/blink-dev/SVQxmsJRa9c/qfd55y_8...,
I've updated the CL to include a runtime flag.

[ojan, 2013-09-11 21:16:01]

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_8...,
> I've updated the CL to include a runtime flag.

Please change the link in the change description to point to the whatwg spec and
base your implementation off of that. Where the two differ, we typically choose
the whatwg version.

[do-not-use, 2013-09-12 08:23:53]

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....
Source/core/dom/Document.cpp:5322: float devicePixelRatio = 1.0;
nit: return m_frame ? m_frame->devicePixelRatio() : 1; ?

https://codereview.chromium.org/23861003/diff/24001/Source/core/dom/Document.h
File Source/core/dom/Document.h (left):

https://codereview.chromium.org/23861003/diff/24001/Source/core/dom/Document....
Source/core/dom/Document.h:461: // Never returns 0.
This was according to coding style:
http://www.chromium.org/blink/coding-style#TOC-Comments

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.cpp:113: return m_bestFitImageURL.isEmpty() ?
fastGetAttribute(srcAttr) : m_bestFitImageURL;
Wouldn't isNull() suffice? Can bestFitSourceForImageAttributes() really return
an empty string?

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:119: if
(!m_srcSetAttribute.isEmpty()) {
Is it normal the one below is enabled at runtime but not this one?

[Yoav Weiss, 2013-09-12 09:30:20]

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.cpp:113: return m_bestFitImageURL.isEmpty() ?
fastGetAttribute(srcAttr) : m_bestFitImageURL;
On 2013/09/12 08:23:53, Christophe Dumez wrote:
> Wouldn't isNull() suffice? Can bestFitSourceForImageAttributes() really return
> an empty string?

m_bestFitImageURL is initialized as an empty string, and
bestFitSourceImageAttributes() can return an empty string, if both srcSttr and
srcsetAttr's values are empty (or srcsetAttr's value is invalid).

Both can be modified to a null string, which will enable an isNull check here.
What are the benefits? Is isNull() faster than isEmpty()?

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:119: if
(!m_srcSetAttribute.isEmpty()) {
On 2013/09/12 08:23:53, Christophe Dumez wrote:
> Is it normal the one below is enabled at runtime but not this one?

No, it's not. Good catch! :)

[Yoav Weiss, 2013-09-12 09:52:05]

On 2013/09/11 21:16:01, ojan wrote:
> 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_8...,
> > I've updated the CL to include a runtime flag.
> 
> Please change the link in the change description to point to the whatwg spec
and
> base your implementation off of that. Where the two differ, we typically
choose
> the whatwg version.

Modified the link. The current implementation matches the DPR switching subset
(i.e. the 'x' qualifier) of the whatwg spec.

[do-not-use, 2013-09-12 09:56:37]

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.cpp:113: return m_bestFitImageURL.isEmpty() ?
fastGetAttribute(srcAttr) : m_bestFitImageURL;
On 2013/09/12 09:30:20, Yoav Weiss wrote:
> On 2013/09/12 08:23:53, Christophe Dumez wrote:
> > Wouldn't isNull() suffice? Can bestFitSourceForImageAttributes() really
return
> > an empty string?
> 
> m_bestFitImageURL is initialized as an empty string, and
> bestFitSourceImageAttributes() can return an empty string, if both srcSttr and
> srcsetAttr's values are empty (or srcsetAttr's value is invalid).
> 
> Both can be modified to a null string, which will enable an isNull check here.
> What are the benefits? Is isNull() faster than isEmpty()?

To be clear the default constructor for String creates a NULL string, not an
empty one. Unless I am mistaken, m_bestFitImageURL is initially a NULL string
and bestFitSourceImageAttributes() may return String(), which is a NULL string
as well.

To answer your question, yes, isNull() is faster than isEmpty() still isEmpty()
checks if the string is null or empty (2 checks instead of 1).

[do-not-use, 2013-09-12 09:57:28]

On 2013/09/12 09:56:37, Christophe Dumez wrote:
>
https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/23861003/diff/24001/Source/core/html/HTMLImag...
> Source/core/html/HTMLImageElement.cpp:113: return m_bestFitImageURL.isEmpty()
?
> fastGetAttribute(srcAttr) : m_bestFitImageURL;
> On 2013/09/12 09:30:20, Yoav Weiss wrote:
> > On 2013/09/12 08:23:53, Christophe Dumez wrote:
> > > Wouldn't isNull() suffice? Can bestFitSourceForImageAttributes() really
> return
> > > an empty string?
> > 
> > m_bestFitImageURL is initialized as an empty string, and
> > bestFitSourceImageAttributes() can return an empty string, if both srcSttr
and
> > srcsetAttr's values are empty (or srcsetAttr's value is invalid).
> > 
> > Both can be modified to a null string, which will enable an isNull check
here.
> > What are the benefits? Is isNull() faster than isEmpty()?
> 
> To be clear the default constructor for String creates a NULL string, not an
> empty one. Unless I am mistaken, m_bestFitImageURL is initially a NULL string
> and bestFitSourceImageAttributes() may return String(), which is a NULL string
> as well.
> 
> To answer your question, yes, isNull() is faster than isEmpty() still
isEmpty()
> checks if the string is null or empty (2 checks instead of 1).

Typo:
To answer your question, yes, isNull() is faster than isEmpty() *because*
isEmpty()
checks if the string is null or empty (2 checks instead of 1).

[Yoav Weiss, 2013-09-12 17:52:34]

> > 
> > To be clear the default constructor for String creates a NULL string, not an
> > empty one. Unless I am mistaken, m_bestFitImageURL is initially a NULL
string
> > and bestFitSourceImageAttributes() may return String(), which is a NULL
string
> > as well.
> > 
> > To answer your question, yes, isNull() is faster than isEmpty() still
> isEmpty()
> > checks if the string is null or empty (2 checks instead of 1).

Didn't know that. Thanks! :)

Changed it to isNull, Fixed the missing runtime flag (by DRYING out that entire
code section to a separate method), and fixed style issues.

[cbiesinger, 2013-09-12 19:31:19]

On 2013/09/07 08:07:40, Yoav Weiss wrote:
>
https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
> > Source/core/html/parser/HTMLParserIdioms.cpp:343: image.imageURL =
> > decodeURLEscapeSequences(data[0]);
> > why decode them?
> 
> The reasoning is at:
> https://bugs.webkit.org/attachment.cgi?id=207963&action=review

So, the loader won't urldecode data: URLs?

I guess what I'm saying is that this will break some URLs, no? decode+encode is
lossy in the general case.

[ojan, 2013-09-12 23:26:37]

This mostly looks good. I'm not too familiar with the html/parser code though.
Adam or Eric, do you mind taking a look at that part of this patch? The rest of
the patch looks good to me.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:110: if
(RuntimeEnabledFeatures::srcsetEnabled() && !m_srcSetAttribute.isEmpty()) {
Nit: single line if should not have curly braces.

[abarth-chromium, 2013-09-13 05:54:06]

not lgtm

This looks like a good start, but there are a number of issues that need to be
resolved before we can land this CL.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.h:82: virtual const AtomicString
imageSourceURL() const OVERRIDE;
const AtomicString  <--- no need for const.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.h:117: AtomicString m_bestFitImageURL;
If this is a URL, why not use the type KURL?

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
File Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.idl:33: [Reflect] attribute DOMString srcset;
This property should only be exposed when the runtime flag is set.  Look at
other examples of EnabledAtRuntime in the IDL files.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:315: String
bestFitSourceForImageAttributes(float deviceScaleFactor, const String&
srcAttribute, const String& srcSetAttribute)
This is too much code for this file.  Please put this function in it's own file.

Parsing the srcset attribute isn't really an idiom of the HTML parser.  It's
something specific to HTMLImageElement.  Perhaps it should be a static function
in HTMLImageElement?

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:319: const String
srcSetAttributeValue = srcSetAttribute.simplifyWhiteSpace(isHTMLSpace);
No need for |const|.  The constness of a string is basically meaningless.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:338: imgScaleFactor =
data.last().substring(0, data.last().length() - 1).toFloat(&validScaleFactor);
This whole implementation is really inefficient.  You're creating a ton of
temporary strings just to parse out a few floating point numbers.  Maybe it
doesn't matter for performance.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:346: imageCandidates.append(image);
Please call imageCandidates.reserveInitialCapacity(srcSetTokens.size()) before
this loop starts to at least avoid reallocing that vector.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:349: const String src = 
srcAttribute.simplifyWhiteSpace(isHTMLSpace);
No const, please.  Also, you've got an extra space after the =

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:94: explicit
StartTagScanner(const StringImpl* tagImpl, float deviceScaleFactor = 1.0)
Please remove the default argument.  All callers should be forced to supply a
device scale factor.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:111:
setUrlToLoad(bestFitSourceForImageAttributes(m_deviceScaleFactor, m_urlToLoad,
m_srcSetAttribute), true);
This line of code isn't correct.  You haven't distinguished between srcset on
images and srcset on other elements, which means you're preloading srcset for
iframes as well.  Please add a test for this case.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:126: applySrcset();
Instead of doing the after every tag, we should only do it when it could
possibly matter, specifically when m_tagImpl matches imgTag.  Please don't just
add a branch here for m_tagImpl.  We already take that branch in
processAttribute.  We don't want to take it twice.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:185: void setUrlToLoad(const
String& value, bool allowReplacement = false)
Please remove the default value for the last argument and use an enum instead of
a boolean so that callers can see what the second argument means.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:235: String m_srcSetAttribute;
Where did you ever assign this member?

Please add a test that verifies that preloading works correctly.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.h:47: explicit
TokenPreloadScanner(const KURL& documentURL, float deviceScaleFactor = 1.0);
Please remove the default argument.  All callers should be forced to supply a
device scale factor.

[Yoav Weiss, 2013-09-13 06:17:23]

On 2013/09/12 19:31:19, cbiesinger wrote:
> On 2013/09/07 08:07:40, Yoav Weiss wrote:
> >
>
https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
> > > Source/core/html/parser/HTMLParserIdioms.cpp:343: image.imageURL =
> > > decodeURLEscapeSequences(data[0]);
> > > why decode them?
> > 
> > The reasoning is at:
> > https://bugs.webkit.org/attachment.cgi?id=207963&action=review
> 
> So, the loader won't urldecode data: URLs?
> 
> I guess what I'm saying is that this will break some URLs, no? decode+encode
is
> lossy in the general case.

Do you have a specific example of a URL that would break from this?
This is necessary for data URIs, since the ',' in them break the parsing
algorithm, they must be provided with a encoded "%2C" instead.
It's not necessary for other URIs, so I could decode only data URIs, if you
think it'd be safer.

[Yoav Weiss, 2013-09-13 06:18:32]

On 2013/09/13 05:54:06, abarth wrote:
> not lgtm
> 
> This looks like a good start, but there are a number of issues that need to be
> resolved before we can land this CL.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
> File Source/core/html/HTMLImageElement.h (right):
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
> Source/core/html/HTMLImageElement.h:82: virtual const AtomicString
> imageSourceURL() const OVERRIDE;
> const AtomicString  <--- no need for const.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
> Source/core/html/HTMLImageElement.h:117: AtomicString m_bestFitImageURL;
> If this is a URL, why not use the type KURL?
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
> File Source/core/html/HTMLImageElement.idl (right):
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
> Source/core/html/HTMLImageElement.idl:33: [Reflect] attribute DOMString
srcset;
> This property should only be exposed when the runtime flag is set.  Look at
> other examples of EnabledAtRuntime in the IDL files.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> File Source/core/html/parser/HTMLParserIdioms.cpp (right):
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLParserIdioms.cpp:315: String
> bestFitSourceForImageAttributes(float deviceScaleFactor, const String&
> srcAttribute, const String& srcSetAttribute)
> This is too much code for this file.  Please put this function in it's own
file.
> 
> Parsing the srcset attribute isn't really an idiom of the HTML parser.  It's
> something specific to HTMLImageElement.  Perhaps it should be a static
function
> in HTMLImageElement?
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLParserIdioms.cpp:319: const String
> srcSetAttributeValue = srcSetAttribute.simplifyWhiteSpace(isHTMLSpace);
> No need for |const|.  The constness of a string is basically meaningless.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLParserIdioms.cpp:338: imgScaleFactor =
> data.last().substring(0, data.last().length() - 1).toFloat(&validScaleFactor);
> This whole implementation is really inefficient.  You're creating a ton of
> temporary strings just to parse out a few floating point numbers.  Maybe it
> doesn't matter for performance.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLParserIdioms.cpp:346:
imageCandidates.append(image);
> Please call imageCandidates.reserveInitialCapacity(srcSetTokens.size()) before
> this loop starts to at least avoid reallocing that vector.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLParserIdioms.cpp:349: const String src = 
> srcAttribute.simplifyWhiteSpace(isHTMLSpace);
> No const, please.  Also, you've got an extra space after the =
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> File Source/core/html/parser/HTMLPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLPreloadScanner.cpp:94: explicit
> StartTagScanner(const StringImpl* tagImpl, float deviceScaleFactor = 1.0)
> Please remove the default argument.  All callers should be forced to supply a
> device scale factor.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLPreloadScanner.cpp:111:
> setUrlToLoad(bestFitSourceForImageAttributes(m_deviceScaleFactor, m_urlToLoad,
> m_srcSetAttribute), true);
> This line of code isn't correct.  You haven't distinguished between srcset on
> images and srcset on other elements, which means you're preloading srcset for
> iframes as well.  Please add a test for this case.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLPreloadScanner.cpp:126: applySrcset();
> Instead of doing the after every tag, we should only do it when it could
> possibly matter, specifically when m_tagImpl matches imgTag.  Please don't
just
> add a branch here for m_tagImpl.  We already take that branch in
> processAttribute.  We don't want to take it twice.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLPreloadScanner.cpp:185: void setUrlToLoad(const
> String& value, bool allowReplacement = false)
> Please remove the default value for the last argument and use an enum instead
of
> a boolean so that callers can see what the second argument means.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLPreloadScanner.cpp:235: String m_srcSetAttribute;
> Where did you ever assign this member?
> 
> Please add a test that verifies that preloading works correctly.
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> File Source/core/html/parser/HTMLPreloadScanner.h (right):
> 
>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLPreloadScanner.h:47: explicit
> TokenPreloadScanner(const KURL& documentURL, float deviceScaleFactor = 1.0);
> Please remove the default argument.  All callers should be forced to supply a
> device scale factor.

Thanks! I'll work on these comments and resubmit.

[Yoav Weiss, 2013-09-13 07:50:58]

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:315: String
bestFitSourceForImageAttributes(float deviceScaleFactor, const String&
srcAttribute, const String& srcSetAttribute)
On 2013/09/13 05:54:07, abarth wrote:
> This is too much code for this file.  Please put this function in it's own
file.
> 
> Parsing the srcset attribute isn't really an idiom of the HTML parser.  It's
> something specific to HTMLImageElement.  Perhaps it should be a static
function
> in HTMLImageElement?

Agreed that it's not an idiom. Since this is also used by HTMLPreloadScanner, I
believe it would be better if this code in in its own file (e.g.
HTMLSrcsetParser). If you disagree, I can add it to HTMLImageElement as a static
function.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:338: imgScaleFactor =
data.last().substring(0, data.last().length() - 1).toFloat(&validScaleFactor);
On 2013/09/13 05:54:07, abarth wrote:
> This whole implementation is really inefficient.  You're creating a ton of
> temporary strings just to parse out a few floating point numbers.  Maybe it
> doesn't matter for performance.

There's work being done by Romain Perier on the WebKit implementation to improve
the parsing efficiency[1]. I was thinking of porting that work as an improvement
CL once it's done. If you deem this critical, I can port the existing work to
this CL.

[1]https://bugs.webkit.org/show_bug.cgi?id=119423

[Yoav Weiss, 2013-09-13 10:04:18]

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:126: applySrcset();
On 2013/09/13 05:54:07, abarth wrote:
> Instead of doing the after every tag, we should only do it when it could
> possibly matter, specifically when m_tagImpl matches imgTag.  Please don't
just
> add a branch here for m_tagImpl.  We already take that branch in
> processAttribute.  We don't want to take it twice.

Assuming m_srcSetAttribute is only initialized for images, is going into
applySrcset() and doing nothing there acceptable? 

I prefer to keep a single if statement inside the function and call it twice.
Alternatively, I can simply inline applySrcset()'s content in both calls.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:235: String m_srcSetAttribute;
On 2013/09/13 05:54:07, abarth wrote:
> Where did you ever assign this member?
> 
> Please add a test that verifies that preloading works correctly.

That is a bug (assignment lost in merge). Thanks!

[cbiesinger, 2013-09-13 16:06:29]

On 2013/09/13 06:17:23, Yoav Weiss wrote:
> On 2013/09/12 19:31:19, cbiesinger wrote:
> > On 2013/09/07 08:07:40, Yoav Weiss wrote:
> > >
> >
>
https://codereview.chromium.org/23861003/diff/9001/Source/core/html/parser/HT...
> > > > Source/core/html/parser/HTMLParserIdioms.cpp:343: image.imageURL =
> > > > decodeURLEscapeSequences(data[0]);
> > > > why decode them?
> > > 
> > > The reasoning is at:
> > > https://bugs.webkit.org/attachment.cgi?id=207963&action=review
> > 
> > So, the loader won't urldecode data: URLs?
> > 
> > I guess what I'm saying is that this will break some URLs, no? decode+encode
> is
> > lossy in the general case.
> 
> Do you have a specific example of a URL that would break from this?
> This is necessary for data URIs, since the ',' in them break the parsing
> algorithm, they must be provided with a encoded "%2C" instead.
> It's not necessary for other URIs, so I could decode only data URIs, if you
> think it'd be safer.

http://foo/bar%3Fbaz
will get decoded to:
http://foo/bar?baz
which is not equivalent!

Mind you, I'm also not completely sure that this is safe even for data URLs but
I haven't tested it. Consider:
data:text/html;charset=iso-8859-1,gr%FCn
I think we'd decode that as if it were utf-8, notice that it is not valid utf-8,
and replace it with a replacement character. when we re-encode, we get something
different. note: this may even matter for this image use case if people use
percent encoding instead of base64.
(I've only skimmed this code, my analysis may be incorrect)

Basically, I think to handle this correctly,
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/data_url....
needs to support the case when the parts before the comma or the comma itself
are encoded as well.

[Yoav Weiss, 2013-09-13 17:44:01]

You're right, of course. 
I've fixed it locally by incorporating the newer parsing algorithm from Romain's
WebKit patch.
With the proper parsing algorithm (that splits on white spaces, rather than
commas), there's no need for URL decoding.

I'll upload a new patch soon.

[abarth-chromium, 2013-09-13 17:58:12]

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.cpp:315: String
bestFitSourceForImageAttributes(float deviceScaleFactor, const String&
srcAttribute, const String& srcSetAttribute)
On 2013/09/13 07:50:58, Yoav Weiss wrote:
> Agreed that it's not an idiom. Since this is also used by HTMLPreloadScanner,
I
> believe it would be better if this code in in its own file (e.g.
> HTMLSrcsetParser).

Sounds great.

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/36001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:126: applySrcset();
On 2013/09/13 10:04:18, Yoav Weiss wrote:
> I prefer to keep a single if statement inside the function and call it twice.
> Alternatively, I can simply inline applySrcset()'s content in both calls.

The important thing is to only do srcset-specific work for imgTags.  Here you're
doing srcset-specific work for all tags.

[Yoav Weiss, 2013-09-13 21:45:15]

I've addressed most issues raised in the review. 
Merging in large parts of Romain Perier's new srcset parser implementation (from
his WebKit patch) enabled to get rid of URL decoding without breaking data URIs.
That also enabled a much more efficient parser. Romain's implementation relied
on StringImpl::createWithoutCopying to avoid copying. Since it's missing in
Blink, I've replaced it with a custom mechanism. I also moved the srcset parser
code into html/parser/HTMLSrcsetParser

I've fixed the HTMLPreloadScanner bugs and added tests.

I did not address the issues below:

>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
> Source/core/html/HTMLImageElement.h:82: virtual const AtomicString
> imageSourceURL() const OVERRIDE;
> const AtomicString  <--- no need for const.

This is an override of a virtual method of Element.h. Changing its signature
results in compile time errors.

>
https://codereview.chromium.org/23861003/diff/36001/Source/core/html/HTMLImag...
> Source/core/html/HTMLImageElement.h:117: AtomicString m_bestFitImageURL;
> If this is a URL, why not use the type KURL?

Since this private member is used to store the URL and then return it as an
AtomicString, I'm not sure converting it from a String to KURL and then from
KURL to AtomicString is useful.

Please let me know if these 2 issues are critical. If so, I'll address them as
well.

[abarth-chromium, 2013-09-13 22:24:05]

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLParserIdioms.cpp (right):

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
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/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:108: enum URLReplacement {
URLReplacementAllow, URLReplacementDisallow };
Each of these should be on its own line.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:110: 
Please remove this extra blank line.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:113: // Resolve between src and
srcSet if we have them.
This "what" comment isn't needed.  Please remove it.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:129: applySrcset();
As I wrote before, we don't want to do any srcset specific processing for
non-img tags.  Please move this code out of the general codepath.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:164: setUrlToLoad(attributeValue,
URLReplacementDisallow);
URLReplacementDisallow -> DisallowURLReplacement

Identifiers should read like normal English.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:16: int m_length;
Instead of having a String*, a start, and a length, please use a StringView,
which combines these members into a class.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:17: float m_scaleFactor;
Please place private declarations after the public declarations and begin the
section with the "private:" keyword.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:30: String toString() const
Please add blank lines between member functions.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:32: return
String(m_source->characters8() + m_start, m_length);
How do you know it's always 8 bits?  Anyway, StringView solves this problem for
you.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:39: typedef Vector<ImageWithScale>
ImageCandidates;
Please add a blank line between top-level declarations.

I would remove this typedef.  It just makes the code harder to read.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:49: }
This function should got in HTMLParserIdioms.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:51: // See the specifications for
more details about the algorithm to follow.
Please omit this line of the comment.  The second line, which links to the spec,
is helpful.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:55: size_t imageCandidateStart = 0;
Lengths within strings are unsigned rather than size_t.  It's not possible to
have a String that's longer than unsigned.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:124: }
This parsing algorithm is too hard to read.  It's also very inefficient because
it operates on Strings with offsets.  A better approach is to operate on UChar
or LChar pointers and use skipUntil / skipWhile / skipExactly functions as in
ContentSecurityPolicy.cpp.  I'd probably move those functions to a more general
location and change the explicit UChar type to a templated CharType.

The top-level branch in the parsing algorithm should be whether the String is 8
bits or 16 bits wide.  Then we can get either the UChar or the LChar pointer out
of the String and call the parsing algorithm, which is templated based on the
CharType.

In this approach, you don't need to juggle offsets and lengths.  You can just
use begin and end pointers, which you can subtract if you need lengths.  It's
also much easier to stay in the bounds of the string because the |end| pointers
are good fences.

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:132: const String src = 
srcAttribute.simplifyWhiteSpace(isHTMLSpace);
You didn't address my previous review comments about this line.

Please address my previous review comments before uploading another patch and
asking me to review it.  It makes me feel like you don't respect my time when
you don't listen to my comments.  :(

https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:135: }
No need for { } for one-line if statements.

[Yoav Weiss, 2013-09-14 11:46:20]

> 
>
https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLPreloadScanner.cpp:129: applySrcset();
> As I wrote before, we don't want to do any srcset specific processing for
> non-img tags.  Please move this code out of the general codepath.

Since I obviously didn't understand what you meant, I'd like to kindly ask for
clarifications.
Assuming that the m_srcSetAttribute is only set when m_tagImpl == imgTag, will
the following srcset treatment code be considered "out of the general
codepath"?:

    void processAttributes(const Vector<CompactHTMLToken::Attribute>&
attributes)
    {
        if (!m_tagImpl)
            return;
        for (Vector<CompactHTMLToken::Attribute>::const_iterator iter =
attributes.begin(); iter != attributes.end(); ++iter)
            processAttribute(iter->name, iter->value);

        if (RuntimeEnabledFeatures::srcsetEnabled() &&
!m_srcSetAttribute.isNull())
            setUrlToLoad(bestFitSourceForImageAttributes(m_deviceScaleFactor,
m_urlToLoad, m_srcSetAttribute), AllowURLReplacement);

    }

If not, could you please outline a better approach? As far as I can tell, I
cannot run this inside processAttribute() since I'm not sure that I already have
the values of both src and srcset when I'm there.
If the above code does answer the "out of general codepath" requirement, will it
change if I use an inline method instead, to avoid repeating it twice (in both
processAttributes() methods)?

>
https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
> File Source/core/html/parser/HTMLSrcsetParser.cpp (right):
> 
>
https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLSrcsetParser.cpp:16: int m_length;
> Instead of having a String*, a start, and a length, please use a StringView,
> which combines these members into a class.

I was not aware of StringView. I'll definitely use it here. Thanks! :)

> 
>
https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLSrcsetParser.cpp:132: const String src = 
> srcAttribute.simplifyWhiteSpace(isHTMLSpace);
> You didn't address my previous review comments about this line.
> 
> Please address my previous review comments before uploading another patch and
> asking me to review it.  It makes me feel like you don't respect my time when
> you don't listen to my comments.  :(

I sincerely apologize if any of my actions seemed disrespectful. They were
definitely not intended as such. :(
This above line was simply an omission on my part (I removed the extra white
space, but failed to remove the const). It won't happen again.

[abarth-chromium, 2013-09-14 14:47:17]

On 2013/09/14 11:46:20, Yoav Weiss wrote:
> Assuming that the m_srcSetAttribute is only set when m_tagImpl == imgTag, will
> the following srcset treatment code be considered "out of the general
> codepath"?:

No, you're still adding work to the code path for every tag.

>     void processAttributes(const Vector<CompactHTMLToken::Attribute>&
> attributes)
>     {
>         if (!m_tagImpl)
>             return;
>         for (Vector<CompactHTMLToken::Attribute>::const_iterator iter =
> attributes.begin(); iter != attributes.end(); ++iter)
>             processAttribute(iter->name, iter->value);
> 
>         if (RuntimeEnabledFeatures::srcsetEnabled() &&
> !m_srcSetAttribute.isNull())
>             setUrlToLoad(bestFitSourceForImageAttributes(m_deviceScaleFactor,
> m_urlToLoad, m_srcSetAttribute), AllowURLReplacement);

^^^ This work here is done for every tag instead of being limited to just img
tags.

Rather than just gluing on the new feature to the existing structure, we're
going to need to restructure the code a bit to be able to implement the new
feature well.

[Yoav Weiss, 2013-09-14 14:54:37]

On 2013/09/14 14:47:17, abarth wrote:
> On 2013/09/14 11:46:20, Yoav Weiss wrote:
> > Assuming that the m_srcSetAttribute is only set when m_tagImpl == imgTag,
will
> > the following srcset treatment code be considered "out of the general
> > codepath"?:
> 
> No, you're still adding work to the code path for every tag.
> 
> >     void processAttributes(const Vector<CompactHTMLToken::Attribute>&
> > attributes)
> >     {
> >         if (!m_tagImpl)
> >             return;
> >         for (Vector<CompactHTMLToken::Attribute>::const_iterator iter =
> > attributes.begin(); iter != attributes.end(); ++iter)
> >             processAttribute(iter->name, iter->value);
> > 
> >         if (RuntimeEnabledFeatures::srcsetEnabled() &&
> > !m_srcSetAttribute.isNull())
> >            
setUrlToLoad(bestFitSourceForImageAttributes(m_deviceScaleFactor,
> > m_urlToLoad, m_srcSetAttribute), AllowURLReplacement);
> 
> ^^^ This work here is done for every tag instead of being limited to just img
> tags.
> 
> Rather than just gluing on the new feature to the existing structure, we're
> going to need to restructure the code a bit to be able to implement the new
> feature well.

OK, I get what you mean now. I'll modify the processAttribute() code so that
either srcset or src will set the best resource fit directly to m_urlToLoad
while taking its previous value into account (while ignoring duplicate src and
duplicate srcset). Thanks!

[Yoav Weiss, 2013-09-15 13:29:18]

> 
>
https://codereview.chromium.org/23861003/diff/53001/Source/core/html/parser/H...
> Source/core/html/parser/HTMLSrcsetParser.cpp:124: }
> This parsing algorithm is too hard to read.  It's also very inefficient
because
> it operates on Strings with offsets.  A better approach is to operate on UChar
> or LChar pointers and use skipUntil / skipWhile / skipExactly functions as in
> ContentSecurityPolicy.cpp.  I'd probably move those functions to a more
general
> location and change the explicit UChar type to a templated CharType.

Sounds good. Would you consider HTMLParserIdioms a good new location for the
skip...() functions?

[abarth-chromium, 2013-09-15 15:28:44]

On 2013/09/15 13:29:18, Yoav Weiss wrote:
> Sounds good. Would you consider HTMLParserIdioms a good new location for the
> skip...() functions?

Maybe Source/core/platform/ParsingUtilities.h ?  They're not specific to HTML in
any way.

[Yoav Weiss, 2013-09-18 09:38:50]

I've uploaded a new patch that addresses the previous review comments:
* Rebased patch
* ParsingUtilites.h added, defining skipUntil(), skipWhile() and skipExactly()
there, instead of inside ContentSecurityPolicy.cpp
* HTMLSrcsetParser was refactored to use the skip...() functions as well as
StringView for better readability and efficiency.
* simplifyWhiteSpace was removed from the srcset parser, since it is not really
required (and I believe it can cause correctness issues for some URLs)
* ContentSecurityPolicy.cpp now also uses ParsingUtilities.h for the skip...()
functions
* HTMLPreloadScanner now performs all the srcset specific processing inside the
imgTag handling, not as part of the general code path.
* Style issues were addressed

[abarth-chromium, 2013-09-19 18:09:51]

This looks much better.  We mostly just need to clean up the license blocks and
then we can land this CL!  A number of small comments below.  Thanks for
iterating on this CL!

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLDocumentParser.cpp:638: }
No need for { }

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.h:89: }
Why is this needed?

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.h:105: }
Why do you need these concrete specializations?

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:95: explicit
StartTagScanner(const StringImpl* tagImpl, float deviceScaleFactor)
No need for |explicit| now that we have more than one argument.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.cpp:160: if ((match(attributeName,
srcAttr) && !m_encounteredImgSrc)) {
No need for the extra set of ( )

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLPreloadScanner.h:47: explicit
TokenPreloadScanner(const KURL& documentURL, float deviceScaleFactor);
No need for |explicit| now that we have more than one parameter.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:3: // found in the LICENSE file.
Same problem with the license block.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:13: static inline bool
compareByScaleFactor(const ImageCandidate& first, const ImageCandidate& second)
The |static| and |inline| keywords are redundant here.  I'd recommend just using
|static|.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:40: skipWhile<CharType,
isNotHTMLSpace<CharType> >(position, attributeEnd);
skipUntil<CharType, isHTMLSpaceOrComma<CharType> > ?

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:63: if (*(qualifierEnd - 1) != 'x')
How do we know qualifierEnd - 1 is a valid address?  Maybe add an ASSERT at the
point where qualifierEnd becomes strictly greater than qualifierStart?

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:82: if (attribute.is8Bit())
What if attribute.isNull() ?  Calling is8Bit() on a null string will crash.  We
probably don't call these functions will null strings, but we might as well
handle that case and just return.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:97: return imageCandidates[i];
This will crash if imageCandidates is empty.

Rather than having the callers check imageCandidates.isEmpty(), we should handle
it in this function.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:113: return srcAttribute;
Or just:

return String()

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:3: // found in the LICENSE file.
This isn't the correct license block.  Please use the license block described in
the style guide:

http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:14: ImageCandidate()
Please initialize m_scaleFactor.  Otherwise m_scaleFactor will contain
uninitalized memory.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:23: }
No need to write this code explicitly.  The compiler will insert it for you.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:36: }
There's no need for the |inline| keyword here.  The compiler will insert it
automatically.

Also, m_string already has a toString() function (or something similar) that
does this work.  No need to write all this code.

[cbiesinger, 2013-09-19 23:01:43]

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:113: return srcAttribute;
On 2013/09/19 18:09:52, abarth wrote:
> Or just:
> 
> return String()

But that's not equivalent, right? If srcset is empty, we want to use src.

https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/Pars...
File Source/core/platform/ParsingUtilities.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/Pars...
Source/core/platform/ParsingUtilities.h:18: template<typename CharType, bool
characterPredicate(CharType)>
Couldn't you make CharType the second template argument, so that the compiler
can automatically deduct the value for CharType?

[Yoav Weiss, 2013-09-20 04:54:26]

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.h:105: }
On 2013/09/19 18:09:52, abarth wrote:
> Why do you need these concrete specializations?

I've added that to avoid changing the calls to isHTMLSpace all over the code. If
that's not a good idea, I can get rid of these specializations, and modify the
callers.

[Yoav Weiss, 2013-09-20 07:31:35]

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:40: skipWhile<CharType,
isNotHTMLSpace<CharType> >(position, attributeEnd);
On 2013/09/19 18:09:52, abarth wrote:
> skipUntil<CharType, isHTMLSpaceOrComma<CharType> > ?

skipUntil<CharType, isHTMLSpace<CharType> >

The algorithm shouldn't split on commas here, or data URIs will break.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:63: if (*(qualifierEnd - 1) != 'x')
On 2013/09/19 18:09:52, abarth wrote:
> How do we know qualifierEnd - 1 is a valid address?  Maybe add an ASSERT at
the
> point where qualifierEnd becomes strictly greater than qualifierStart?

When going into the skipUntil at line 51, we know that *position is not a white
space nor a comma.
Therefore, skipUntil must have advances, and qualifierEnd must be greater than
qualifierStart.

I'll add an ASSERT for that at line 53.
Should I also add a comment that explains the above?

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:36: }
On 2013/09/19 18:09:52, abarth wrote:
> There's no need for the |inline| keyword here.  The compiler will insert it
> automatically.
> 
> Also, m_string already has a toString() function (or something similar) that
> does this work.  No need to write all this code.

m_string is a StringView, so it doesn't have toString at the moment.
I'll add this code to StringView and only call it here.

[abarth-chromium, 2013-09-20 16:08:06]

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLParserIdioms.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLParserIdioms.h:105: }
On 2013/09/20 04:54:27, Yoav Weiss wrote:
> On 2013/09/19 18:09:52, abarth wrote:
> > Why do you need these concrete specializations?
> 
> I've added that to avoid changing the calls to isHTMLSpace all over the code.
If
> that's not a good idea, I can get rid of these specializations, and modify the
> callers.

Most of the callers should be able to infer the type from the type of the
argument.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:63: if (*(qualifierEnd - 1) != 'x')
On 2013/09/20 07:31:36, Yoav Weiss wrote:
> On 2013/09/19 18:09:52, abarth wrote:
> > How do we know qualifierEnd - 1 is a valid address?  Maybe add an ASSERT at
> the
> > point where qualifierEnd becomes strictly greater than qualifierStart?
> 
> When going into the skipUntil at line 51, we know that *position is not a
white
> space nor a comma.
> Therefore, skipUntil must have advances, and qualifierEnd must be greater than
> qualifierStart.
> 
> I'll add an ASSERT for that at line 53.
> Should I also add a comment that explains the above?

I think an ASSERT is probably enough.

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:113: return srcAttribute;
On 2013/09/19 23:01:44, cbiesinger wrote:
> On 2013/09/19 18:09:52, abarth wrote:
> > Or just:
> > 
> > return String()
> 
> But that's not equivalent, right? If srcset is empty, we want to use src.

In this case, you're testing for isNull().  There's only one null string, and
it's the same one you get from calling String().

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:36: }
On 2013/09/20 07:31:36, Yoav Weiss wrote:
> On 2013/09/19 18:09:52, abarth wrote:
> > There's no need for the |inline| keyword here.  The compiler will insert it
> > automatically.
> > 
> > Also, m_string already has a toString() function (or something similar) that
> > does this work.  No need to write all this code.
> 
> m_string is a StringView, so it doesn't have toString at the moment.
> I'll add this code to StringView and only call it here.

Thanks!

https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/Pars...
File Source/core/platform/ParsingUtilities.h (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/Pars...
Source/core/platform/ParsingUtilities.h:18: template<typename CharType, bool
characterPredicate(CharType)>
On 2013/09/19 23:01:44, cbiesinger wrote:
> Couldn't you make CharType the second template argument, so that the compiler
> can automatically deduct the value for CharType? 

+1

[abarth-chromium, 2013-09-20 16:08:57]

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/23861003/diff/18001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:113: return srcAttribute;
On 2013/09/20 16:08:07, abarth wrote:
> On 2013/09/19 23:01:44, cbiesinger wrote:
> > On 2013/09/19 18:09:52, abarth wrote:
> > > Or just:
> > > 
> > > return String()
> > 
> > But that's not equivalent, right? If srcset is empty, we want to use src.
> 
> In this case, you're testing for isNull().  There's only one null string, and
> it's the same one you get from calling String().

Oh, you're totally right.  Please ignore me.

[Yoav Weiss, 2013-09-20 21:00:10]

> 
>
https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/Pars...
> File Source/core/platform/ParsingUtilities.h (right):
> 
>
https://codereview.chromium.org/23861003/diff/18001/Source/core/platform/Pars...
> Source/core/platform/ParsingUtilities.h:18: template<typename CharType, bool
> characterPredicate(CharType)>
> On 2013/09/19 23:01:44, cbiesinger wrote:
> > Couldn't you make CharType the second template argument, so that the
compiler
> > can automatically deduct the value for CharType? 
> 
> +1

I've uploaded a new patch set that answers all the review comments besides the
one above.
Since the function definition depends on the CharType, I was unable to define
the CharType second.
If it's essential, I'd appreciate guidance on how to do that despite that
dependancy.

[abarth-chromium, 2013-09-20 21:13:33]

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/StringVie...
Source/wtf/text/StringView.h:95: return StringImpl::create(characters16(),
m_length);
We should probably add an optimization when m_offset is zero and m_length ==
m_impl->length().  In that case, we can just return m_impl instead of making a
copy.

[abarth-chromium, 2013-09-20 21:14:12]

Thanks for iterating on this CL.  We can optimize StringView::toString in a
followup CL.

[commit-bot: I haz the power, 2013-09-20 21:14:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/23861003/80001

[commit-bot: I haz the power, 2013-09-20 21:14:57]

Can't process patch for file
LayoutTests/http/tests/loading/resources/base-image1.png.
Binary file is empty. Maybe the file wasn't uploaded in the first place?

[commit-bot: I haz the power, 2013-09-20 21:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/23861003/91001

[commit-bot: I haz the power, 2013-09-20 23:47:26]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Yoav Weiss, 2013-09-21 03:51:35]

On 2013/09/20 23:47:26, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

Fixed the flaky tests that caused the try job to fail.

[commit-bot: I haz the power, 2013-09-21 03:52:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/23861003/109001

[commit-bot: I haz the power, 2013-09-21 06:13:53]

Change committed as 158158