Closed Bug 89493 Opened 20 years ago Closed 20 years ago

3d Groove install does not happen

Categories

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

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: shrir, Assigned: peterlubczynski-bugs)

References

()

Details

(Whiteboard: nsBranch, [PDT+], [need sr=])

Attachments

(2 files)

branch builds 0705

tested 85334, works on win, not on mac 

steps:

complete the shockwave installation;registration as mentioned in bug 85334.
Go to Games|Arcade|Tank Wars|High Bandwidth

Observe that a blank page loads up and no 3d groove installation happens.

cannot play any games that require 3d groove.
This is caused by a WM_FONTCHANGE message (probably sent my Shockwave) which
then causes us to do a re-frame. Currently, plugins don't like to be reframed as
their lifetime is somewhat tied to the object frame. 

cc:ing Waterson as he's got some code to try to make the object frame re-framable. 

The shockwave testcase is very large, is there a simple way to force a reframe
on a simple testcase?

On the other hand, at this late stage in the game, I was thinking of hacking
this, maybe with the internaluseonly pref, and special casing Shockwave so a
WM_FONTCHANGE does nothing.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.3
Just got off the phone with chofmann. I guess what I'd recommend at this point 
would be going with peterl's hack (stuffing the fontchange pref), or one of the 
variations that attinasi/rbs has proposed on the branch only. We should verify 
that this doesn't break the font preference (e.g., changing your font from the 
preferences dialog -- it would suck if you had to restart the browser to pick 
that up). 

In the meantime, we should pursue a more aggressive fix on the trunk where we 
handle the reframe by disconnecting the plugin from the old frame, and 
reconnecting it to the new frame. I'm pretty sure that this situation is 
analogous to 4.x resize-reload behavior (for those who might not recall, 4.x 
didn't do incremental reflow, so it actually reloaded the doc from cache when 
you resized). In theory, if we get the ordering of the plugin API calls right, 
shockwave and other plugins should be able to deal with us giving the plugin a 
new widget. (We can refer at the MozClassic codebase if necessary.)

Does this make sense?
Sounds good to me, anyone else?

Attaching patch that does prevents the reframe (from other bug). This needs some
good testing. Also, nominating.
Keywords: patch
Whiteboard: nsBranch
This hack will basically undo the fix of bug 69117 which was deemed of "interest 
from the very important embedding customer" :-) c.f. comments on that bug.
Cc:ing ftang & yokoyama.
Are we adding the special casing for Shockwave as mentioned 
(2001-07-05 16:41) together with the internaluseonly pref?


I'm not quite sure how the current system works to add Shockwave special casing.
My current patch is just keying off the string pref value,
"font.internaluseonly.changed". 

I guess if Shockwave were to be special cased here we can either set another
pref, or better yet, use the global service manager to ask the plugin manager if
Shockwave is running. Hm....Andrei, is this even possible? How else from the
nsPresContext could I find out if we are in Shockwave 3D Groove setup?
I had some thought about this little conundrum and here is a suggestion.

First, the crux of the problem stems from the fact that WM_FONTCHANGE is handled 
in a blindly way. This is due to the fact that it is not possible to tell what 
is really happening -- as yokoyama pointed out in bug 69117, Windows does give 
enough details about that.

So a kind of state is needed, e.g., last-modified-date of the Fonts directory
for example. Unfortunately, doing this will involve other things and can make 
the code even more complex (perhaps, it could be that not all fonts are 
installed in the Fonts directory in the future, and there are third-party font 
managers like Adobe Type-Manager) who intercept Windows GDI calls and tweak 
them.

So the idea is to add a return flag to UpdateFontList(), says 
UpdateFontList(aDidChanged), and proceed with the rest of the processing only if
aDidChanged is set to TRUE. This way, the gratuitous WM_FONTCHANGE messages that 
are sent by plugins and other applications will be filtered out in a natural 
way. And the overall logic of this will not be invasive -- it will be 
self-contained in the GFX font-subsystem. Implementation-wise, UpdateFontList() 
will need to keep the old list of fonts around, and set the flag if the new list 
of fonts is different from the old list. From this, the code in 
widget/src/windows/nsWindow.cpp will read as indicated below and everybody will 
be happy. Of course because WM_FONTCHANGE seems to be sent frequently (at each 
open/close of a window in some cases -- as waterson mentoned in the other bug), 
this is still not the optimal solution. Another more efficient way that could 
prevent from unnecessarily creating the new list of fonts in the first place 
will be more efficient. Until then, this idea could work rather well in common 
situations.

