Last Comment Bug 457548 - Window titles are incorrect on mac
: Window titles are incorrect on mac
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: seamonkey2.0b2
Assigned To: Stefan [:stefanh]
:
Mentors:
Depends on: 462997 591550
Blocks: 56995
  Show dependency treegraph
 
Reported: 2008-09-28 09:04 PDT by Stefan [:stefanh]
Modified: 2010-08-27 19:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1: fix tabbrowser (1.39 KB, patch)
2008-09-28 09:16 PDT, Stefan [:stefanh]
neil: superreview+
Details | Diff | Splinter Review
Composer fix (pushed) (1.68 KB, patch)
2008-10-04 07:55 PDT, Stefan [:stefanh]
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Fix debugQA (1.31 KB, patch)
2008-10-04 08:48 PDT, Stefan [:stefanh]
neil: superreview+
Details | Diff | Splinter Review
editor.xul followup per comment #20 (pushed) (882 bytes, patch)
2008-10-07 09:58 PDT, Stefan [:stefanh]
neil: review+
neil: superreview+
Details | Diff | Splinter Review
New attempt (4.06 KB, patch)
2009-08-09 13:07 PDT, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
Another attempt (9.43 KB, patch)
2009-08-22 05:33 PDT, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
new attempt - fix tabbrowser (7.93 KB, patch)
2009-08-23 05:39 PDT, Stefan [:stefanh]
neil: superreview-
Details | Diff | Splinter Review
New version (7.02 KB, patch)
2009-08-24 12:16 PDT, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
Another version (8.09 KB, patch)
2009-08-24 13:12 PDT, Stefan [:stefanh]
mnyromyr: review+
neil: superreview+
Details | Diff | Splinter Review

Description Stefan [:stefanh] 2008-09-28 09:04:52 PDT
You'll see this if you disable debugQA, has been like this since we became a MOZ_XUL_APP.

- Browser window titles are "PageTitle - " (used to be PageTitle - SeaMonkey")
- Composer window titles are "PageTitle - " (used to be PageTitle - Composer")

The reason for this is can be found in http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#913.

Note that displaying the appName in window titles is not correct on mac, so we don't want the old behaviour.
Comment 1 Stefan [:stefanh] 2008-09-28 09:16:18 PDT
Created attachment 340807 [details] [diff] [review]
Part 1: fix tabbrowser

I'm going to do this in parts. This is the tabbrowser part - it basically makes us behave the same as firefox.
Comment 2 Stefan [:stefanh] 2008-09-28 09:19:41 PDT
For editor, I think we have 2 choices:

1) Display "PageTitle - Composer"
2) Display "Edit: PageTitle"

The point here is to distinguish from a page loaded in Composer and a page loaded in the browser - so I don't think we should go with just displaying "PageTitle" on mac.

Thoughts?
Comment 3 Karsten Düsterloh 2008-09-28 12:00:14 PDT
> 1) Display "PageTitle - Composer"
> 2) Display "Edit: PageTitle"

I'd vote for (1), 'cos ":" in titles is ugly.
Comment 4 neil@parkwaycc.co.uk 2008-09-28 12:02:43 PDT
Comment on attachment 340807 [details] [diff] [review]
Part 1: fix tabbrowser

Don't you want to get someone to test this on a Mac?
Comment 5 Stefan [:stefanh] 2008-09-28 12:22:04 PDT
Comment on attachment 340807 [details] [diff] [review]
Part 1: fix tabbrowser

(In reply to comment #4)
> (From update of attachment 340807 [details] [diff] [review])
> Don't you want to get someone to test this on a Mac?
Right, sorry. Karsten: in order to test this you need to disable debugQA.
Comment 6 Stefan [:stefanh] 2008-09-28 12:26:42 PDT
(In reply to comment #3)
> > 1) Display "PageTitle - Composer"
> > 2) Display "Edit: PageTitle"
> 
> I'd vote for (1), 'cos ":" in titles is ugly.

We have "Compose: Subject". My idea was that having "Edit: PageTitle" sort of refers to that you can do something with the page that you have loaded.
Comment 7 Stefan [:stefanh] 2008-09-29 14:58:51 PDT
I do notice that with debugQA we don't seem to update the title that well (before and after the patch):

1) Load mozilla.org
2) Open a new, blank tab
3) Switch to mozilla.org
4) Switch back to the blank tab

