Bug 12375 – REGRESSION(r19038): fast/dom/title-text-property-2.html failing

• Component: New Bugs
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

Test is failing with:

--- /Users/buildbot/Desktop/BuildData/WebKit-BuildSlave/post-commit-powerpc-mac-os-x/build/LayoutTests/fast/dom/title-text-property-2-expected.txt	2007-01-04 19:06:07.000000000 -0800
+++ layout-test-results/fast/dom/title-text-property-2-actual.txt	2007-01-22 20:40:27.000000000 -0800
@@ -1,3 +1,2 @@
 TITLE CHANGED: 1. setting document.title
-TITLE CHANGED: 2. setting title.text

[Mark Rowe (bdash)]

Created attachment 12619 [details]
Patch

This patch also addresses Ian Hickson's suggestion of matching the Web Applications 1.0 specification for handling of document.title.  Namely, we materialize a title element in the document's head when setting document.title if there is no existing title element.  See bug 11692, comment 10 for info.

Comment on attachment 12619 [details]
Patch

+Expected New document title, got Document title: FAIL

Expected results that say FAIL are confusing. Looking at the test, I can't figure out where that line comes from.

I think the tests belong in the Document subdirectory of fast/dom.

+    if (m_titleSetExplicitly && m_titleElement && isHTMLDocument() && m_titleElement->hasTagName(titleTag))
+        static_cast<HTMLTitleElement *>(m_titleElement.get())->setText(m_title);

Extra space before the *. Why does the above need to be in updateTitle() and not in the appropriate case in setTitle()?

+            if (Node *headElement = headElements->item(0)) {

* on the wrong side.

I think it's unfortunate that there's no easier way to get to the HEAD element. Isn't it guaranteed to be the first grandchild of the document, if it exists?

+                headElement->appendChild(m_titleElement, ec);

This ends up calling back into setTitle() as a result of the TITLE element being inserted into the document (so does the setText() in the case that the element already exists). Maybe a little more elegant design could have two separate functions on Document, one for JS to call when setting the attribute and one for TITLE elements to call when they're inserted or updated. If the "attribute setter" function decides to create/update a TITLE element, it will not have to do anything else since the other function will get called as a result. Then again, perhaps it's not worth the trouble.

[Darin Adler]

Comment on attachment 12619 [details]
Patch

Mitz's comments seem to justify a review-. Here are a few additional ones:

+            RefPtr<NodeList> headElements = getElementsByTagName("head");
+            if (Node *headElement = headElements->item(0)) {

I suggest adding a head() function alongside the body() function to encapsulate this.

Now that HTML and non-HTML are going to work differently as far as titles are concerned, we might need some SVG or XML test cases.

-        m_titleElement = 0;
+        if (!m_titleElement && isHTMLDocument()) {

In the non-HTML-document case, maybe we should be setting m_titleElement to 0 as before?

[Darin Adler]

P1 because we need layout tests passing again.

[Mark Rowe (bdash)]

Created attachment 12641 [details]
Patch v2

This includes a layout test for the handling of multiple SVG title elements.  document.title is read-only in the SVG DOM which means there are fewer cases to test.  I cannot see a specification for the expected behaviour when multiple title elements are present and the first (i.e., the one currently in use) is removed.

The biggest change in this patch is to removeTitle: we update the document title based on the first title element in the head element, if one exists.  This matches the current Web App 1.0 specification, but differs from both Opera and Firefox.  Opera will update the document title based on the first title element anywhere in the document, while Firefox continues using a title set via document.title even when the element it applied to was removed.

I believe I've addressed all the other review comments made by Darin + Mitz.

[Mark Rowe (bdash)]

Landed in r19070.