2991         case WM_FONTCHANGE: 
2992           {
2993             nsresult rv;
+  PRBool didChanged = PR_FALSE
2994 
2995             // update the global font list
2996             nsCOMPtr<nsIFontEnumerator> fontEnum = 
do_GetService("@mozilla.org/gfx/fontenumerator;1", &rv);
2997             if (NS_SUCCEEDED(rv)) {
-2998               fontEnum->UpdateFontList();
+  fontEnum->UpdateFontList(&didChanged);
2999             }
+  if (didChanged) {
3000 
3001             // update device context font cache
3002             // Dirty but easiest way: 
3003             // Changing nsIPref entry which triggers callbacks
3004             // and flows into calling mDeviceContext->FlushFontCache()
3005             // to update the font cache in all the instance of Browsers
3006             nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, 
&rv); 
3007             if (NS_SUCCEEDED(rv)) { 
3008               PRBool fontInternalChange = PR_FALSE;  
3009               pPrefs->GetBoolPref("font.internaluseonly.changed", 
&fontInternalChange);
3010               pPrefs->SetBoolPref("font.internaluseonly.changed", 
!fontInternalChange);
3011             }
3012           }
+ }
Continuing... re: implementation. An alternative could be a double enumeration
in nsFontMetricsWin.

- The first EnumFontFamiliesEx() is a search -- see if all the enumerated fonts 
are found in the current global list, and if all the current fonts in the global 
list are still enumerated. This can be handled with some flags (with an early 
exit if an enumerated font isn't found).

- The second EnumFontFamiliesEx() is the real reconstruction of the global list 
if indeed something changed.

So there is an extra cost of another enumeration, with the advantage that there 
won't be two lists around at the same time. But since *real* WM_FONTCHANGE are 
really rare, nothing would generally happen. Hence it is a slight optimization 
for the common case.
If this happens at each open/close of a windows, then this isn't helping the 
open/close window performance effort. So I am wondering what IE is doing?!
Surely, they must be having the same problem? Anyone knows what happens when a 
new font is installed with IE? Or are they using another (undocumented?) API
to update their internal data?
rbs, don't panic! :-) My Win32 X-server app (``Exceed'') is what appears to be 
broadcasting the WM_FONTCHANGE messages.
Based on discussions with pdt, marking pdt+
Whiteboard: nsBranch → nsBranch, pdt+
For the branch, I think we need a safe solution with as small an impact as
possible. To this end, I'd suggest going with the 'hack' to partially ignore the
internaluseonly font change pref change, however I think we should at least
flush the font cache in that case, something like this:

 void
 nsPresContext::PreferenceChanged(const char* aPrefName)
 {
+  // REALLY OBVIOUS HACK COMMENT HERE (bug=89493)
+  if (strcmp(aPrefName,"font.internaluseonly.changed") == 0) {
+    if (mDeviceContext) {
+      mDeviceContext->FlushFontCache();
+    }
+    return;
+  }
   // Initialize our state from the user preferences
   GetUserPreferences();
 
   // update the presShell: tell it to set the preference style rules up
   if (mShell) {
     mShell->SetPreferenceStyleRules(PR_TRUE);
   }
 
We will be giving up something here: namely, when a new font is installed the
suer will have to refresh the page to pick it up instead of it getting picked up
automatically. This seems like a small cost for now. In the long term, I think
that rbs' notion of being smarter about what has really changed is a great
approach. I hate to keep advocating hacks and wallpapers, but for the branch we
need to consider stability and risk first and foremost at this time.

I agree with waterson that we need to verify that normal pref changes act as
they always have (and I'm sure that they will- in fact, you will not need to
restart the browser to pick up new fonts either, just reload the page).
I like Marc's patch for the branch. We really need to give Shockwave a build for
testing and this seems like the safest way to go (pending our own testing). For
the trunk, we need to work on getting the object frame being able to handle a
reframe and I think Waterson has some code laying the groundwork.

r=peterl on Marc's patch for the branch. Can I get some super review(s)?

Whiteboard: nsBranch, pdt+ → nsBranch, [PDT+], [need sr=]
Blocks: 35916
sr=waterson
This has been fixed on the branch and trunk, marking FIXED. 

See bug 90268 object frame should be able to withstand a reframe or meta plugin
tracking cleanup bug 88998.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
ok verified on windows branch today(0711) that this is working again. Mac worked 
fine all the time..and linux does not have this plugin. So we are good to go. 
VERIFIED
Status: RESOLVED → VERIFIED
verified on windows trunk too 071104.
Um...I'm completely clueless as to what everyone said here, but could someone
explain how to fix this in english (I'm only 14)
You need to log in before you can comment on or make changes to this bug.