But I guess this is known?
Comment 8 neil@parkwaycc.co.uk 2008-10-01 01:41:17 PDT
Works fine for me in Windows with or without the patch.
Comment 9 Stefan [:stefanh] 2008-10-01 10:50:54 PDT
With the patch, window title in 2) is "SeaMonkey", window title in 4) is "SeaMonkey - SeaMonkey {...}"

With an unpatched trunk build 2) is the same, but 4) is OK (only "SeaMonkey {...}")
Comment 10 Stefan [:stefanh] 2008-10-01 12:58:07 PDT
Hmm, looking at the js in debugQANavigatorOverlay.xul, maybe I can remove the titledefault attribute there if I don't have any titlemodifier attribute...
Comment 11 neil@parkwaycc.co.uk 2008-10-01 13:48:23 PDT
(In reply to comment #9)
> With the patch, window title in 2) is "SeaMonkey", window title in 4) is
> "SeaMonkey - SeaMonkey {...}"
> 
> With an unpatched trunk build 2) is the same, but 4) is OK (only "SeaMonkey
> {...}")
Correct for both is "SeaMonkey {...}" (which flashs up very briefly for me)
Comment 12 Stefan [:stefanh] 2008-10-01 14:50:22 PDT
ajschult tells me that a new, blank tab just displays "SeaMonkey" in the title (release build with debugQA installed).
Comment 13 Stefan [:stefanh] 2008-10-04 07:55:23 PDT
Created attachment 341750 [details] [diff] [review]
Composer fix (pushed)

While I'm thinking of how to handle debugQA - here's a patch for editor (should have the same impact on win/mac/nix). It seems to me that SetDocumentTitle(title) is never called when you switch between normal and html source mode in composer. My solution here is to always call the function (not sure how good that is). Since I think we want "- Composer" to be appended to the title and titlemodifier is "Composer", contentsetting="false" looks like the best way here.
Comment 14 Stefan [:stefanh] 2008-10-04 08:48:34 PDT
Created attachment 341753 [details] [diff] [review]
Fix debugQA

This fixes 4) for me. What happens for me is that without any page title, I get both attribute values in the title - titlemodifier is removed, but debugQA adds it back.
Comment 15 neil@parkwaycc.co.uk 2008-10-04 11:58:38 PDT
Comment on attachment 341750 [details] [diff] [review]
Composer fix (pushed)

>+        SetDocumentTitle(title);        
Nit: trailing spaces. And there weren't any before, so you've no excuse!

>+        contenttitlesetting="false"
Nit: you can probably simply delete this attribute.
Comment 16 Stefan [:stefanh] 2008-10-05 06:32:23 PDT
Comment on attachment 341750 [details] [diff] [review]
Composer fix (pushed)

Pushed (without whitespace and contenttitlesetting attribute):
http://hg.mozilla.org/comm-central/rev/49fff21693a1
Comment 17 Stefan [:stefanh] 2008-10-05 06:59:35 PDT
Comment on attachment 341753 [details] [diff] [review]
Fix debugQA

This one assumes that attachment #340807 [details] [diff] [review] has been applied. This fixes 4) for me. I have no solution for 2), but I think that can go in another bug since the behaviour has been there for a while. One interesting thing is that if you open a popup window with 'javascript:window.open("","","menubar=no,location=no,resizable=yes,scrollbars=no,status=no");' , I get "about:blank - SeaMonkey" in the titlebar (without debugQA). Looking at the code, the behavour looks expected. However, with Minefield, I just get "Minefield" - but I can't seem to find what they're doing different here.
Comment 18 Stefan [:stefanh] 2008-10-05 10:58:04 PDT
Once browser is fixed, I'll resolve this and continue fixing the rest in bug 56995 .
Comment 19 Karsten Düsterloh 2008-10-06 17:11:12 PDT
Using a trunk debug build of 2008-09-29, opening Composer showed "Composer" during load, "untitled -" afterwards and "xxx" when setting xxx as the title; debugQA had no influence there.
With attachment 341750 [details] [diff] [review], it becomes ""/"untitled - Composer"/"xxx - Composer", respectively. Again, no debugQA influence.
I guess the "" during load is acceptable.


In the following, I specifically don't take bug 56995 into account, i.e. I consider "xxx - SeaMonkey {Build ID: 20080929205810}" to be the expected value for normal pages.

