Bug 125279 - Change how the form element pointer affects parsing template elements, to reduce weirdness in templates
Summary: Change how the form element pointer affects parsing template elements, to red...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-04 21:09 PST by Ryosuke Niwa
Modified: 2013-12-05 11:42 PST (History)
10 users (show)

See Also:


Attachments
Work in progress (6.27 KB, patch)
2013-12-04 21:17 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updates the parser (12.39 KB, patch)
2013-12-04 22:40 PST, Ryosuke Niwa
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-12-04 21:09:45 PST
Make HTML5 parser changes to match the HTML5 specification change: http://html5.org/tools/web-apps-tracker?from=8330&to=8331

Change how the form element pointer affects parsing <template> elements, to reduce weirdness in templates (e.g. before you couldn't have a template that contained a form if it was itself inside a form).
Comment 1 Ryosuke Niwa 2013-12-04 21:17:50 PST
Created attachment 218487 [details]
Work in progress
Comment 2 Ryosuke Niwa 2013-12-04 22:24:17 PST
Filed the corresponding Gecko and Blink bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=946585
https://code.google.com/p/chromium/issues/detail?id=326058
Comment 3 Ryosuke Niwa 2013-12-04 22:40:05 PST
Created attachment 218489 [details]
Updates the parser
Comment 4 Antti Koivisto 2013-12-05 11:30:00 PST
Comment on attachment 218489 [details]
Updates the parser

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

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:416
> +    if (!insideTemplateElement())
> +        m_form = static_pointer_cast<HTMLFormElement>(element);
> +    static_pointer_cast<HTMLFormElement>(element)->setDemoted(isDemoted);

Please do the cast only once and put it to a local

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:544
> +inline bool HTMLConstructionSite::insideTemplateElement()
> +{
> +    return !ownerDocumentForCurrentNode().frame();
> +}

Maybe we should just have a boolean for this state? This is bit hard to understand.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1869
> +#if ENABLE(TEMPLATE_ELEMENT)
> +        if (!isParsingTemplateContents()) {
> +#endif

Not a big fan of #ifs like this
Comment 5 Ryosuke Niwa 2013-12-05 11:42:11 PST
Comment on attachment 218489 [details]
Updates the parser

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

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:416
>> +    static_pointer_cast<HTMLFormElement>(element)->setDemoted(isDemoted);
> 
> Please do the cast only once and put it to a local

Done.

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:544
>> +}
> 
> Maybe we should just have a boolean for this state? This is bit hard to understand.

Yeah, I'd love to. Unfortunately, it's very tricky to do in one patch :(

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1869
>> +#endif
> 
> Not a big fan of #ifs like this

Got rid of the if-def since the latter part doesn't use any template-specific code and "return false" is hard-coded
in isParsingTemplateContents() so that the compiler can easily eliminate the code block.
Comment 6 Ryosuke Niwa 2013-12-05 11:42:18 PST
Thanks for the review!
Comment 7 Ryosuke Niwa 2013-12-05 11:42:33 PST
Committed r160182: <http://trac.webkit.org/changeset/160182>