Bug 57404 - Teach the preload scanner about <input type=image>
Summary: Teach the preload scanner about <input type=image>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-29 16:59 PDT by Tony Gentilcore
Modified: 2011-03-29 18:36 PDT (History)
4 users (show)

See Also:

Attachments
Patch (6.22 KB, patch)
2011-03-29 17:05 PDT, Tony Gentilcore 		no flags Details | Formatted Diff | Diff
Patch for landing (6.07 KB, patch)
2011-03-29 18:04 PDT, Tony Gentilcore 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2011-03-29 16:59:11 PDT 
Teach the preload scanner about <input type=image>
Comment 1 Tony Gentilcore 2011-03-29 17:05:23 PDT 
Created attachment 87436 [details]
Patch
Comment 2 James Simonsen 2011-03-29 17:44:57 PDT 
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.
Comment 3 Adam Barth 2011-03-29 17:58:49 PDT 
Comment on attachment 87436 [details]
Patch

I agree with James.
Comment 4 Tony Gentilcore 2011-03-29 18:04:42 PDT 
Created attachment 87446 [details]
Patch for landing
Comment 5 Tony Gentilcore 2011-03-29 18:04:51 PDT 
> 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.
Comment 6 Darin Adler 2011-03-29 18:14:03 PDT 
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.
Comment 7 Tony Gentilcore 2011-03-29 18:16:25 PDT 
(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.
Comment 8 WebKit Commit Bot 2011-03-29 18:36:09 PDT 
Comment on attachment 87446 [details]
Patch for landing

Clearing flags on attachment: 87446

Committed r82379: <http://trac.webkit.org/changeset/82379>
Comment 9 WebKit Commit Bot 2011-03-29 18:36:15 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 10 Tony Gentilcore 2011-03-29 18:36:55 PDT 
(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.