So, same build, now browser: new empty tabs cause title "SeaMonkey", normal pages with title xxx show "xxx - SeaMonkey {Build ID: 20080929205810}", switching normal tab to empty tab gets just "SeaMonkey {Build ID: 20080929205810}". 
-> empty tab title isn't consistent, normal page title is okay.

Without debugQA, this gets "SeaMonkey"/"xxx -"/"", respectively.
-> empty tab title isn't consistent, normal page title is broken, too.

With attachment 340807 [details] [diff] [review] applied, this becomes "SeaMonkey"/"xxx - SeaMonkey {Build ID: 20080929205810}"/"SeaMonkey - SeaMonkey {Build ID: 20080929205810}".
-> empty tab title isn't consistent, normal page title is okay.

Without debugQA it's "SeaMonkey"/"xxx"/"SeaMonkey", respectively.
-> empty tab title _is_ consistent, but normal page title lacks extension.

Now enter attachment 341753 [details] [diff] [review]:

With debugQA: "SeaMonkey"/"xxx - SeaMonkey {Build ID: 20080929205810}"/"SeaMonkey {Build ID: 20080929205810}".
-> empty tab title isn't consistent, normal page title is okay.

Without debugQA: "SeaMonkey"/"xxx"/"SeaMonkey".
-> empty tab title _is_ consistent, but normal page title lacks extension.


I think this is still a complete mess, regardless of whether bug 56995 is considered useful/wanted or not. 
First, we should define
- how normal page titles should look 
- how empty page titles should look
- how these should differ between debug and optimized builds
- how these should differ between development and release builds (if at all)

BTW: for empty page titles, e.g. Safari displays "untitled"...
Comment 20 neil@parkwaycc.co.uk 2008-10-07 01:14:47 PDT
Comment on attachment 341750 [details] [diff] [review]
Composer fix (pushed)

