RESOLVED FIXED 57433
clean up Document's handling of title changes 
https://bugs.webkit.org/show_bug.cgi?id=57433
Summary clean up Document's handling of title changes
Evan Martin
Reported 2011-03-30 02:16:36 PDT
clean up Document's handling of title changes
Attachments
Patch (7.50 KB, patch)
2011-03-30 02:34 PDT, Evan Martin 		rniwa: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Evan Martin
Comment 1 2011-03-30 02:34:35 PDT
Created attachment 87488 [details] Patch
Evan Martin
Comment 2 2011-03-30 02:39:07 PDT
Ryosuke, let me know if this is what you were suggesting I do.
Ryosuke Niwa
Comment 3 2011-03-30 03:34:59 PDT
Comment on attachment 87488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87488&action=review > Source/WebCore/dom/Document.cpp:1354 > + if (m_titleElement) Okay, so this code is valid because only HTMLTitleElement and SVGTitleElement calls setTitleElement with an arbitrary element but that's not entirely obvious to me. Please assert that m_titleElement->hasTagName(titleTag). But I'd also prefer to have the condition in the if statement as well because otherwise we'll be introducing a security vulnerability if we ever hit the assertion.
Evan Martin
Comment 4 2011-03-30 04:22:04 PDT
Committed r82422: <http://trac.webkit.org/changeset/82422>
WebKit Review Bot
Comment 5 2011-03-30 05:44:58 PDT
http://trac.webkit.org/changeset/82422 might have broken Windows XP Debug (Tests)
Alexey Proskuryakov
Comment 6 2011-03-30 09:53:47 PDT
+ if (!isHTMLDocument()) + m_titleElement = 0; What happens in the case of XHTML documents?
Evan Martin
Comment 7 2011-03-31 03:41:19 PDT
I'm not sure if your comment is about the prior code or the new code. I tried pretty hard to not change the behavior of the code, so please let me know if I got that wrong. As for the prior code: For some context, m_titleElement is attempting to track the element that we are deriving the Document's title from. That is important so its contents can be updated if JS does 'document.title="foo"'. We only learn about these title elements if the title element notifies the Document via setTitle("...", this) (this is the function I renamed to setTitleElement in this change). It seems isHTMLDocument() is false for XHTML documents, but they still use HTMLTitleElement? So in that case, the code you quoted clears the reference from Document to the element, and so document.title=... doesn't keep it in sync. I just verified this with a local xhtml file. That is to say, the following does something different in XHTML vs HTML. <head><title id='t'>test</title></head> <body> <script>document.title = 'foo'; console.log(document.getElementById('t').innerText);</script> Thankfully, Ryosuke had me add an extra bit of verification before I landed the patch, so at least I didn't make it worse. I will open a bug about it. https://bugs.webkit.org/show_bug.cgi?id=57537
Note You need to log in before you can comment on or make changes to this bug.