Add <menuitem>, new HTMLMenuElement IDL attributes.

Add <menuitem>, new HTMLMenuElement IDL attributes.

Specifications:
http://www.whatwg.org/specs/web-apps/current-work/#the-menu-element
http://www.whatwg.org/specs/web-apps/current-work/#the-menuitem-element

Changes in /parser/HTMLStackItem.h are required as per this part of specification
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#special
And isSpecial() is used here
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsing-main-inbody
with section "Any other end tag"
During end tag processing, if it is a special element, it should report parseError.

Changes in /parser/HTMLTreeBuilder.cpp are required as per this part of specification
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#tokenization
"When a start tag token is emitted with its self-closing flag set, if the
flag is not acknowledged when it is processed by the tree construction
stage, that is a parse error."
<menuitem> has no end tag, its a self closing tag, and it should not
contain any inner node, so adding token using insertSelfClosingHTMLElement().

Changes in /rendering/RenderTheme.cpp are as per this part of specification
http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#hidden-elements


BUG=87553
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180037

[pals, 2014-08-07 07:18:48]

Please take a look.

[tkent, 2014-08-07 07:26:29]

Don't we need to update the HTML parser for <menuitem>?  Please refer to the
parser section of the HTML standard.

https://codereview.chromium.org/443373002/diff/1/Source/core/html/HTMLMenuIte...
File Source/core/html/HTMLMenuItemElement.idl (right):

https://codereview.chromium.org/443373002/diff/1/Source/core/html/HTMLMenuIte...
Source/core/html/HTMLMenuItemElement.idl:13: // FIXME(sanjoy): icon, radiogroup
and command need to be implemented.
FIXME(sanjoy) -> FIXME

http://www.chromium.org/blink/coding-style#TOC-Comments

[pals, 2014-08-07 09:46:49]

Yes we need to update HTML parser for <menuitem>. I need some time to understand
the html parser specification and the parser code. I shall update the CL once I
figure out the changes required.

[pals, 2014-08-07 11:27:13]

Updated HTML parser for <menuitem> as per specification. PTAL.

https://codereview.chromium.org/443373002/diff/1/Source/core/html/HTMLMenuIte...
File Source/core/html/HTMLMenuItemElement.idl (right):

https://codereview.chromium.org/443373002/diff/1/Source/core/html/HTMLMenuIte...
Source/core/html/HTMLMenuItemElement.idl:13: // FIXME(sanjoy): icon, radiogroup
and command need to be implemented.
On 2014/08/07 07:26:29, tkent wrote:
> FIXME(sanjoy) -> FIXME
> 
> http://www.chromium.org/blink/coding-style#TOC-Comments

Done.

[tkent, 2014-08-07 13:19:52]

This CL needs tests for the HTML parser changes.

[tkent, 2014-08-07 13:54:56]

On 2014/08/07 13:19:52, tkent wrote:
> This CL needs tests for the HTML parser changes.

And needs rendering tests.  e.g. what should be shown with the following URL?

data:text/html,<menuitem>orphan item</menuitem><menu><menuitem>item in a
menu</menuitem></menu>

[pals, 2014-08-07 14:26:47]

On 2014/08/07 13:54:56, tkent wrote:
> On 2014/08/07 13:19:52, tkent wrote:
> > This CL needs tests for the HTML parser changes.

I don't see any existing tests for
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#void-...

Should I add similar test as this one?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> 
> And needs rendering tests.  e.g. what should be shown with the following URL?
> 
> data:text/html,<menuitem>orphan item</menuitem><menu><menuitem>item in a
> menu</menuitem></menu>

<menuitem> does not render anything on its own.

The URL above is invalid. This following url does not render anything.

data:text/html,<menuitem label="orphan item"><menu><menuitem label="item in a
menu"></menu>

[tkent, 2014-08-08 00:45:17]

On 2014/08/07 14:26:47, sanjoy.pal wrote:
> On 2014/08/07 13:54:56, tkent wrote:
> > On 2014/08/07 13:19:52, tkent wrote:
> > > This CL needs tests for the HTML parser changes.
> 
> I don't see any existing tests for
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#void-...
> 
> Should I add similar test as this one?
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I don't know if menuitem tests are similar to it.  Anyway, please add tests.

> > And needs rendering tests.  e.g. what should be shown with the following
URL?
> > 
> > data:text/html,<menuitem>orphan item</menuitem><menu><menuitem>item in a
> > menu</menuitem></menu>
> 
> <menuitem> does not render anything on its own.

Why?  What does the specification say?

