282955 - Run-on title in urlbar-less windows on Mac

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Daniel Veditz [:dveditz]]

(Not really docshell but I don't know where appshell bugs get filed)

bug 22183 puts the domain in the titlebar when a browser window has no location
bar. On Mac OSX nsContentTreeOwner::mTitleSeparator is not set (windows don't
show "- Mozilla" at the end) so the new domain runs directly into the start of
the title. e.g. "https://bugzilla.mozilla.orgEnter Bug: Core"

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: Add separator to mac title

Comment 2

[Daniel Veditz [:dveditz]]

presuming blocking status to make sure this is on the radar for Monday's drivers
meeting.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Don't we need an equivalent fix for the tabbrowser code?

Comment 4

[Daniel Veditz [:dveditz]]

You'd think, but apparently tabbrowser is "broken" on MacOSX to behave like all
the other platforms. See step 5 of
https://bugzilla.mozilla.org/show_bug.cgi?id=22183#c247

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174914 [details] [diff] [review]
Add separator to mac title

ok, sr=bzbarsky (and file a bug on tabbrowser on mac?)

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: alternate smaller patch

I owe you a real explanation...
 
On Mac nsContentTreeOwner::XULWindow copies the window modifier text ("Mozilla
Firefox") into the default title (if empty) and actually removes the modifier
attribute from the docshellElement. XULWindow() doesn't remove the separator
from the docshell, but doesn't set nsContentTreeOwner's mTitleSeparator member
in the Mac's case since it's never used.

In my nsContentTreeOwner changes I was relying on mTitleSeparator, which was
empty on Mac and caused the run-on. The change here gets the separator
attribute from the docshell. 

A smaller change would have been to go ahead and set mTitleSeparator even on
the mac, then my code in ::SetTitle() wouldn't need any changes.

Over in the tabbrowser the title is constructed, but if modifier is null (and
on Mac the content tree owner made sure it was) then the separator+modifier
isn't added to the end. But the separator attribute my tabbrowser changes were
using *did* exist so the bug did not occur when the tabbrowser was setting the
title.

This is a smaller change, do you prefer it? This always sets mTitleSeparator so
it's available for my new code. The added check for the empty modifier in
SeTitle shouldn't be necessary if the Mac is setting the default title, but
seems safer anyway just in case some redistributor doesn't like branding their
browser windows and has no modifier.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So does this last patch change the default behavior on mac in any way?

Comment 8

[Daniel Veditz [:dveditz]]

The change unconditionally sets nsContentTreeOwner::mTitleSeparator rather than
leave it blank on the Mac. The only place that used this member besides my new
code is seen in the patch--the reason I gave so much context. On the Mac we
never get there because the existing ifdef mac code sets the default title, and
for good measure I added a check for an empty modifier which the Mac code also zaps.

I like the second patch simply because there's less conditional code and it
eliminates the need for a couple temp autoStrings, but either one works. The
second also eliminates the extra GetAttribute every time a title is set on the
Mac, but I don't know if that was worth worrying about.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 174935 [details] [diff] [review]
alternate smaller patch

sr=bzbarsky

Comment 10

[[e-mail]]

Comment on attachment 174935 [details] [diff] [review]
alternate smaller patch

Yum, this version is much better, though I wonder why FireFox didn't just
#ifdef the attributes in the XUL in the first place...

Comment 11

[[e-mail]]

I filed bug 283000 on the lousy content title setting code.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 174914 [details] [diff] [review]
Add separator to mac title

We're going with the other one

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 174935 [details] [diff] [review]
alternate smaller patch

a=caillon for the branches

Comment 14

[Daniel Veditz [:dveditz]]

Fix checked into 1.7 and aviary1.0.1 branches.

Comment 15

[sairuh (rarely reading bugmail)]

verified fixed using the tests in bug 22183 comment 247. there are no longer
run-ons btwn the site name and the title. looks good!

Comment 16

[sairuh (rarely reading bugmail)]

note, I tested this on Mac OS X 10.3.8 with 2005022304-1.0.1 firefox bits.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 174935 [details] [diff] [review]
alternate smaller patch

fixes bug 312426

Comment 18

[Daniel Veditz [:dveditz]]

fix checked into 1.8 branch