Document.title getter should return text of title element and setter should stop when head el…

Document.title getter should return text of title element and setter should stop when head element is null

SPEC: http://www.whatwg.org/specs/web-apps/current-work/#document.title

This CL allowed implicit title set from Document::setTitleElement()
and stopped operation on setter when head element is null as per specification.

In addition, with this change, failed test cases will be passed in
http://w3c-test.org/html/dom/documents/dom-tree-accessors/document.title-01.html

Behavior in other browsers:
*)FF: PASS
*)IE: PASS

Plus, this CL deleted obsolete title argument from Document::setTitleElement.

TEST=LayoutTests/fast/dom/Document/title-01.html
    ,LayoutTests/svg/custom/multiple-title-elements.svg

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

[Inactive, 2014-07-16 14:42:05]

Some initial feedback.

https://codereview.chromium.org/384413003/diff/40001/LayoutTests/svg/custom/m...
File LayoutTests/svg/custom/multiple-title-elements.svg (right):

https://codereview.chromium.org/384413003/diff/40001/LayoutTests/svg/custom/m...
LayoutTests/svg/custom/multiple-title-elements.svg:43: expect('',
document.title);
Why this behavior change for SVG?

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1393: if (!isSVGDocument() && !headElement)
This !isSVGDocument() check seems superfluous. If it were an SVGDocument, it
would execute the if case, not the else if one I believe.

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1405: void Document::setTitleElement(const String&
title, Element* titleElement)
It looks like you broke the SVG case in this method. You now only handle
HTMLTitleElement properly. How about SVGTitleElement? I think this is why your
svg test cases are failing.

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1409: m_titleElement =
Traversal<HTMLTitleElement>::next(*this);
Wouldn't Traversal<HTMLTitleElement>::firstWithin(*this) be clearer?

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1426: if (HTMLTitleElement* title =
Traversal<HTMLTitleElement>::next(*this))
firstWithin?

[kangil_, 2014-07-17 00:08:43]

On 2014/07/16 14:42:05, Chris Dumez wrote:
> Some initial feedback.

Thanks Chris for initial feedback. I will look at it after fixing blocking issue
for this CL.
Seems this isn't the first try to follow DOM spec.

https://codereview.chromium.org/98373012 tried but broke
ClickToPlayPluginTest.NoCallbackAtLoad
so reverted by https://codereview.chromium.org/109823011.

I don't think I can proceed this patch without making that test case green with
this CL.
(Actual blocking point to me is that I don't have any method to reproduce that
issue on linux build :/)

[kangil_, 2014-07-17 06:52:27]

FWIW, dom tree for ClickToPlayPluginTest.NoCallbackAtLoad test case.

*#document	0x53614010
	HTML	0x53608118
		BODY	0x53608170 STYLE="background-color: rgb(38,38,38)"
			EMBED	0x53618010
				#document-fragment	0x53628010
					CONTENT	0x53634010

[kangil_, 2014-07-17 07:21:21]

This CL depends on https://codereview.chromium.org/400523004/

[kangil_, 2014-07-18 06:55:58]

PTAL

https://codereview.chromium.org/384413003/diff/40001/LayoutTests/svg/custom/m...
File LayoutTests/svg/custom/multiple-title-elements.svg (right):

https://codereview.chromium.org/384413003/diff/40001/LayoutTests/svg/custom/m...
LayoutTests/svg/custom/multiple-title-elements.svg:43: expect('',
document.title);
On 2014/07/16 14:42:05, Chris Dumez wrote:
> Why this behavior change for SVG?

Done.

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1393: if (!isSVGDocument() && !headElement)
On 2014/07/16 14:42:05, Chris Dumez wrote:
> This !isSVGDocument() check seems superfluous. If it were an SVGDocument, it
> would execute the if case, not the else if one I believe.

Done.

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1405: void Document::setTitleElement(const String&
title, Element* titleElement)
On 2014/07/16 14:42:05, Chris Dumez wrote:
> It looks like you broke the SVG case in this method. You now only handle
> HTMLTitleElement properly. How about SVGTitleElement? I think this is why your
> svg test cases are failing.

Done.

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1409: m_titleElement =
Traversal<HTMLTitleElement>::next(*this);
On 2014/07/16 14:42:05, Chris Dumez wrote:
> Wouldn't Traversal<HTMLTitleElement>::firstWithin(*this) be clearer?

Done.

https://codereview.chromium.org/384413003/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1426: if (HTMLTitleElement* title =
Traversal<HTMLTitleElement>::next(*this))
On 2014/07/16 14:42:05, Chris Dumez wrote:
> firstWithin?

Done.

[kangil_, 2014-07-21 12:47:39]

@Chris: Gentle ping. :)

[Inactive, 2014-07-22 00:55:23]

I'll let tkent review this as I don't feel confident doing it.

https://codereview.chromium.org/384413003/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/384413003/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:1405: void Document::setTitleElement(const String&
title, Element* titleElement)
Hmm, this title argument is no longer used. We should remove it. Also, the
method name name is slightly misleading now as the behavior has changed, maybe
updateTitleElementIfNeeded()? tkent may have a better suggestion.

