Bug 57404 – Teach the preload scanner about <input type=image>

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Teach the preload scanner about <input type=image>

[Tony Gentilcore]

Created attachment 87436 [details]
Patch

[James Simonsen]

Comment on attachment 87436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87436&action=review

LGTM

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:101
> +    static bool inputTypeAttributeIsImage(const String& attributeValue)

I'd just inline this. The others are multiple lines, so it helps readability. Here, I think it'd be just as readable if this was inline.

[Adam Barth]

Comment on attachment 87436 [details]
Patch

I agree with James.

[Tony Gentilcore]

Created attachment 87446 [details]
Patch for landing

[Tony Gentilcore]

> I'd just inline this. The others are multiple lines, so it helps readability. Here, I think it'd be just as readable if this was inline.

Good call. Done.

[Darin Adler]

Comment on attachment 87436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87436&action=review

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:64
> -        if (m_tagName != scriptTag && m_tagName != imgTag && m_tagName != linkTag)
> +        if (m_tagName != imgTag
> +            && m_tagName != inputTag
> +            && m_tagName != linkTag
> +            && m_tagName != scriptTag)

Seems like this would still fit fine all on one line.

[Tony Gentilcore]

(In reply to comment #6)
> (From update of attachment 87436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87436&action=review
> 
> > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:64
> > -        if (m_tagName != scriptTag && m_tagName != imgTag && m_tagName != linkTag)
> > +        if (m_tagName != imgTag
> > +            && m_tagName != inputTag
> > +            && m_tagName != linkTag
> > +            && m_tagName != scriptTag)
> 
> Seems like this would still fit fine all on one line.

I am planning to add iframe as well so I broke it up and alphabetized. If you prefer I could leave it as-is for now. Don't worry, I understand not to over line break conditionals in general.

[WebKit Commit Bot]

Comment on attachment 87446 [details]
Patch for landing

Clearing flags on attachment: 87446

Committed r82379: <http://trac.webkit.org/changeset/82379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Gentilcore]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 87436 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87436&action=review
> > 
> > > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:64
> > > -        if (m_tagName != scriptTag && m_tagName != imgTag && m_tagName != linkTag)
> > > +        if (m_tagName != imgTag
> > > +            && m_tagName != inputTag
> > > +            && m_tagName != linkTag
> > > +            && m_tagName != scriptTag)
> > 
> > Seems like this would still fit fine all on one line.
> 
> I am planning to add iframe as well so I broke it up and alphabetized. If you prefer I could leave it as-is for now. Don't worry, I understand not to over line break conditionals in general.

The cq landed this. I can go back and undo this change if you like.