>-        contenttitlesetting="true"
>+        contenttitlesetting="false"
>         titlemodifier="&editorWindow.titlemodifier;"
>         titlemenuseparator="&editorWindow.titlemodifiermenuseparator;"
You could replace contenttitlesetting with title="&editorWindow.titlemodifier;" so that base title appears while the page is loading.
Comment 21 Stefan [:stefanh] 2008-10-07 03:28:48 PDT
(In reply to comment #19)
> In the following, I specifically don't take bug 56995 into account, i.e. I
> consider "xxx - SeaMonkey {Build ID: 20080929205810}" to be the expected value
> for normal pages.

The point was to fix bug 56995 for browser at the same time. Once this is fixed, it's only MailNews and View Partial Source left.
Comment 22 Karsten Düsterloh 2008-10-07 04:46:24 PDT
> The point was to fix bug 56995 for browser at the same time.

I don't think you should fix that here, just stay in this bug's scope.
(One of the open questions to be discussed in 56995 is "where would we put the build id?")
Comment 23 Stefan [:stefanh] 2008-10-07 05:54:45 PDT
(In reply to comment #22)
> > The point was to fix bug 56995 for browser at the same time.
> I don't think you should fix that here, just stay in this bug's scope.

The scope is "window titles are incorrect on mac" ;-) Normal page titles should not have an app name appended to the title. This is something that have annoyed me for years.

> (One of the open questions to be discussed in 56995 is "where would we put the
> build id?")

I don't really think that is such an important issue - what really is important is that we have correct window titles in our forthcoming releases. I can also live with the debugQA behaviour that my patches introduces. That is, I don't really change the debugQA window titles here. Empty page titles are inconsistent also without the patch when debugQA is installed (on all OS, fwiw).
Comment 24 Stefan [:stefanh] 2008-10-07 09:58:46 PDT
Created attachment 342096 [details] [diff] [review]
editor.xul followup per comment #20 (pushed)

> You could replace contenttitlesetting with title="&editorWindow.titlemodifier;"
> so that base title appears while the page is loading.
Good point.
Comment 25 Stefan [:stefanh] 2008-10-07 10:31:41 PDT
> - how normal page titles should look 

Window title should just be the page title (no app name)

> - how empty page titles should look

The default behaviour is to display the app name, see http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#706 and note that mTitleDefault is the titlemodifier on mac (see http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsContentTreeOwner.cpp#912). So, I would think that the best option would be to just display the app name here.

> - how these should differ between debug and optimized builds
> - how these should differ between development and release builds (if at all)

There shouldn't be any difference. Final release builds doesn't have debugQA installed, though. If you install debugQA, I think the optional on mac would be to just have the build ID appended to the page title and to show appName+buildID on pages without a title (consistent with non-debugQA behaviour).
> 
> BTW: for empty page titles, e.g. Safari displays "untitled"...

Yeah, I know (Camino does that too). Opera shows the address. We could do it the Safari/Camino way, but that wold require more work and I'm not sure it's the right thing. For example, should we then show the build ID together with untitled? I think I saw a comment in a bug where someone said that he considered the Safari behaviour to be a bug.
Comment 26 Stefan [:stefanh] 2008-10-08 08:52:01 PDT
Comment on attachment 342096 [details] [diff] [review]
editor.xul followup per comment #20 (pushed)

Pushed changeset 554:004c7427680a
Comment 27 Stefan [:stefanh] 2008-10-12 04:52:25 PDT
Putting this on blocking radar (2.0).
Comment 28 Stefan [:stefanh] 2008-10-13 13:55:30 PDT
Karsten and I talked about this (irc), for some time ago. Karsten didn't liked that we're displaying the appName for pages without a title when we in fact doesn't display the appName for pages with a title. I sort of agree with him - it is inconsistent. However, I'm not sure I want to hack nsContentTreeOwner.cpp at this stage. I also expect a lot of discussion on what we should display for empty page titles. For example, I'd say that "Untitled" would be reasonable - but that doesn't fit with what we display in the tab, and showing "(Untitled)" in the titlebar isn't acceptable imo (and I'm not sure that I'll be able to win when it comes to changing the tab title). So, I'm not sure of how to proceed here.
Comment 29 Stefan [:stefanh] 2009-04-04 15:10:11 PDT
OK, so the solution here is probably to combine removing the contenttitlesetting attribute in navigator.xul and make tabbrowser not displaying the appname on mac. On empty pages we have then the option to either display just the appname (a bit inconsistent, but in line with Firefox) or just display "Untitled" (like Safari). If we go for "Untitled", I think we should display it for tab titles as well. Whatever the solution is, it has to work with debugQA and other extensions that changes the window title.
Comment 30 Stefan [:stefanh] 2009-07-11 15:47:19 PDT
When bug 202315 is fixed we won't have debugQA for beta - this makes me think this is a blocker for beta1. We should at least fix this so we keep the "old" (even though it's incorrect) behaviour. FYI, comment #0 only applies for browser (rest is fixed).
Comment 31 Stefan [:stefanh] 2009-07-11 15:49:07 PDT
I think we might be able to get back the old behaviour by just removing the contenttitlesetting attribute.
Comment 32 Stefan [:stefanh] 2009-07-12 08:22:22 PDT
(In reply to comment #31)
> I think we might be able to get back the old behaviour by just removing the
> contenttitlesetting attribute.

The patch in bug 462997 will get back the old behaviour.
Comment 33 Stefan [:stefanh] 2009-07-12 08:30:42 PDT
Not blocking since we fix the outstanding issues with the patch in bug 462997.
Comment 34 Stefan [:stefanh] 2009-08-09 13:07:13 PDT
Created attachment 393457 [details] [diff] [review]
New attempt

Here's a new attemp in trying to fix the last issue with our window titles on mac.
Comment 35 Stefan [:stefanh] 2009-08-22 05:33:53 PDT
Created attachment 396061 [details] [diff] [review]
Another attempt

It appears that we can set the window title with nsIBaseWindow (Neils idea), then we can still use the contenttitlesetting attribute.
Comment 36 Stefan [:stefanh] 2009-08-23 05:39:10 PDT
Created attachment 396137 [details] [diff] [review]
new attempt - fix tabbrowser

This patch will result in the following behaviour:

1) Mac window titles will be the same as win/nix when debugQA is installed
2) On mac, we behave like this:
   - Pages with titles: window title and tab label = page title
   - Pages without a title: window title and tab label = uri
   - about:blank: window title and tab label = "Untitled"
Comment 37 neil@parkwaycc.co.uk 2009-08-24 05:09:43 PDT
Comment on attachment 396137 [details] [diff] [review]
new attempt - fix tabbrowser

