Summary: | Make BackgroundHTMLParser rewind the preload scanner instead of clear it | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||
Component: | New Bugs | Assignee: | Tony Gentilcore <tonyg> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, eric, esprehn+autocc, ojan.autocc, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 106127 | ||||||
Attachments: |
|
Description
Tony Gentilcore
2013-02-21 15:02:09 PST
Created attachment 189607 [details]
Patch
Comment on attachment 189607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189607&action=review > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:285 > + m_cssScanner.reset(); It looks like it is safe to just reset the CSSPreloadScanner instead of restoring any of its members, but I'm not 100% sure about this part. Comment on attachment 189607 [details]
Patch
Great! Does this have any effect on the top25 benchmark? (Just curious---not a requirement for landing.)
Yes. There's no way to document.write in the middle of a <style> element. Comment on attachment 189607 [details]
Patch
Should this just be a state struct? And we have one single checkpoint object? Which has one of each state struct? The preload scanner just has an m_state member for all its state?
(In reply to comment #5) > (From update of attachment 189607 [details]) > Should this just be a state struct? And we have one single checkpoint object? Which has one of each state struct? The preload scanner just has an m_state member for all its state? I don't quite see where you are going. Can you please explain a bit more? (In reply to comment #3) > (From update of attachment 189607 [details]) > Great! Does this have any effect on the top25 benchmark? (Just curious---not a requirement for landing.) I didn't run it, but it sure seems like we would have missed some preload scanning opportunities before. class PreloadScanner { struct State { bool inStyle; int templateCount; ... } private: PreloadScannerState m_state; } class CheckPoint { TokenizerState tokenizerState; PreloadScannerState preloadScannerState; } (In reply to comment #8) > class PreloadScanner { > struct State { > bool inStyle; > int templateCount; > ... > } > > private: > PreloadScannerState m_state; > } > > class CheckPoint { > TokenizerState tokenizerState; > PreloadScannerState preloadScannerState; > } Sure, I'll rework this My review comment was more a question than anything. :) You should use your discretion of course and do whatever you think reads best. (In reply to comment #9) > (In reply to comment #8) > > class PreloadScanner { > > struct State { > > bool inStyle; > > int templateCount; > > ... > > } > > > > private: > > PreloadScannerState m_state; > > } > > > > class CheckPoint { > > TokenizerState tokenizerState; > > PreloadScannerState preloadScannerState; > > } > > Sure, I'll rework this On second thought, BackgroundHTMLInputStream's rewindTo() is a bit more involved than just copying a single state object. I'm not sure it lends itself well to this approach. I also think it is a bit awkward for the preload scanner code to have to do m_state.inStyle everywhere instead of just m_inStyle. WDYT? I don't think it's a big deal either way. I'm not sure it matters much either way. Just felt a bit odd at first sight to have 2 separate Checkpoint objects. Comment on attachment 189607 [details] Patch Clearing flags on attachment: 189607 Committed r143661: <http://trac.webkit.org/changeset/143661> All reviewed patches have been landed. Closing bug. |