Bug 40557 – Add a preload scanner for the HTML5 parser

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

[Adam Barth]

Add a preload scanner for the HTML5 parser

[Adam Barth]

Created attachment 58624 [details]
Patch

[Adam Barth]

I made the new file as an svn copy, but that was probably a waste.  I wasn't expecting to end up editing the file so much.

[Eric Seidel (no email)]

Attachment 58624 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3271115

[Eric Seidel (no email)]

Comment on attachment 58624 [details]
Patch

Lots going on in this patch.  Did you mean to leave the gurads and the class name as PreloadScanner?  Won't that cause a link error?

I think we should consider spliting this into bits.

r- for the ews fail.

[Adam Barth]

> Lots going on in this patch.  Did you mean to leave the gurads and the class name as PreloadScanner?  Won't that cause a link error?

I don't think you're reading the diff correctly.  I can regenerated it not using svn copy if that would make it easier to understand.

[Adam Barth]

Created attachment 58628 [details]
Patch

[WebKit Review Bot]

Attachment 58628 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3291124

[Adam Barth]

> Attachment 58628 [details] did not build on chromium:

Strange.  I wonder if the change to the gypi file isn't be processed.  Or maybe I screwed it up somehow?

[Adam Barth]

Created attachment 58714 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 58714 [details]
Patch

You said I should r- this.

But here are some comments anyway:
WebCore/html/HTML5PreloadScanner.cpp:51
 +      // FIXME: We should re-use these tokens in HTML5DocumentParser if the
We should save and re-use these tokens (otherwise I think you're meaning we should use them right there and then)

WebCore/html/HTML5PreloadScanner.cpp:59
 +  void HTML5PreloadScanner::processToken()
This function is quite large.

WebCore/html/HTML5PreloadScanner.cpp:64
 +      AtomicString tagName(m_token.name().data(), m_token.name().size());
Strange the token doesn't expose a method to do this.  Like makeAtomicTagName()?

WebCore/html/HTML5PreloadScanner.cpp:78
 +      for (HTML5Token::AttributeList::const_iterator iter = m_token.attributes().begin();
Can this loop be a separate function?

WebCore/html/HTML5PreloadScanner.cpp:74
 +      String urlToLoad;
Should these be a struct?

WebCore/html/HTML5PreloadScanner.cpp:103
 +      if (tagName == scriptTag)
If the above was a struct, then this could be a separate "startPreloads()" function or something, which was passed the struct.

[Eric Seidel (no email)]

Comment on attachment 58714 [details]
Patch

Arg name not needed:

+    void scan(const SegmentedString& source);

I would have named it m_preloadScanner:

+    OwnPtr<HTML5PreloadScanner> m_scanner;

[Adam Barth]

Created attachment 58726 [details]
Patch

[Adam Barth]

Created attachment 58727 [details]
Patch for landing

[Adam Barth]

Committed r61163: <http://trac.webkit.org/changeset/61163>

[Antti Koivisto]

CSS scanning got @imports is a critical performance feature and should be added back.

[Antti Koivisto]

for @imports :)

[Antti Koivisto]

https://bugs.webkit.org/show_bug.cgi?id=40666