>+            if (aURI) {
[Should always be true.]

>+              try {
>+                aURI = this.mURIFixup.createExposableURI(aURI).spec;
>+              } catch (e) {
>+              }
Still need the spec even if we fail to create an exposable URI.

>+            if (aURI && aURI != "about:blank") {
[aURI should still always be true!]
Reversing the test will make the logic flow easier to follow.

>+              if (!docTitle)
>+                docTitle = this.mStringBundle.getString("tabs.untitled");
Please comment here that we're overriding contenttitlesetting because we don't want to use titledefault here.

>-            var browser = aTab.linkedBrowser;
Still need this ;-)

>-            var crop = "end";
>+            var cropping = "end";
Why the renaming?

>+              title = this.getTitleForURI(this.mCurrentBrowser.currentURI);
Wrong browser.

>       // Set the new title modifier
>       document.documentElement.setAttribute("titlemodifier", titlemodifier);
What about titledefault?
Comment 38 Stefan [:stefanh] 2009-08-24 12:16:21 PDT
Created attachment 396266 [details] [diff] [review]
New version

(In reply to comment #37)
> (From update of attachment 396137 [details] [diff] [review])
> >+            if (aURI) {
> [Should always be true.]


Right, I start with the try direct
> >+              try {
> >+                aURI = this.mURIFixup.createExposableURI(aURI).spec;
> >+              } catch (e) {
> >+              }
> Still need the spec even if we fail to create an exposable URI.

Fixed.

> >+            if (aURI && aURI != "about:blank") {
> [aURI should still always be true!]
> Reversing the test will make the logic flow easier to follow.
Done.
> >+              if (!docTitle)
> >+                docTitle = this.mStringBundle.getString("tabs.untitled");
> Please comment here that we're overriding contenttitlesetting because we don't
> want to use titledefault here.
Done.
> >-            var browser = aTab.linkedBrowser;
> Still need this ;-)
yes, fixed. 
> >-            var crop = "end";
> >+            var cropping = "end";
> Why the renaming?
I thought it looked nicer with "aTab.crop = cropping" than "aTab.crop = crop;". If you don't like it, I keep the "crop".
> >+              title = this.getTitleForURI(this.mCurrentBrowser.currentURI);
> Wrong browser.
Oops. Fixed.
> >       // Set the new title modifier
> >       document.documentElement.setAttribute("titlemodifier", titlemodifier);
> What about titledefault?


I don't know if there's any point removing it - we don't use it and it seems that this happens after it has been removed by contenttitlesetting.
Comment 39 Stefan [:stefanh] 2009-08-24 12:18:15 PDT
(In reply to comment #38)
> Created an attachment (id=396266) [details]
> New version
> 
> (In reply to comment #37)

> I don't know if there's any point removing it - we don't use it and it seems
> that this happens after it has been removed by contenttitlesetting.

document.documentElement.setAttribute("titlemodifier", titlemodifier);
happens after the titlemodifier has been removed by contenttitlesetting.
Comment 40 Stefan [:stefanh] 2009-08-24 12:27:49 PDT
Comment on attachment 396266 [details] [diff] [review]
New version

Sorry, I think I made some mistake here...
Comment 41 Stefan [:stefanh] 2009-08-24 13:12:33 PDT
Created attachment 396280 [details] [diff] [review]
Another version

I forgot to include the diff for navigator.xul... This version keeps the crop var and also removes the titledefault (in debugQABuildIDOnLoad).
Comment 42 neil@parkwaycc.co.uk 2009-08-24 14:15:01 PDT
Comment on attachment 396280 [details] [diff] [review]
Another version

>+            } catch (e) {
>+                aURI = aURI.spec;
Nit: incorrect indentation

>             // If location bar is hidden and the URL type supports a host,
>             // add the scheme and host to the title to prevent spoofing.
Bah, this is harder than it looks; on the Mac you might already have the URL.
Let's get this change in and then we can think about what to do in this case.
For instance, we might skip this for the Mac, as you can just click the pill.

>+      // titlemodifier attribute. We don't use titledefault, but lets remove it
Nit: let's
Comment 43 Karsten Düsterloh 2009-08-24 17:00:07 PDT
Comment on attachment 396280 [details] [diff] [review]
Another version

Just a couple of nits:

>+++ b/suite/browser/tabbrowser.xml
>+      <method name="getTitleForURI">
...
>+            if (aURI == "about:blank")
>+              aURI = "";
>+            else {

If one if-branch has braces, the other should have as well.

>+              try {
>+                let characterSet = this.mCurrentBrowser.contentDocument.characterSet;
>+                const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
>+                                               .getService(Components.interfaces.nsITextToSubURI);
>+                aURI = textToSubURI.unEscapeNonAsciiURI(characterSet, aURI);

Using const here is a bit esoteric, but if you insist, please add the missing k prefix.

>+              if (!docTitle)
>+                // Here we actually override contenttitlesetting, because we
>+                // don't want the titledefault value.
>+                docTitle = this.mStringBundle.getString("tabs.untitled");

Please use braces here.

>             if (docTitle) {
>               newTitle += docElement.getAttribute("titlepreface");
>               newTitle += docTitle;

Drop the unnecessary assignment, while you're at it.

>@@ -1089,44 +1130,25 @@
>             var browser = aTab.linkedBrowser;

Unused variable.


r=me with nits fixed.
Comment 44 neil@parkwaycc.co.uk 2009-08-25 02:05:15 PDT
(In reply to comment #43)
> (From update of attachment 396280 [details] [diff] [review])
> >+            if (aURI == "about:blank")
> >+              aURI = "";
> >+            else {
> If one if-branch has braces, the other should have as well.
Or an alternative is to return ""; and not have an else block at all.

> >+                const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
> Using const here is a bit esoteric, but if you insist, please add the missing k prefix.
This was just moved code, but you could change it to use var while you're there.

> >+              if (!docTitle)
> >+                // Here we actually override contenttitlesetting, because we
> >+                // don't want the titledefault value.
> >+                docTitle = this.mStringBundle.getString("tabs.untitled");
> Please use braces here.
(This is to avoid confusion because the comment makes the block look longer.)

> >             if (docTitle) {
> >               newTitle += docElement.getAttribute("titlepreface");
> >               newTitle += docTitle;
> Drop the unnecessary assignment, while you're at it.
I don't know what this means... maybe because we don't provide titlepreface? I'm not sure what we should do if an extension adds one.

> >             var browser = aTab.linkedBrowser;
> Unused variable.
I think Mnyromyr might want to read comment #37 and related comments ;-)
Comment 45 Stefan [:stefanh] 2009-08-25 08:58:03 PDT
(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 396280 [details] [diff] [review])
> > >+            if (aURI == "about:blank")
> > >+              aURI = "";
> > >+            else {
> > If one if-branch has braces, the other should have as well.
> Or an alternative is to return ""; and not have an else block at all.

Good idea.
> 
> > >+                const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
> > Using const here is a bit esoteric, but if you insist, please add the missing k prefix.
> This was just moved code, but you could change it to use var while you're
> there.
Sure.
> 
> > >+              if (!docTitle)
> > >+                // Here we actually override contenttitlesetting, because we
> > >+                // don't want the titledefault value.
> > >+                docTitle = this.mStringBundle.getString("tabs.untitled");
> > Please use braces here.
> (This is to avoid confusion because the comment makes the block look longer.)
> 
> > >             if (docTitle) {
> > >               newTitle += docElement.getAttribute("titlepreface");
> > >               newTitle += docTitle;
> > Drop the unnecessary assignment, while you're at it.
> I don't know what this means... maybe because we don't provide titlepreface?
> I'm not sure what we should do if an extension adds one.

I think we should keep it for now.
Comment 46 Karsten Düsterloh 2009-08-25 12:52:54 PDT
(In reply to comment #44)
> > >             var browser = aTab.linkedBrowser;
> > Unused variable.
> I think Mnyromyr might want to read comment #37 and related comments ;-)

I did. But I'm still none the wiser - what's the point of an unused variable?
Comment 47 Karsten Düsterloh 2009-08-25 13:08:51 PDT
> I did. But I'm still none the wiser - what's the point of an unused variable?

Argl. Forget it. :-(
Comment 48 Stefan [:stefanh] 2009-08-25 13:42:56 PDT
Landed with comments addressed (used return "";), fixed braces and used a let instead of the const, dropped the assignment etc:

http://hg.mozilla.org/comm-central/rev/d5cabc748ec7

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