Jesse Ruderman Reporter
	Description • 19 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=22183#c238 was only checked in on
the branch.  In a security bug meeting on Tuesday, we decided to do the same
thing for Firefox 1.5 (and trunk?).  I'll leave bug 22183 open for a long-term
solution.

	Jesse Ruderman Reporter
	Comment 1 • 19 years ago

I hand-applied dveditz's patch
(https://bugzilla.mozilla.org/show_bug.cgi?id=22183#c238) to the trunk and
edited a few code comments.  I wasn't able to test the code in
nsContentTreeOwner::SetTitle because the code in tabbrowser always seems to be
what determines the window title in Firefox.  I left out the SeaMonkey-specific
part of the patch.

cvs diff -p
xpfe/appshell/src/nsContentTreeOwner.cpp 
content/html/document/src/nsHTMLDocument.cpp 
toolkit/content/widgets/tabbrowser.xml

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 19 years ago

Comment on attachment 192787 [details] [diff] [review]
update dveditz's patch to trunk

It'll take me some time to get to this...  not even sure how much.

	Jesse Ruderman Reporter
	Comment 3 • 19 years ago

How can I test nsContentTreeOwner::SetTitle?  Build Camino?

	Mike Connor [:mconnor]
	Comment 4 • 19 years ago

Comment on attachment 192787 [details] [diff] [review]
update dveditz's patch to trunk

I'm not really a good reviewer for appshell or content code, it should be a
peer in the appropriate module if at all possible.

	Gervase Markham [:gerv]
	Comment 5 • 19 years ago

<sigh> I wish we'd had time to do something better. Still, as long as we don't
see this as the long-term solution...

Gerv

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 19 years ago

So is there a reason this is not patching both tabbrowsers?  Is there a reason
that embedding builds don't have the same problem... or do they?

That said, doesn't the nsContentTreeOwner code just run whenever you load a new
page in a non-tabbrowser setup?  Just remove the hookup of the DOMTitleChanged
listener in tabbrowser to exercise it visually, I think.  And file a followup
bug on the fact that we have this code in 3 different places?

	Jesse Ruderman Reporter
	Comment 7 • 19 years ago

> So is there a reason this is not patching both tabbrowsers?

I don't build Seamonkey and didn't realize I was expected to help maintain that
fork.  If you want me to build Seamonkey, I can.

I don't know the answers to your other questions.  dveditz (the original author
of the patch) might.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 19 years ago

Comment on attachment 192787 [details] [diff] [review]
update dveditz's patch to trunk

This needs at least a similar patch for xpfe and a check on whether
nsWebBrowser has similar issues, as well as testing of the nsContentTreeOwner
code.  Please rerequest review when those have happened.

	Asa Dotzler [:asa]
	Comment 9 • 19 years ago

we're gonna have to take what we did for 1.0  because we dont' have anything
better and time's run out. Dan, can you drive this in?

	Daniel Veditz [:dveditz] Assignee
	Comment 11 • 19 years ago

Comment on attachment 197923 [details] [diff] [review]
including merged xpfe/global changes

r=myself, this is just a trunk merge of a previously reviewed patch (r=mconnor,
sr=bzbarsky, a=asa)

Still requesting sr=bz since he had a problem with the first version here.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 19 years ago

Comment on attachment 197923 [details] [diff] [review]
including merged xpfe/global changes

I guess this is ok given the situation, but could we please file a bug to have
one and only one place where this code lives by 1.9?

	Daniel Veditz [:dveditz] Assignee
	Comment 13 • 19 years ago

fixed on trunk

	Daniel Veditz [:dveditz] Assignee
	Comment 14 • 19 years ago

fix checked into 1.8 branch

	neil@parkwaycc.co.uk
	Comment 15 • 19 years ago

Comment on attachment 197923 [details] [diff] [review]
including merged xpfe/global changes

I checked Editor and it has its own title setting code to, so it's unaffected
by this change.

	neil@parkwaycc.co.uk
	Comment 16 • 19 years ago

(In reply to comment #12)
>could we please file a bug to have one and only one place where this code lives
For anyone interested in fixing this follow-up bug, preliminary investigation
suggests that one of the callers of updateTitlebar is unnecessary and the other
caller can be worked around by setting this.contentDocument.title to itself.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 19 years ago

Did we ever get this followup filed?

Also, note that this patch caused bug 317750 (though imo that site is just broken).

	Jesse Ruderman Reporter
	Comment 18 • 19 years ago

How did this break bug 317750?  Does the site's JavaScript somehow see the modified title?