Bug 40557

Summary: Add a preload scanner for the HTML5 parser
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, koivisto, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 39259    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing abarth: commit-queue+

Adam Barth
Reported 2010-06-14 00:46:19 PDT
Add a preload scanner for the HTML5 parser
Attachments
Patch (85.76 KB, patch)
2010-06-14 00:55 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (26.26 KB, patch)
2010-06-14 01:43 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (18.21 KB, patch)
2010-06-14 15:31 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (19.66 KB, patch)
2010-06-14 16:29 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch for landing (20.07 KB, patch)
2010-06-14 16:52 PDT, Adam Barth 		abarth: commit-queue+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2010-06-14 00:55:54 PDT
Created attachment 58624 [details] Patch
Adam Barth
Comment 2 2010-06-14 00:56:35 PDT
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)
Comment 3 2010-06-14 01:00:48 PDT
Attachment 58624 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3271115
Eric Seidel (no email)
Comment 4 2010-06-14 01:12:49 PDT
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
Comment 5 2010-06-14 01:25:16 PDT
> 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
Comment 6 2010-06-14 01:43:20 PDT
Created attachment 58628 [details] Patch
WebKit Review Bot
Comment 7 2010-06-14 03:47:10 PDT
Attachment 58628 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3291124
Adam Barth
Comment 8 2010-06-14 10:59:22 PDT
> 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
Comment 9 2010-06-14 15:31:08 PDT
Created attachment 58714 [details] Patch
Eric Seidel (no email)
Comment 10 2010-06-14 15:41:32 PDT
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 11 2010-06-14 15:42:05 PDT
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
Comment 12 2010-06-14 16:29:06 PDT
Created attachment 58726 [details] Patch
Adam Barth
Comment 13 2010-06-14 16:52:47 PDT
Created attachment 58727 [details] Patch for landing
Adam Barth
Comment 14 2010-06-14 17:07:28 PDT
Committed r61163: <http://trac.webkit.org/changeset/61163>
Antti Koivisto
Comment 15 2010-06-16 02:29:48 PDT
CSS scanning got @imports is a critical performance feature and should be added back.
Antti Koivisto
Comment 16 2010-06-16 02:30:45 PDT
for @imports :)
Antti Koivisto
Comment 17 2010-06-16 03:28:20 PDT
https://bugs.webkit.org/show_bug.cgi?id=40666
Note You need to log in before you can comment on or make changes to this bug.