Bug 46676 - HTMLTreeBuilder's InForeignContent code needs a re-write
Summary: HTMLTreeBuilder's InForeignContent code needs a re-write
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 47706
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-27 16:56 PDT by Eric Seidel (no email)
Modified: 2010-10-21 22:49 PDT (History)
5 users (show)

See Also:


Attachments
Patch (44.64 KB, patch)
2010-10-21 14:58 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (44.77 KB, patch)
2010-10-21 15:28 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (41.77 KB, patch)
2010-10-21 21:18 PDT, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-09-27 16:56:30 PDT
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.
Comment 1 James Simonsen 2010-10-13 16:09:13 PDT
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.
Comment 2 Eric Seidel (no email) 2010-10-13 16:12:29 PDT
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.
Comment 3 James Simonsen 2010-10-21 14:58:23 PDT
Created attachment 71497 [details]
Patch
Comment 4 Darin Adler 2010-10-21 14:59:33 PDT
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?
Comment 5 James Simonsen 2010-10-21 15:28:54 PDT
Created attachment 71502 [details]
Patch
Comment 6 James Simonsen 2010-10-21 15:31:09 PDT
(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.
Comment 7 Adam Barth 2010-10-21 17:57:50 PDT
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) ?
Comment 8 James Simonsen 2010-10-21 21:18:20 PDT
Created attachment 71523 [details]
Patch
Comment 9 James Simonsen 2010-10-21 21:19:44 PDT
(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.
Comment 10 Adam Barth 2010-10-21 22:04:09 PDT
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.
Comment 11 WebKit Commit Bot 2010-10-21 22:49:23 PDT
Comment on attachment 71523 [details]
Patch

Clearing flags on attachment: 71523

Committed r70293: <http://trac.webkit.org/changeset/70293>
Comment 12 WebKit Commit Bot 2010-10-21 22:49:31 PDT
All reviewed patches have been landed.  Closing bug.