Bug 62971 – view-source doesn't colorize </script> correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[WebKit Review Bot]

view-source doesn't colorize </script> correctly
Requested by abarth on #webkit.

[Alexey Proskuryakov]

See bug 61774 for more information (which should have been in this bug, but using the bot precludes one from filing good bugs).

[Adam Barth]

I'm actually surprised there isn't another bug to dupe this one against.

[Adam Barth]

This bug is worth fixing, but it's non-trivial.

[Adam Barth]

Created attachment 120621 [details]
Patch

[Adam Barth]

Note: Once we fix this bug, we should try adding these ASSERTs back:
http://trac.webkit.org/changeset/89258/trunk/Source/WebCore/html/parser/HTMLToken.h

[Eric Seidel (no email)]

Comment on attachment 120621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120621&action=review

LGTM if the perf is OK.

> Source/WebCore/ChangeLog:35
> +            previous segment of source.  We copy them into our accumulation
> +            buffer and adjust the token base offset to account for the extra
> +            characters.

Did you perf test this change?  Could you please mention the effect (if any) in the ChangeLog?

> Source/WebCore/ChangeLog:56
> +          - Previously, we cleared the temporary buffer lazily when we needed
> +            to add new characters to it.  Now we clear it eagerly so that it's
> +            length tells us whether we're currently using it to store
> +            characters.

Moar perf fear.

> Source/WebCore/html/parser/HTMLTokenizer.cpp:168
> +    m_temporaryBuffer.clear();

Is temporary buffer a spec name?  Seems we could give htis a more descriptive name about what it contains or how it's used...

> Source/WebCore/html/parser/HTMLTokenizer.cpp:1590
> +    characters.append('<');
> +    characters.append('/');

Can we assert that we're in the "waiting for end tag state"?  Seems this should at least have a comment that the only time we buffer characters is when searching for an end tag.

[Adam Barth]

I've re-tested the perf and there's no measurable difference.

[Adam Barth]

(In reply to comment #6)
> (From update of attachment 120621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120621&action=review
>
> > Source/WebCore/ChangeLog:56
> > +          - Previously, we cleared the temporary buffer lazily when we needed
> > +            to add new characters to it.  Now we clear it eagerly so that it's
> > +            length tells us whether we're currently using it to store
> > +            characters.
> 
> Moar perf fear.

Yes.  That was from before we had the benchmark.  :)

> > Source/WebCore/html/parser/HTMLTokenizer.cpp:168
> > +    m_temporaryBuffer.clear();
> 
> Is temporary buffer a spec name?  Seems we could give htis a more descriptive name about what it contains or how it's used...

It's a name from the spec:

http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#temporary-buffer

We don't need to use the spec name if something else would be clearer.

> > Source/WebCore/html/parser/HTMLTokenizer.cpp:1590
> > +    characters.append('<');
> > +    characters.append('/');
> 
> Can we assert that we're in the "waiting for end tag state"?  Seems this should at least have a comment that the only time we buffer characters is when searching for an end tag.

I've added a FIXME about adding this ASSERT.  I need to study this more closely to get the full list of states in which we use the temporary buffer.

[Adam Barth]

Created attachment 121013 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 121013 [details]
Patch for landing

Clearing flags on attachment: 121013

Committed r103999: <http://trac.webkit.org/changeset/103999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.