> The URL above is invalid. This following url does not render anything.

Validity doesn't matter. What should be shown with the above invalid markup
according to the standard?

For example, the following invalid markup shows nothing because the HTML
standard defines |datalist { display: none; }|.
   data:text/html<datalist>abc</datalist>

[pals, 2014-08-08 11:49:02]

On 2014/08/08 00:45:17, tkent wrote:
> On 2014/08/07 14:26:47, sanjoy.pal wrote:
> > On 2014/08/07 13:54:56, tkent wrote:
> > > On 2014/08/07 13:19:52, tkent wrote:
> > > > This CL needs tests for the HTML parser changes.
> > 
> > I don't see any existing tests for
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#void-...
> > 
> > Should I add similar test as this one?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I don't know if menuitem tests are similar to it.  Anyway, please add tests.
> 
> > > And needs rendering tests.  e.g. what should be shown with the following
> URL?
> > > 
> > > data:text/html,<menuitem>orphan item</menuitem><menu><menuitem>item in a
> > > menu</menuitem></menu>
> > 
> > <menuitem> does not render anything on its own.
> 
> Why?  What does the specification say?
Specification says:
Content model:
Nothing.

So, I added |menuitem { display: none; }|

Also added tests for parser and rendering test for menuitem.

[tkent, 2014-08-11 01:57:13]

On 2014/08/08 11:49:02, sanjoy.pal wrote:
> > Why?  What does the specification say?
> Specification says:
> Content model:
> Nothing.
> 
> So, I added |menuitem { display: none; }|

Content model:nothing doesn't mean display:none.
Please refer to "Rendering" section of the HTML specification.

[tkent, 2014-08-11 01:58:33]

https://codereview.chromium.org/443373002/diff/40001/LayoutTests/fast/parser/...
File LayoutTests/fast/parser/stray-menuitem.html (right):

https://codereview.chromium.org/443373002/diff/40001/LayoutTests/fast/parser/...
LayoutTests/fast/parser/stray-menuitem.html:1: <!DOCTYPE html>
This test doesn't make sense.  This test passes without the parser-related
changes in this CL, right?

