Last Comment Bug 710978 - Title attribute of a window is not updated in nsXULWindow::SetTitle
: Title attribute of a window is not updated in nsXULWindow::SetTitle
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
Depends on:
Blocks: 729474
  Show dependency treegraph
 
Reported: 2011-12-14 22:57 PST by Geoff Lankow (:darktrojan)
Modified: 2012-02-25 04:23 PST (History)
3 users (show)
geoff: in‑testsuite+
geoff: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.05 KB, patch)
2011-12-14 23:11 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
patch v2 (2.12 KB, patch)
2011-12-15 01:03 PST, Geoff Lankow (:darktrojan)
neil: review-
Details | Diff | Review
patch v3 (1.85 KB, patch)
2011-12-19 04:01 PST, Geoff Lankow (:darktrojan)
neil: review+
Details | Diff | Review
patch v4 (1.91 KB, patch)
2012-02-23 23:45 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review

Description Geoff Lankow (:darktrojan) 2011-12-14 22:57:25 PST
When a window's title is update by calling SetTitle, the DOM's title does not change. For example, in a view source window, the document element's title attribute is always "Nightly".

I've got a patch on Try now.
Comment 1 Geoff Lankow (:darktrojan) 2011-12-14 23:11:12 PST
Created attachment 581890 [details] [diff] [review]
patch

Appears to have made it through try without breaking anything, although only Linux64 opt has finished.
Comment 2 Boris Zbarsky [:bz] 2011-12-15 00:38:26 PST
Comment on attachment 581890 [details] [diff] [review]
patch

Hmm.  I'm not sure what the right behavior is here.  Neil?
Comment 3 Geoff Lankow (:darktrojan) 2011-12-15 01:03:15 PST
Created attachment 581911 [details] [diff] [review]
patch v2

Gah, forgot about the OS X quirk.
Comment 4 neil@parkwaycc.co.uk 2011-12-16 06:19:11 PST
Comment on attachment 581911 [details] [diff] [review]
patch v2

Sorry, but nsXULWindow::SetTitle is (eventually) how setting a XUL window element's title attribute updates the window's title, so it's fortunate that you were saved from an infinite loop.

If you really want the source window's chrome document to have the same title as the window, you need to find the code that's setting the window's title directly and change it to set the document's title instead. (And not by setting some random attribute either.)

And then SeaMonkey can remove its title setting hack in its tabbrowser.
Comment 5 Geoff Lankow (:darktrojan) 2011-12-19 04:01:29 PST
Created attachment 582785 [details] [diff] [review]
patch v3

Is this more like it?
Comment 6 neil@parkwaycc.co.uk 2011-12-19 05:34:45 PST
(In reply to comment #4)
> And then SeaMonkey can remove its title setting hack in its tabbrowser.
Which this patch will in fact break...
Comment 7 neil@parkwaycc.co.uk 2011-12-19 09:01:24 PST
(In reply to comment #6)
> (In reply to comment #4)
> > And then SeaMonkey can remove its title setting hack in its tabbrowser.
> Which this patch will in fact break...
Quite badly, in fact :-(
Comment 8 neil@parkwaycc.co.uk 2011-12-19 16:41:22 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > And then SeaMonkey can remove its title setting hack in its tabbrowser.
> > Which this patch will in fact break...
> Quite badly, in fact :-(
So, what happens is that this call to SetTitle is asynchronous because it happens at an unsafe time. The subsequent call in SeaMonkey's tabbrowser to set the XUL window title is still synchronous, so it gets clobbered by the asynchronous title update that this patch generates. Fixing tabbrowser to set the document title will simply coalesce the asynchronous title updates.
Comment 9 neil@parkwaycc.co.uk 2011-12-19 16:45:24 PST
(In reply to comment #8)
> So, what happens is that this call to SetTitle is asynchronous because it
> happens at an unsafe time.
Actually that's not true either. Calls to nsXULWindow::SetTitle synchronously update the title (but there's no way of knowing what it is set to, except by the user looking at the screen). Calls to nsDocument::SetTitle synchronously update its title, but then the chrome/content tree owner gets notified asynchronously, and then a DOM title changed event is also fired. The chrome tree owner simply updates the window title, while the content tree owner calls through into this code to set the chrome document's title.
Comment 10 neil@parkwaycc.co.uk 2011-12-19 16:53:08 PST
Comment on attachment 582785 [details] [diff] [review]
patch v3

r=me if you give me a chance to fix SeaMonkey when you check this in.
Comment 11 Serge Gautherie (:sgautherie) 2012-02-23 08:41:26 PST
Comment on attachment 582785 [details] [diff] [review]
patch v3

Review of attachment 582785 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/viewsource/test/browser/browser_bug699356.js
@@ +10,5 @@
>      let gBrowser = aWindow.gBrowser;
>  
>      is(gBrowser.contentDocument.title, source, "Correct document title");
> +    is(aWindow.document.documentElement.getAttribute("title"),
> +      "Source of: " + source + ("nsILocalFileMac" in Components.interfaces ? "" : " - Nightly"),

Isn't "Nightly" Firefox specific?

I suggest code (taken from bug 729474 patch Av1) using indexOf() like
+        isnot(title.indexOf(aEvent.accessible.name), -1,
+              "Window title contains the name of active tab document" +
+              " (Is '" + aEvent.accessible.name + "' in '" + title + "'?)");
Comment 12 Geoff Lankow (:darktrojan) 2012-02-23 23:45:50 PST
Created attachment 600323 [details] [diff] [review]
patch v4

This is more like it. Tested it on Try.
Comment 13 Geoff Lankow (:darktrojan) 2012-02-24 00:52:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb48e7c6aef1
Comment 14 Marco Bonardo [::mak] 2012-02-24 09:25:14 PST
https://hg.mozilla.org/mozilla-central/rev/bb48e7c6aef1
Comment 15 Jens Hatlak (:InvisibleSmiley) 2012-02-24 10:45:45 PST
(In reply to neil@parkwaycc.co.uk from comment #10)
> r=me if you give me a chance to fix SeaMonkey when you check this in.

Two questions:
1. where (bug #) will that happen?
2. will this bug fix finally fix the nasty bug where every browser window in DOMI is called "SeaMonkey"? I thought it was somehow impossible to fix, but maybe this bug made the difference.
Comment 16 Jens Hatlak (:InvisibleSmiley) 2012-02-24 12:11:17 PST
I'll answer them myself...

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #15)
> Two questions:
> 1. where (bug #) will that happen?

Already happened, and this bug was referenced:
http://hg.mozilla.org/comm-central/rev/61a16304321b

> 2. will this bug fix finally fix the nasty bug where every browser window in
> DOMI is called "SeaMonkey"?

Yes, this bug fixed that! :-)
Comment 17 Serge Gautherie (:sgautherie) 2012-02-25 04:23:19 PST
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #16)
> http://hg.mozilla.org/comm-central/rev/61a16304321b

V.Fixed wrt SeaMonkey part only, per bug 729474 comment 7 :-)

Note You need to log in before you can comment on or make changes to this bug.