Bug 46676 – HTMLTreeBuilder's InForeignContent code needs a re-write

• Component: DOM
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

HTMLTreeBuilder's InForeignContent code will needs a re-write

The spec was just overhauled:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=10314

CCing folks who have some knowledge of the new parser.  I'll eventually get around to doing this if no one else does.  We'll need to sync the libhtml5 test suite again too I'm sure.

[James Simonsen]

I'll take a look at this. I'm new though, so I might be a bit slow.

I did sync the html5lib test suite, but there don't seem to be any new tests due to this change. I'll add new ones to the webkit tests instead.

[Eric Seidel (no email)]

A good place to start is to add tests for this, even if they fail.

You may need to turn this into a master bug and have dependent bugs for the individual changes.  Happy to talk about this more over IRC too.

[James Simonsen]

Created attachment 71497 [details]
Patch

[Darin Adler]

Comment on attachment 71497 [details]
Patch

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

> WebCore/html/parser/HTMLElementStack.cpp:73
> +#if ENABLE(SVG_FOREIGN_OBJECT)
> +        || element->hasTagName(SVGNames::foreignObjectTag)
> +#endif

Given that the other MathML and SVG names are handled even if the language is not compiled in, should this really be ifdef'd?

[James Simonsen]

Created attachment 71502 [details]
Patch

[James Simonsen]

(In reply to comment #4)
> (From update of attachment 71497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71497&action=review
> 
> > WebCore/html/parser/HTMLElementStack.cpp:73
> > +#if ENABLE(SVG_FOREIGN_OBJECT)
> > +        || element->hasTagName(SVGNames::foreignObjectTag)
> > +#endif
> 
> Given that the other MathML and SVG names are handled even if the language is not compiled in, should this really be ifdef'd?

Good point. Fixed.

[Adam Barth]

Comment on attachment 71502 [details]
Patch

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

This looks generally good.  Just some stylistic comments.  Eric should look at this too because he did most of the mixed content work.

> LayoutTests/ChangeLog:19
> +        * html5lib/runner-expected.txt: Since the behavior of InForeignContentMode has changed, the expectations need to be updated. These have been manually verified.

At some point we'll sync with html5lib and rebaseline the expected results.

> WebCore/html/parser/HTMLTreeBuilder.cpp:1141
>          defaultForInitial();
> +        prepareToReprocessToken();

Should we just make the prepare call part of the default call?

> WebCore/html/parser/HTMLTreeBuilder.cpp:1234
> +            prepareToReprocessToken();
>              processStartTag(token);

Can we combine these as reprocessStartTag(token) ?

[James Simonsen]

Created attachment 71523 [details]
Patch

[James Simonsen]

(In reply to comment #7)
> (From update of attachment 71502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71502&action=review
> > WebCore/html/parser/HTMLTreeBuilder.cpp:1141
> >          defaultForInitial();
> > +        prepareToReprocessToken();
> 
> Should we just make the prepare call part of the default call?

Sure. Done.
 
> > WebCore/html/parser/HTMLTreeBuilder.cpp:1234
> > +            prepareToReprocessToken();
> >              processStartTag(token);
> 
> Can we combine these as reprocessStartTag(token) ?

Yep. Done.

[Adam Barth]

Comment on attachment 71523 [details]
Patch

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

Quite nice.  Thanks!

> WebCore/html/parser/HTMLTreeBuilder.cpp:2616
> +        setInsertionMode(InBodyMode);
>          processEndOfFile(token);

No prepareToReprocess here?

> WebCore/html/parser/HTMLTreeBuilder.cpp:2629
> +        prepareToReprocessToken();
>          processEndOfFile(token);

We could have added a reprocessEndOfFile call for symmetry, but I can see why you skipped it.

[WebKit Commit Bot]

Comment on attachment 71523 [details]
Patch

Clearing flags on attachment: 71523

Committed r70293: <http://trac.webkit.org/changeset/70293>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.