https://codereview.chromium.org/443373002/diff/40001/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/443373002/diff/40001/Source/core/css/html.css...
Source/core/css/html.css:653: menuitem {
We can't add CSS rules for a runtime-enabled feature to html.css because the
rules are effective even if the feature is disabled.  Please refer to
RenderTheme::extraDefaultStyleSheet.

[pals, 2014-08-11 09:11:58]

Modified the tests to check no endTag generation for <menuitem>.

https://codereview.chromium.org/443373002/diff/40001/LayoutTests/fast/parser/...
File LayoutTests/fast/parser/stray-menuitem.html (right):

https://codereview.chromium.org/443373002/diff/40001/LayoutTests/fast/parser/...
LayoutTests/fast/parser/stray-menuitem.html:1: <!DOCTYPE html>
On 2014/08/11 01:58:32, tkent wrote:
> This test doesn't make sense.  This test passes without the parser-related
> changes in this CL, right?

Done. Made the necessary changes to check no endTag.

https://codereview.chromium.org/443373002/diff/40001/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/443373002/diff/40001/Source/core/css/html.css...
Source/core/css/html.css:653: menuitem {
On 2014/08/11 01:58:32, tkent wrote:
> We can't add CSS rules for a runtime-enabled feature to html.css because the
> rules are effective even if the feature is disabled.  Please refer to
> RenderTheme::extraDefaultStyleSheet.

Done.

[pals, 2014-08-11 09:44:10]

On 2014/08/11 09:11:58, sanjoy.pal wrote:
> Modified the tests to check no endTag generation for <menuitem>.
> 
>
https://codereview.chromium.org/443373002/diff/40001/LayoutTests/fast/parser/...
> File LayoutTests/fast/parser/stray-menuitem.html (right):
> 
>
https://codereview.chromium.org/443373002/diff/40001/LayoutTests/fast/parser/...
> LayoutTests/fast/parser/stray-menuitem.html:1: <!DOCTYPE html>
> On 2014/08/11 01:58:32, tkent wrote:
> > This test doesn't make sense.  This test passes without the parser-related
> > changes in this CL, right?
> 
> Done. Made the necessary changes to check no endTag.
> 
> https://codereview.chromium.org/443373002/diff/40001/Source/core/css/html.css
> File Source/core/css/html.css (right):
> 
>
https://codereview.chromium.org/443373002/diff/40001/Source/core/css/html.css...
> Source/core/css/html.css:653: menuitem {
> On 2014/08/11 01:58:32, tkent wrote:
> > We can't add CSS rules for a runtime-enabled feature to html.css because the
> > rules are effective even if the feature is disabled.  Please refer to
> > RenderTheme::extraDefaultStyleSheet.
> 
> Done.

Also, moved the parser related changes under RuntimeEnabled check.

[tkent, 2014-08-11 10:23:01]

<menu type=popup><menuitem><menuitem></menu>

The second <menuitem> won't be a child of the first <menuitem>, right?

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLStackItem.h (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLStackItem.h:182: ||
(RuntimeEnabledFeatures::contextMenuEnabled() && tagName ==
HTMLNames::menuitemTag)
Could you explain what part of the specification brings this change, and what
behavior is changed?

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:803: if
((RuntimeEnabledFeatures::contextMenuEnabled() && token->name() == menuitemTag)
Could you explain what part of the specification brings this change and what
behavior is changed?

https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTheme.cpp (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTheme.cpp:423:
runtimeCSS.appendLiteral("menu[type=\"popup\" i] { display: none; }");
Could you explain what part of the specification brings this change?

[pals, 2014-08-11 11:10:44]

On 2014/08/11 10:23:01, tkent wrote:
> <menu type=popup><menuitem><menuitem></menu>
> 
> The second <menuitem> won't be a child of the first <menuitem>, right?
Yes. Thats correct. Shall I add a test for the same.
> 
>
https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLStackItem.h (right):
> 
>
https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLStackItem.h:182: ||
> (RuntimeEnabledFeatures::contextMenuEnabled() && tagName ==
> HTMLNames::menuitemTag)
> Could you explain what part of the specification brings this change, and what
> behavior is changed?
Specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#special

As per this section
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
"Any other end tag"
During end tag processing
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
if it is a special element, it should report parseError.

> 
>
https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
> File Source/core/html/parser/HTMLTreeBuilder.cpp (right):
> 
>
https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
> Source/core/html/parser/HTMLTreeBuilder.cpp:803: if
> ((RuntimeEnabledFeatures::contextMenuEnabled() && token->name() ==
menuitemTag)
> Could you explain what part of the specification brings this change and what
> behavior is changed?
Specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#ackno...
"When an end tag token is emitted with its self-closing flag set, that is a
parse error."
<menuitem> has no end tag, its a self closing tag, and it should not contain any
inner node.

> 
>
https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
> File Source/core/rendering/RenderTheme.cpp (right):
> 
>
https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
> Source/core/rendering/RenderTheme.cpp:423:
> runtimeCSS.appendLiteral("menu[type=\"popup\" i] { display: none; }");
> Could you explain what part of the specification brings this change?
Specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#hi...

[pals, 2014-08-11 11:27:21]

<menu type=popup><menuitem><menuitem></menu>

Added tests to check that the second <menuitem> won't be a child of the first
<menuitem>.

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLStackItem.h (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLStackItem.h:182: ||
(RuntimeEnabledFeatures::contextMenuEnabled() && tagName ==
HTMLNames::menuitemTag)
On 2014/08/11 10:23:01, tkent wrote:
> Could you explain what part of the specification brings this change, and what
> behavior is changed?
Specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#special

As per this section
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
"Any other end tag"
During end tag processing
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
if it is a special element, it should report parseError.

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:803: if
((RuntimeEnabledFeatures::contextMenuEnabled() && token->name() == menuitemTag)
On 2014/08/11 10:23:01, tkent wrote:
> Could you explain what part of the specification brings this change and what
> behavior is changed?
Specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#ackno...
"When an end tag token is emitted with its self-closing flag set, that is a
parse error."
<menuitem> has no end tag, its a self closing tag, and it should not contain any
inner node.

https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTheme.cpp (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTheme.cpp:423:
runtimeCSS.appendLiteral("menu[type=\"popup\" i] { display: none; }");
On 2014/08/11 10:23:01, tkent wrote:
> Could you explain what part of the specification brings this change?

http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#hi...

[tkent, 2014-08-11 13:36:57]

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLStackItem.h (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLStackItem.h:182: ||
(RuntimeEnabledFeatures::contextMenuEnabled() && tagName ==
HTMLNames::menuitemTag)
On 2014/08/11 11:27:20, sanjoy.pal wrote:
> On 2014/08/11 10:23:01, tkent wrote:
> > Could you explain what part of the specification brings this change, and
what
> > behavior is changed?
> Specification:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#special
> 
> As per this section
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
> "Any other end tag"
> During end tag processing
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> if it is a special element, it should report parseError.

Unfortunately URLs are broken.  Please write explanation in the CL description.

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:803: if
((RuntimeEnabledFeatures::contextMenuEnabled() && token->name() == menuitemTag)
On 2014/08/11 11:27:20, sanjoy.pal wrote:
> On 2014/08/11 10:23:01, tkent wrote:
> > Could you explain what part of the specification brings this change and what
> > behavior is changed?
> Specification:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#ackno...
> "When an end tag token is emitted with its self-closing flag set, that is a
> parse error."
> <menuitem> has no end tag, its a self closing tag, and it should not contain
any
> inner node.

Unfortunately the URL is broken.  Please mention it in the CL description.

https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTheme.cpp (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTheme.cpp:423:
runtimeCSS.appendLiteral("menu[type=\"popup\" i] { display: none; }");
On 2014/08/11 11:27:21, sanjoy.pal wrote:
> On 2014/08/11 10:23:01, tkent wrote:
> > Could you explain what part of the specification brings this change?
> 
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#hi...

Thanks.  Please mention it in the CL description.

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/parse-menuitem.html (right):

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
LayoutTests/fast/parser/parse-menuitem.html:4: <script>
Please move this <script> element to the last of this document, and the code
doesn't need to be a onload handler.

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/stray-menuitem-expected.txt (right):

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
LayoutTests/fast/parser/stray-menuitem-expected.txt:1: PASS
Remove this file.

[pals, 2014-08-11 14:07:08]

Updated the CL description. Please take another look.

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLStackItem.h (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLStackItem.h:182: ||
(RuntimeEnabledFeatures::contextMenuEnabled() && tagName ==
HTMLNames::menuitemTag)
sorry, I copied the text of my previous comment which got the urls ellipsisized.
Anyway, I updated the CL description.

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/443373002/diff/80001/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:803: if
((RuntimeEnabledFeatures::contextMenuEnabled() && token->name() == menuitemTag)
sorry, I copied the text of my previous comment which got the urls ellipsisized.
Anyway, I updated the CL description.

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/parse-menuitem.html (right):

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
LayoutTests/fast/parser/parse-menuitem.html:4: <script>
On 2014/08/11 13:36:57, tkent wrote:
> Please move this <script> element to the last of this document, and the code
> doesn't need to be a onload handler.

Done.

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/stray-menuitem-expected.txt (right):

https://codereview.chromium.org/443373002/diff/100001/LayoutTests/fast/parser...
LayoutTests/fast/parser/stray-menuitem-expected.txt:1: PASS
On 2014/08/11 13:36:57, tkent wrote:
> Remove this file.

Done.

[tkent, 2014-08-11 16:01:02]

> Changes in /parser/HTMLTreeBuilder.cpp are required as per this part of
specification
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
> with section "Any other end tag"

Is it really "any other end tag"?  In the HTMLTreeBuilder.cpp change, I think
|token| is a start tag.

[pals, 2014-08-11 16:41:32]

On 2014/08/11 16:01:02, tkent wrote:
> > Changes in /parser/HTMLTreeBuilder.cpp are required as per this part of
> specification
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
> > with section "Any other end tag"
> 
> Is it really "any other end tag"?  In the HTMLTreeBuilder.cpp change, I think
> |token| is a start tag.

Yes, misplaced the comments. Modified correctly.

[tkent, 2014-08-11 16:55:35]

On 2014/08/11 16:41:32, sanjoy.pal wrote:
> On 2014/08/11 16:01:02, tkent wrote:
> > > Changes in /parser/HTMLTreeBuilder.cpp are required as per this part of
> > specification
> > >
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
> > > with section "Any other end tag"
> > 
> > Is it really "any other end tag"?  In the HTMLTreeBuilder.cpp change, I
think
> > |token| is a start tag.
> 
> Yes, misplaced the comments. Modified correctly.

I don't understand your explanation yet.

>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#the-s...
> "When an end tag token is emitted with its self-closing flag set, that is a
> parse error."

- I couldn't find the quoted sentence in section "12.2.3.2 The stack of open
elements."
- The quoted sentence is about end tag handling.  Your code change is about
start tag.

[tkent, 2014-08-11 16:56:13]

On 2014/08/11 16:55:35, tkent wrote:
> On 2014/08/11 16:41:32, sanjoy.pal wrote:
> > On 2014/08/11 16:01:02, tkent wrote:
> > > > Changes in /parser/HTMLTreeBuilder.cpp are required as per this part of
> > > specification
> > > >
> > >
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
> > > > with section "Any other end tag"
> > > 
> > > Is it really "any other end tag"?  In the HTMLTreeBuilder.cpp change, I
> think
> > > |token| is a start tag.
> > 
> > Yes, misplaced the comments. Modified correctly.
> 
> I don't understand your explanation yet.
> 
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#the-s...
> > "When an end tag token is emitted with its self-closing flag set, that is a
> > parse error."
> 
> - I couldn't find the quoted sentence in section "12.2.3.2 The stack of open
> elements."
> - The quoted sentence is about end tag handling.  Your code change is about
> start tag.

Also, what part of the specification says <menuitem> is self-closing?

[pals, 2014-08-11 17:11:47]

On 2014/08/11 16:56:13, tkent wrote:
> On 2014/08/11 16:55:35, tkent wrote:
> > On 2014/08/11 16:41:32, sanjoy.pal wrote:
> > > On 2014/08/11 16:01:02, tkent wrote:
> > > > > Changes in /parser/HTMLTreeBuilder.cpp are required as per this part
of
> > > > specification
> > > > >
> > > >
> > >
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
> > > > > with section "Any other end tag"
> > > > 
> > > > Is it really "any other end tag"?  In the HTMLTreeBuilder.cpp change, I
> > think
> > > > |token| is a start tag.
> > > 
> > > Yes, misplaced the comments. Modified correctly.
> > 
> > I don't understand your explanation yet.
> > 
> > >
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#the-s...
> > > "When an end tag token is emitted with its self-closing flag set, that is
a
> > > parse error."
> > 
> > - I couldn't find the quoted sentence in section "12.2.3.2 The stack of open
> > elements."
> > - The quoted sentence is about end tag handling.  Your code change is about
> > start tag
Its under this section
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#token...
> 
> Also, what part of the specification says <menuitem> is self-closing?

Specification says menuitem does not have an end tag which means its a self
closing tag similar to param, source, track.

[pals, 2014-08-11 18:14:45]

On 2014/08/11 16:55:35, tkent wrote:
> On 2014/08/11 16:41:32, sanjoy.pal wrote:
> > On 2014/08/11 16:01:02, tkent wrote:
> > > > Changes in /parser/HTMLTreeBuilder.cpp are required as per this part of
> > > specification
> > > >
> > >
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#parsi...
> > > > with section "Any other end tag"
> > > 
> > > Is it really "any other end tag"?  In the HTMLTreeBuilder.cpp change, I
> think
> > > |token| is a start tag.
> > 
> > Yes, misplaced the comments. Modified correctly.
> 
> I don't understand your explanation yet.

Please check this comment about insertSelfClosingHTMLElement here
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#the-s...
> > "When an end tag token is emitted with its self-closing flag set, that is a
> > parse error."
> 
> - I couldn't find the quoted sentence in section "12.2.3.2 The stack of open
> elements."
> - The quoted sentence is about end tag handling.  Your code change is about
> start tag.

[pals, 2014-08-12 04:09:36]

Sorry, I was editing from a mobile browser yesterday, I wrongly quoted the
statement about "When an end tag token ...". Updated the CL description now.

I'm not much familiar with the parser code. I understood the following from the
specification and code.
http://www.whatwg.org/specs/web-apps/current-work/#the-menuitem-element
<menuitem> specification says menuitem has no end tag.
My understanding is "void elements"
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#void-...
with no end tags are self closing tag. I may be wrong. As per my understanding
and I checked with "tasak@chromium.org" that tokens with no end tag are added by
insertSelfClosingHTMLElement(token). Normally HTMLElementStack is responsible
for calling finishParsingChildren,but self-closing elements are never in the
element stack so the stack doesn't get a chance to tell them that we're done
parsing their children.
http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html...

Changes in /parser/HTMLTreeBuilder.cpp are required as per this part of
specification
http://www.whatwg.org/specs/web-apps/current-work/multipage/syntax.html#token...
"When a start tag token is emitted with its self-closing flag set, if the flag
is not acknowledged when it is processed by the tree construction stage, that is
a parse error."
<menuitem> has no end tag, its a self closing tag, and it should not contain any
inner node, so adding token using insertSelfClosingHTMLElement().

[tkent, 2014-08-12 05:01:05]

ok, the current CL description sounds reasonable, and the code change looks to
follow the specification.  LGTM!

[tkent, 2014-08-12 05:01:42]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-08-12 05:02:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/443373002/120001

[commit-bot: I haz the power, 2014-08-12 05:47:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19389)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22186)

[commit-bot: I haz the power, 2014-08-12 06:50:08]

Change committed as 180037