59749 - window title not updated for full-page plugins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Dave Barrowman]

From Bugzilla Helper:
User-Agent: Mozilla/4.7 [en] (Win95; U)
BuildID:    2000110604

When I load a PDF document and view it with the Acrobat plugin inside Mozilla, 
the Mozilla window title is not updated, so it continues to display the title of 
the previous page viewed.

Reproducible: Always
Steps to Reproduce:
0. Make sure you have the Acrobat plugin installed.

1. Go to an arbitrary web page - say http://home.netscape.com
Note the window title (e.g., "Netscape.com") in the blue title bar at the top of 
thw browser window.

2. Now go directly to a PDF document in the same window - 
http://web.mit.edu/norstadt/Public/election.pdf is a good example.  The PDF 
document will appear in the browser window (inside the Acrobat plugin 
interface), and note the Mozilla title bar.  It remains unchanged, showing the 
title of the previous document.

3.

Actual Results:  The title bar continues to display the title of the previous 
document.

Expected Results:  The title bar should display the title of the current 
document (What should it be? In NS 4.7x it is empty; I think it should be the 
URL of the document.) 

Not sure if this impacts the document.title property or is just manifested in 
the browser window title bar.

Comment 1

[David Evola]

Same problem build 2000110904 Win98

Comment 2

[Asa Dotzler [:asa]]

not sure if this belongs to plugins or apps.  over to plugins first.

Comment 3

[sean echevarria]

updating summary - affects all plugins.
Happening on NT.  Will check Mac later.

Comment 4

[karnaze (gone)]

Moving to m1.0 and reassigning to peterl.

Comment 5

[Peter Lubczynski]

Confirming 2k. I can't believe we do not update the title bar for full-page 
plugins.

Should be easy task to fix.

Comment 6

[Peter Lubczynski]

This needs to be done with embedding in mind.

Jud, do you know how to do this?

Comment 7

[Judson Valeski]

As long as we're consistent about how we update the page title between regular
content, and plugin content (sounds like we're not), embedding will be fine.

Comment 8

[Peter Lubczynski]

Attachment: patch to nsDocShell to clear window title for nsIContentViewers

Comment 9

[Judson Valeski]

r=valeski

Comment 10

[Chris Waterson]

sr=waterson

Comment 11

[Brendan Eich [:brendan]]

chofmann approved on behalf of [e-mail] for inclusion in 0.9.

/be

Comment 12

[Peter Lubczynski]

Patch checked in. Marking FIXED.

/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.269; previous revision: 1.268

Comment 13

[Peter Lubczynski]

This is not fixed yet. Using nsnull gives too many assertions in debug builds. 
Safer to use " ".

Comment 14

[Peter Lubczynski]

Attachment: better patch not using nsnull

Comment 15

[Peter Lubczynski]

This is a much better patch. It prevents mTitle from being null and prevents the 
asserts.

Comment 16

[Arun Ranganathan]

nominating nsbeta1 based on peterl's comments to my pdt bug list .  sounds like
peterl has a fix in hand.

Comment 17

[Peter Lubczynski]

Took a closer look at this. Setting the title to null or even blank in 
nsDocShell is wrong because it's too general. There are too many cases that null 
or blank causes ASSERTS which could possibly cause havok down the road.

Since this only effects full-page plugins to my knowledge, a simple QI of the 
content viewer should reveal if it's implementing a plugin interface. However, 
full-page plugins don't have any interface. The class nsPluginViewerImpl doesn't 
actually implement nsIPluginViewer because it doesn't exsist!

Andrei would it be okay for me to create an nsIPluginViewer and have 
nsPluginViewerImpl implement it?

Then, Jud/Adam could I do a QI on it in nsDocShell and then and only then set 
the title to null?

Perhaps there is an easier way, but if nsIPluginViewer were IDL-ized then I 
guess it could be usefull for other things. Not quite sure about other benefits 
of IDL.

Comment 18

[av (gone)]

That's a good idea. Maybe nsPIPluginViewer?

Comment 19

[Peter Lubczynski]

Looking at the class again, nsPluginViewer.cpp, I don't see any reason to use 
IDL as there doesn't look like any methods I want to make public (yet). Could 
someone correct me if I'm wrong?

Comment 20

[Judson Valeski]

yea, it's best to avoid creating an interface simply for "type" identification
so you can special case behavior.

Should full page plugins have *some* window title? If so, I could see creating a
nsIPluginViewer.idl that had a readonly title attribute on it. So in the
docshell you could ask for nsIPluginViewer, and call GetTitle() on it.

Comment 21

[Peter Lubczynski]

That sounds like a nice feature since it appears that 4.x reverts to saying 
"Netscape" and IE makes the title of the page the URL of the PDF. I guess the 
quickest way to fix this would be to do what IE does. If there is an url, then 
use that for the title in the docshell. Plugins will have an url and my guess is 
that others that ASSERT on null titles won't have an href.

But there would probably be other things I could think of that a full-page 
plugin inteface would be good for. Some may overlap code already in 
nsObjectFrame.cpp

Comment 22

[karnaze (gone)]

Moving to m0.9.2

Comment 23

[Peter Lubczynski]

->[no eta]

Comment 24

[Peter Lubczynski]

The following patch creates a new public interface, nsIPluginViewer and has has
PluginViewerImpl implement it. Then, in nsDocShell, when we create the content
viewer, I check if it's a nsIPluginViewer and clear the title bar.

So much work just to clear the title bar for full-page plugins, but I'm seeking
reviews (or other ideas).

Oh, as a side effect, it also has a one-line fix for bug 80105.

Comment 25

[Peter Lubczynski]

Attachment: create and implement nsIPluginViewer and clear the title bar and fix for bug 80105

Comment 26

[av (gone)]

It is quite an amount of work but I think it is pretty straightforward and 
should not introduce any implications. Also, having an interface for a plugin 
viewer is not a bad idea in my opinion.

r=av

Just a minor thing not affecting r= , is it going to be plain blank? Why not to 
write something there like 'Netscape 6' or whatever the application name can be.

Comment 27

[Peter Lubczynski]

It won't be blank, it'll be the "default" (which in the cas e of Mozilla
includes the name and buildID. The reason for all this work is because I
couldn't set all content viewers to "default" in the docshell because I got a
ton of ASSERTs and I felt I may have introduced regressions. Adding this, pretty
much dummy interface seemed like least amount of risk and with our macros, it's
not too much coding.

Comment 28

[av (gone)]

Peter, I've just checked in NS_RELEASE --> NS_IF_RELEASE in nsPluginViewer, so 
you may remove it from your patch.

Comment 29

[Marc Attinasi]

Looks good. sr=attinasi

Comment 30

[chris hofmann]

a=chofmann

Comment 31

[Peter Lubczynski]

Patch checked in, marking FIXED.

Comment 32

[Christopher Blizzard (:blizzard)]

just tracking

Comment 33

[shrirang khanzode]

verif working on win /mac 0624