Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1752713002:
Fix SVGDocument return title that is not a child of the root element (Closed)
|
Created:
4 years, 9 months ago by hyunjunekim2 Modified:
4 years, 9 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix SVGDocument return title that is not a child of the root element
The title element should be a child of the root element.
But currently return invalid title that is not a child of the root element.
Because on getting, if the root element is an svg element in the SVG
namespace, then let value ba a concatenation of the data of all
the child Text nodes of the first title element in the SVG namespace
that is a child of the root element.[1]
And on setting, let element be the first title element in the SVG
namespace that is a child of the root element, if any.
If there isn't one, create a title element in the SVG namespace,
insert it as the first child of the root element, and let element
be that element.[1]
[1] https://html.spec.whatwg.org/multipage/dom.html#document.title
BUG=543061
Committed: https://crrev.com/2764e5d8a0ec6cf13cb4d918a5032e45da691247
Cr-Commit-Position: refs/heads/master@{#379301}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : Add test #
Total comments: 19
Patch Set 6 : Testing #Patch Set 7 : #
Total comments: 12
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Patch Set 10 : #
Total comments: 2
Patch Set 11 : #
Total comments: 7
Patch Set 12 : #
Messages
Total messages: 47 (31 generated)
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element the title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
hyunjune.kim@samsung.com changed reviewers: + fs@opera.com, pdr@chromium.org
pdr, fs Could you check this patch and give me advice? Thank you :)
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And when set title, if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one , create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one , create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one , create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one , create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one , create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one , create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't one, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't that, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09-expected.txt (right): https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09-expected.txt:5: FAIL Title element not in SVG namespace assert_equals: expected "" but got "foo" Why not look into fixing this too while we're at it? https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09-expected.txt:6: FAIL Root element not named "svg" assert_equals: expected 0 but got 1 This looks like a regression. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/svg-document-test.html (right): https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/svg-document-test.html:8: description("Tests that get and set document.title."); Not sure if this test tests anything that the web-platform-test (above) doesn't. If it does then please rewrite it to use testharness.js - else remove it. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1289: if (!isHTMLDocument() && !isXHTMLDocument() && !isSVGDocument()) { It would probably make sense to just reverse these and get a structure similar to that in setTitleElement below. if (!m_titleElement) { if (isHTMLDocument() || isXHTMLDocument()) { ... } else if (isSVGDocument()) { ... } } else { ... } https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1294: documentElement()->insertBefore(m_titleElement.get(), documentElement()->firstChild()); Check that documentElement() exists. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1307: toSVGTitleElement(m_titleElement)->setTextContent(title); Don't we need protection against mutation here? (Like HTMLTitleElement::setText) https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1329: SVGElement* rootElement = toSVGElement(documentElement()); Dangerous (but unnecessary) https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1330: // If the root element is an svg element, the first title element in the SVG namespace It looks like a better fit to do this above? https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1333: for (SVGElement* child = Traversal<SVGElement>::firstChild(*rootElement); child; child = Traversal<SVGElement>::nextSibling(*child)) { This appears to be a very long-winded way of checking if: isSVGSVGElement(m_titleElement->parentNode()) is true. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1335: updateTitle(toSVGTitleElement(m_titleElement)->textContent()); Spec says: "If the root element is an svg element in the SVG namespace, then let value be a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element. [SVG]" which differs from what this does.
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't that, create a title element as first child of the root element. #https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't that, create a title element as first child of the root element. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't that, create a title element as first child of the root element. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. If the root element is an svg element in And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. If the root element is an svg element in And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. If the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because when set title element, check whether is a child of the root element or not. If the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, if there isn't that, create a title element as first child of the root element[1]. [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, Let element be the first title element in the SVG namespace that is a child of the root element, if any. If there isn't one, create a title element in the SVG namespace, insert it as the first child of the root element, and let element be that element.[1] [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, Let element be the first title element in the SVG namespace that is a child of the root element, if any. If there isn't one, create a title element in the SVG namespace, insert it as the first child of the root element, and let element be that element.[1] [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, let element be the first title element in the SVG namespace that is a child of the root element, if any. If there isn't one, create a title element in the SVG namespace, insert it as the first child of the root element, and let element be that element.[1] [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
fs, pdr Could you check PS7? Thank you. :) https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09-expected.txt (right): https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09-expected.txt:5: FAIL Title element not in SVG namespace assert_equals: expected "" but got "foo" On 2016/03/01 15:49:03, fs wrote: > Why not look into fixing this too while we're at it? Done. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09-expected.txt:6: FAIL Root element not named "svg" assert_equals: expected 0 but got 1 On 2016/03/01 15:49:03, fs wrote: > This looks like a regression. Done. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/dom/svg-document-test.html (right): https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/dom/svg-document-test.html:8: description("Tests that get and set document.title."); On 2016/03/01 15:49:03, fs wrote: > Not sure if this test tests anything that the web-platform-test (above) doesn't. > If it does then please rewrite it to use testharness.js - else remove it. Done.removed this test, and added mutation test for title of svg document. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1289: if (!isHTMLDocument() && !isXHTMLDocument() && !isSVGDocument()) { On 2016/03/01 15:49:03, fs wrote: > It would probably make sense to just reverse these and get a structure similar > to that in setTitleElement below. > > if (!m_titleElement) { > if (isHTMLDocument() || isXHTMLDocument()) { > ... > } else if (isSVGDocument()) { > ... > } > } else { > ... > } Done. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1294: documentElement()->insertBefore(m_titleElement.get(), documentElement()->firstChild()); On 2016/03/01 15:49:03, fs wrote: > Check that documentElement() exists. Done. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1307: toSVGTitleElement(m_titleElement)->setTextContent(title); On 2016/03/01 15:49:03, fs wrote: > Don't we need protection against mutation here? (Like HTMLTitleElement::setText) Done. made SVGTitleElement::setText that referred to HTMLTitleElement::setText. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1329: SVGElement* rootElement = toSVGElement(documentElement()); On 2016/03/01 15:49:03, fs wrote: > Dangerous (but unnecessary) Done. Removed it. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1333: for (SVGElement* child = Traversal<SVGElement>::firstChild(*rootElement); child; child = Traversal<SVGElement>::nextSibling(*child)) { On 2016/03/01 15:49:03, fs wrote: > This appears to be a very long-winded way of checking if: > > isSVGSVGElement(m_titleElement->parentNode()) > > is true. Done. https://codereview.chromium.org/1752713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1335: updateTitle(toSVGTitleElement(m_titleElement)->textContent()); On 2016/03/01 15:49:03, fs wrote: > Spec says: > > "If the root element is an svg element in the SVG namespace, then let value be a > concatenation of the data of all the child Text nodes of the first title element > in the SVG namespace that is a child of the root element. [SVG]" > > which differs from what this does. Done. passed document.title-09.html.
https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/dom/svg-document-set-title-mutations.html (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/dom/svg-document-set-title-mutations.html:7: function newSVGDocument() { This is only used once, so just do this directly where it's called. Similarly for SVG_NAMESPACE. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1299: if (documentElement()) { Here we know that documentElement() is non-null already (isSVGSVGElement checks that.) So, I' d suggest that you try to structure this code like the HTML case: Element* documentElement = documentElement(); if (!isSVGSVGElement(documentElement)) return; m_titleElement = ...; documentElement->...; https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1332: if (isSVGDocument() && isSVGSVGElement(documentElement())) { I'd suggest that you separate the calls to updateTitle() from the logic that determine if they should be called at all. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGTitleElement.cpp (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGTitleElement.cpp:62: void SVGTitleElement::setText(const String &value) String& (not String &) https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGTitleElement.cpp:68: m_ignoreTitleUpdatesWhenChildrenChange = !value.isEmpty(); Could also do this as: { // ... TemporaryChange<bool> inhibitTitleUpdateScope(m_ignoreTitleUpdatesWhenChildrenChange, !value.isEmpty()); removeChildren(...); } https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGTitleElement.h (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGTitleElement.h:34: private: Add space between the above two lines.
fs, pdr Could you check PS8? Thank you. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/dom/svg-document-set-title-mutations.html (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/dom/svg-document-set-title-mutations.html:7: function newSVGDocument() { On 2016/03/03 15:49:44, fs wrote: > This is only used once, so just do this directly where it's called. Similarly > for SVG_NAMESPACE. Done. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1299: if (documentElement()) { On 2016/03/03 15:49:44, fs wrote: > Here we know that documentElement() is non-null already (isSVGSVGElement checks > that.) So, I' > d suggest that you try to structure this code like the HTML case: > > Element* documentElement = documentElement(); > if (!isSVGSVGElement(documentElement)) > return; > m_titleElement = ...; > documentElement->...; Done. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1332: if (isSVGDocument() && isSVGSVGElement(documentElement())) { On 2016/03/03 15:49:44, fs wrote: > I'd suggest that you separate the calls to updateTitle() from the logic that > determine if they should be called at all. Done. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGTitleElement.cpp (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGTitleElement.cpp:62: void SVGTitleElement::setText(const String &value) On 2016/03/03 15:49:44, fs wrote: > String& (not String &) Done. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGTitleElement.cpp:68: m_ignoreTitleUpdatesWhenChildrenChange = !value.isEmpty(); On 2016/03/03 15:49:45, fs wrote: > Could also do this as: > > { > // ... > TemporaryChange<bool> > inhibitTitleUpdateScope(m_ignoreTitleUpdatesWhenChildrenChange, > !value.isEmpty()); > removeChildren(...); > } Done. https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGTitleElement.h (right): https://codereview.chromium.org/1752713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGTitleElement.h:34: private: On 2016/03/03 15:49:45, fs wrote: > Add space between the above two lines. Done.
https://codereview.chromium.org/1752713002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1329: String titleText; This is not quite what I meant... (note that now you'll update the title always.) More like: if (<is in SVG document but title structure is not proper>) m_titleElement = nullptr; if (isHTMLTitleElement(m_titleElement)) updateTitle(...); ...like before your change...
fs, Could you check condition? Thank you. https://codereview.chromium.org/1752713002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1329: String titleText; On 2016/03/04 11:47:06, fs wrote: > This is not quite what I meant... (note that now you'll update the title > always.) > > More like: > > if (<is in SVG document but title structure is not proper>) > m_titleElement = nullptr; > > if (isHTMLTitleElement(m_titleElement)) > updateTitle(...); > ...like before your change... Done.
https://codereview.chromium.org/1752713002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1329: if (isSVGDocument() && ((isSVGSVGElement(documentElement()) && isHTMLTitleElement(m_titleElement)) I think a comment/spec quote here would be reasonable. Probably also to split this condition into two (or more) ifs: if (isSVGDocument()) { if (...) ... } Also, I don't think you ever need to accept a <title> that is not in the SVG-namespace in a SVG document?
fs, Could you check PS 11? https://codereview.chromium.org/1752713002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1329: if (isSVGDocument() && ((isSVGSVGElement(documentElement()) && isHTMLTitleElement(m_titleElement)) On 2016/03/04 13:11:08, fs wrote: > I think a comment/spec quote here would be reasonable. Probably also to split > this condition into two (or more) ifs: > > if (isSVGDocument()) { > if (...) > ... > } > > Also, I don't think you ever need to accept a <title> that is not in the > SVG-namespace in a SVG document? Done. https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1332: if (isSVGSVGElement(documentElement()) && documentElement() != m_titleElement->parentNode()) { Added condition whether the root or not. https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1340: if ((!isSVGSVGElement(m_titleElement->parentNode()) && isSVGTitleElement(m_titleElement)) Has two case, First condition, the root element is not svg element and title element is in svg namespace, Test) var doc = document.implementation.createDocument(SVG_NAMESPACE, "SVG", null); doc.documentElement.appendChild(doc.createElementNS(SVG_NAMESPACE, "title")); doc.documentElement.lastChild.textContent = "foo"; assert_equals(doc.title, ""); // But an HTML title is respected doc.documentElement.appendChild(doc.createElementNS(HTML_NAMESPACE, "title")); doc.documentElement.lastChild.textContent = "bar"; assert_equals(doc.title, "bar"); Second condition, the root element is svg element and title element is not in svg namespace, var doc = newSVGDocument(); var title = doc.createElementNS(HTML_NAMESPACE, "title"); title.textContent = "foo"; doc.documentElement.appendChild(title); assert_equals(doc.title, "");
LGTM w/ nits https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1331: // If the root element that is svg element is not the parent of title element, ignore title element. Nit: I'd suggest that you change "svg element" to "<svg> element" to make it even more clear that we're talking about <svg> specifically and not "a svg element" (element in the svg namespace) in general. https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1337: // If the root element is not svg element and title element is in svg namespace, Same here. https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1340: if ((!isSVGSVGElement(m_titleElement->parentNode()) && isSVGTitleElement(m_titleElement)) On 2016/03/04 at 14:42:17, hyunjunekim2 wrote: > Has two case, > First condition, the root element is not svg element and title element is in svg namespace, > Test) > var doc = document.implementation.createDocument(SVG_NAMESPACE, "SVG", null); > doc.documentElement.appendChild(doc.createElementNS(SVG_NAMESPACE, "title")); > doc.documentElement.lastChild.textContent = "foo"; > assert_equals(doc.title, ""); > > // But an HTML title is respected > doc.documentElement.appendChild(doc.createElementNS(HTML_NAMESPACE, "title")); > doc.documentElement.lastChild.textContent = "bar"; > assert_equals(doc.title, "bar"); > > Second condition, the root element is svg element and title element is not in svg namespace, > var doc = newSVGDocument(); > var title = doc.createElementNS(HTML_NAMESPACE, "title"); > title.textContent = "foo"; > doc.documentElement.appendChild(title); > assert_equals(doc.title, ""); Acknowledged.
fs, Could you check PS12? Thank you. https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1331: // If the root element that is svg element is not the parent of title element, ignore title element. On 2016/03/04 14:57:12, fs wrote: > Nit: I'd suggest that you change "svg element" to "<svg> element" to make it > even more clear that we're talking about <svg> specifically and not "a svg > element" (element in the svg namespace) in general. Done. https://codereview.chromium.org/1752713002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1337: // If the root element is not svg element and title element is in svg namespace, On 2016/03/04 14:57:12, fs wrote: > Same here. Done.
lgtm
The CQ bit was checked by hyunjune.kim@samsung.com
The CQ bit was unchecked by hyunjune.kim@samsung.com
The CQ bit was checked by hyunjune.kim@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752713002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752713002/220001
Message was sent while issue was closed.
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, let element be the first title element in the SVG namespace that is a child of the root element, if any. If there isn't one, create a title element in the SVG namespace, insert it as the first child of the root element, and let element be that element.[1] [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, let element be the first title element in the SVG namespace that is a child of the root element, if any. If there isn't one, create a title element in the SVG namespace, insert it as the first child of the root element, and let element be that element.[1] [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, let element be the first title element in the SVG namespace that is a child of the root element, if any. If there isn't one, create a title element in the SVG namespace, insert it as the first child of the root element, and let element be that element.[1] [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 ========== to ========== Fix SVGDocument return title that is not a child of the root element The title element should be a child of the root element. But currently return invalid title that is not a child of the root element. Because on getting, if the root element is an svg element in the SVG namespace, then let value ba a concatenation of the data of all the child Text nodes of the first title element in the SVG namespace that is a child of the root element.[1] And on setting, let element be the first title element in the SVG namespace that is a child of the root element, if any. If there isn't one, create a title element in the SVG namespace, insert it as the first child of the root element, and let element be that element.[1] [1] https://html.spec.whatwg.org/multipage/dom.html#document.title BUG=543061 Committed: https://crrev.com/2764e5d8a0ec6cf13cb4d918a5032e45da691247 Cr-Commit-Position: refs/heads/master@{#379301} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2764e5d8a0ec6cf13cb4d918a5032e45da691247 Cr-Commit-Position: refs/heads/master@{#379301}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1771703003/ by hyunjune.kim@samsung.com. The reason for reverting is: Crash in blink:Document::setTitleElement. Issue :https://bugs.chromium.org/p/chromium/issues/detail?id=592323. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