[tkent, 2014-07-22 03:10:54]

Need a test for SVG document too.

dpranke, this CL tries to import a single W3C test to fast/dom/Document/. 
Should we enable w3c/web-platform-tests/html in W3CImportExpectations instead?

[Dirk Pranke, 2014-07-22 19:11:47]

On 2014/07/22 03:10:54, tkent wrote:
> Need a test for SVG document too.
> 
> dpranke, this CL tries to import a single W3C test to fast/dom/Document/. 
> Should we enable w3c/web-platform-tests/html in W3CImportExpectations instead?

I'm in the middle of changing how we import the tests this week (see
https://code.google.com/p/chromium/issues/detail?id=395862), so now would be a
bad time to try and import more tests. I'd either wait until I'm done w/ that
bug, at which point adding html should be straightforward, or just add the one
test for now and remember to come back and delete it later when we do import
html.

[kangil_, 2014-07-23 01:52:54]

On 2014/07/22 19:11:47, Dirk Pranke wrote:
> On 2014/07/22 03:10:54, tkent wrote:
> > Need a test for SVG document too.

Done. PTAL.

> > 
> > dpranke, this CL tries to import a single W3C test to fast/dom/Document/. 
> > Should we enable w3c/web-platform-tests/html in W3CImportExpectations
instead?
> 
> I'm in the middle of changing how we import the tests this week (see
> https://code.google.com/p/chromium/issues/detail?id=395862), so now would be a
> bad time to try and import more tests. I'd either wait until I'm done w/ that
> bug, at which point adding html should be straightforward, or just add the one
> test for now and remember to come back and delete it later when we do import
> html.

I will delete title-01.html and title-01-expected.txt when import is done. :)

[tkent, 2014-07-25 00:36:42]

On 2014/07/23 01:52:54, kangil_ wrote:
> On 2014/07/22 19:11:47, Dirk Pranke wrote:
> > On 2014/07/22 03:10:54, tkent wrote:
> > > Need a test for SVG document too.
> 
> Done. PTAL.
> 
> > > 
> > > dpranke, this CL tries to import a single W3C test to fast/dom/Document/. 
> > > Should we enable w3c/web-platform-tests/html in W3CImportExpectations
> instead?
> > 
> > I'm in the middle of changing how we import the tests this week (see
> > https://code.google.com/p/chromium/issues/detail?id=395862), so now would be
a
> > bad time to try and import more tests. I'd either wait until I'm done w/
that
> > bug, at which point adding html should be straightforward, or just add the
one
> > test for now and remember to come back and delete it later when we do import
> > html.

Dirk, thank you for the information.  According to the latest post on blink-dev,
it seems we need some time to complete it. So, importing a test in this CL and
deleting it later is ok.

> I will delete title-01.html and title-01-expected.txt when import is done. :)

Please file a bug about deleting title-01.html.

[Dirk Pranke, 2014-07-25 00:37:32]

On 2014/07/25 00:36:42, tkent wrote:
> On 2014/07/23 01:52:54, kangil_ wrote:
> > On 2014/07/22 19:11:47, Dirk Pranke wrote:
> > > On 2014/07/22 03:10:54, tkent wrote:
> > > > Need a test for SVG document too.
> > 
> > Done. PTAL.
> > 
> > > > 
> > > > dpranke, this CL tries to import a single W3C test to
fast/dom/Document/. 
> > > > Should we enable w3c/web-platform-tests/html in W3CImportExpectations
> > instead?
> > > 
> > > I'm in the middle of changing how we import the tests this week (see
> > > https://code.google.com/p/chromium/issues/detail?id=395862), so now would
be
> a
> > > bad time to try and import more tests. I'd either wait until I'm done w/
> that
> > > bug, at which point adding html should be straightforward, or just add the
> one
> > > test for now and remember to come back and delete it later when we do
import
> > > html.
> 
> Dirk, thank you for the information.  According to the latest post on
blink-dev,
> it seems we need some time to complete it. So, importing a test in this CL and
> deleting it later is ok.

Yup. Thanks!

-- Dirk

[tkent, 2014-07-25 00:37:44]

lgmt.

TEST= line should be at the last part of the description.

[tkent, 2014-07-25 00:37:58]

On 2014/07/25 00:37:44, tkent wrote:
> lgmt.

oops, lgtm :-)

[kangil_, 2014-07-25 01:00:13]

On 2014/07/25 00:36:42, tkent wrote:
> Please file a bug about deleting title-01.html.

Done at https://code.google.com/p/chromium/issues/detail?id=397370

[kangil_, 2014-07-25 01:02:03]

On 2014/07/25 00:37:44, tkent wrote:
> lgmt.
> 
> TEST= line should be at the last part of the description.

Done, thanks!

[kangil_, 2014-07-25 01:02:14]

The CQ bit was checked by kangil.han@samsung.com

[commit-bot: I haz the power, 2014-07-25 01:02:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kangil.han@samsung.com/384413003/80001

[commit-bot: I haz the power, 2014-07-25 02:17:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-07-25 02:34:04]

Change committed as 178889