Make BackgroundHTMLParser rewind the preload scanner instead of clear it

[Tony Gentilcore]

Make BackgroundHTMLParser rewind the preload scanner instead of clear it

[Tony Gentilcore]

Created attachment 189607 [details]
Patch

[Tony Gentilcore]

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.

[Adam Barth]

Comment on attachment 189607 [details]
Patch

Great!  Does this have any effect on the top25 benchmark?  (Just curious---not a requirement for landing.)

[Adam Barth]

Yes.  There's no way to document.write in the middle of a <style> element.

[Eric Seidel (no email)]

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?

[Tony Gentilcore]

(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?

[Tony Gentilcore]

(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.

[Eric Seidel (no email)]

class PreloadScanner {
    struct State {
        bool inStyle;
        int templateCount;
         ...
    }

private:
   PreloadScannerState m_state;
}

class CheckPoint {
    TokenizerState tokenizerState;
    PreloadScannerState preloadScannerState;
}

[Tony Gentilcore]

(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

[Eric Seidel (no email)]

My review comment was more a question than anything. :)  You should use your discretion of course and do whatever you think reads best.

[Tony Gentilcore]

(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?

[Adam Barth]

I don't think it's a big deal either way.

[Eric Seidel (no email)]

I'm not sure it matters much either way.  Just felt a bit odd at first sight to have 2 separate Checkpoint objects.

[WebKit Review Bot]

Comment on attachment 189607 [details]
Patch

Clearing flags on attachment: 189607

Committed r143661: <http://trac.webkit.org/changeset/143661>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.