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.