bug 1156 broke pluginfinder, it needs to be fixed (using XBL). This bug includes removing the IsSupported* functions from nsObjectFrame and other PFS-related code.

objectframe cleanup will be done in part II, together with making the puzzlepiece appear again. this patch makes the infobar appear again when unsupported plugins are found.

Still reading through this, but firing events sync from ObjectURIChanged is bad -- consider an event handler that changes an attr, calling ObjectURIChanged? See what images do for onerror/onload events; perhaps we need to factor that code out into nsContentUtils or something?

all right, then. the nsImageLoadingContent stuff looked special in some respects, so I didn't reuse that.

oh, I ran into a major problem with part II... it requires support for inline-block :( (because I want to be able to size the <object> to the requested size, but keep text flow around it like display:inline, i.e. like it'd be if it would display normally)

Hmm... Can we create some random thing like an nsAreaFrame here? And just let it size?

Thinking about it some more, inline-block would not help at all, really, since if the page sets display we'd render with that display (say inline). Unless you were planning to use !important? But that breaks if the page uses display:block... I do still think that using some sort of random frame class that allows kids and can deal with either display (and nsAreaFrame is a decent example) is a good idea here.

but that'd mean that I can't do this with XBL only, right?

Hmm... yes. With XBL only you can do no better than just using the original display type and giving up on the sizing for inline objects...

if that's ok with you and dbaron... the code I currently have creates this structure: <object style="display:inline;"><a style="display:block; width: inherit; height:inherit;">...</a></object> But clearly that makes all content after that appear on the next line. (I intend to only attach that once part I is checked in)

Why not use display:inherit on the <a> instead?

hm... http://stud4.tuwien.ac.at/~e0225227/nullplug.png shows usage of display:inherit; for all affected elements; the page doesn't style <object>s itself. doesn't look too great, and of course it ignores the specified size of the object. Compare http://stud4.tuwien.ac.at/~e0225227/nullplug_block.png, which uses that block-inside-inline setup.

It's OK with me; hopefully we'll have inline-block actually working by 1.9 and you can use it then.

and, bz: I think I can actually use inline-block here; I'll just have to use it in the (anonymous) child of the object. right?

Ah, yes. If we're styling the anonymous child then using inline-block should work great. Given those screenshots, let's use display:block for now.

Comment on attachment 201679 [details] [diff] [review] part I: Fire the event, v2 >Index: content/base/src/nsObjectLoadingContent.cpp >+nsObjectLoadingContent::FirePluginNotFound() Make this static and takling an nsIContent? This doesn't actually use any members, and all callers have an nsIContent on hand anyway... You can remove some nsObjectFrame code at this point, no? r+sr=bzbarsky

Checking in content/base/src/nsObjectLoadingContent.cpp; /cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v <-- nsObjectLoadingContent.cpp new revision: 1.13; previous revision: 1.12 done Checking in content/base/src/nsObjectLoadingContent.h; /cvsroot/mozilla/content/base/src/nsObjectLoadingContent.h,v <-- nsObjectLoadingContent.h new revision: 1.5; previous revision: 1.4 done

OK, the long-awaited part II. This includes the nsObjectFrame cleanup. Some notes: - This introduces two new CSS pseudo-classes, :-moz-has-handlerref (for plugin elements with a pluginurl attribute) and :-moz-type-unsupported (for plugin objects that use an unsupported type) - This contains an XBL-ized pluginfinder puzzlepiece. Since this runs, as far as I know, with page privileges, the PluginNotFound listener has to accept untrusted events as well, and thus verifies that the element is actually a plugin - PluginNotAvailable is never called on the instance owner, as best as I can tell (we always have a mime type to pass to the pluginhost, except in some cases when we have an unsupported URL scheme; but then, the plugin host can't find a type either). I made it a no-op. - I added the missingPlugin.css file to the other two themes (qute, pinstripe) too, but that's not in the diff because the directory is new and I didn't want to add it until I have reviews - As long as I can't use inline-block, this causes a slight regression in page rendering when the puzzle piece is shown (it splits lines).

Comment on attachment 202802 [details] [diff] [review] part II: XBL-ized pluginfinder mconnor, please review the toolkit/ changes in this patch (and note the previous comment re themes)

Oh, I forgot to mention... The reason this uses an xul:image instead of an HTML one is so that I can specify the image location in the theme, rather than having to hardcode the URL.

Comment on attachment 202802 [details] [diff] [review] part II: XBL-ized pluginfinder >Index: browser/base/content/browser.js >+ gBrowser.addEventListener("PluginNotFound", gMissingPluginInstaller.newMissingPlugin, false, true); Add a comment that explains that this is listening to untrusted events only because missingPlugin.xml dispatches them? That said, is there anything we can do to make this event trusted? If not, I'd like mconnor to have a good hard look at what this newMissingPlugin method does and what happens if a page fires this event on some random <object> of its choosing at some random point... >Index: layout/generic/nsObjectFrame.cpp > PluginNotAvailable is never called on the instance > owner, as best as I can tell Then we should remove it. It was only added to support the plugin finder; if we're not using it for that anymore, it should go away, imo. Separate bug if you wish. >Index: layout/style/nsCSSStyleSheet.cpp >+ else if (nsCSSPseudoClasses::mozHasHandlerRef == pseudoClass->mAtom) { >+ nsIContent *element = data.mContent; That can be null, actually, in which case, this crashes. I see that a few of the other things here (:empty, mozEmptyExceptChildrenWithLocalname) have the same problem (sad how I can see these sort of things in _other_ people's patches, but not my own....), so if you want to, just file a followup bug on me to fix that? Should this selector really match any node that has a child <html:param name="pluginurl">? I guess there's really no harm in that, eh? >Index: toolkit/mozapps/plugins/Makefile.in >+# The Initial Developer of the Original Code is >+# Netscape Communications Corporation. ... >+# Contributor(s): >+# Ben Goodger <ben@bengoodger.com> Really? I guess if you made an exact copy of some other makefile... >Index: toolkit/mozapps/plugins/pluginGlue.js Same here; I doubt this file was written by >+ * Bill Law <law@netscape.com> >+ * Scott MacGregor <mscott@netscape.com> in the form it's in now. ;) >+var module = { >+ categoryEntry: "pluginfinder xbl binding", Should we try harder to make sure that's a unique string (eg use a uuid here)? Or is readability more important? Document somewhere near the top of this file that this component just exists to register the category entry and doesn't actually register anything with the component manager? >Index: toolkit/mozapps/plugins/content/missingPlugin.xml >+ xmlns:xbl="http://www.mozilla.org/xbl" I think this file would be more readable if this were the default namespace here. >+ <a href="#"> Please document why this is here (so that it can be tabbed to, etc). It's not obvious otherwise, and CVS blame trail is kinda long. >Index: toolkit/mozapps/plugins/content/missingPluginBinding.css You need @namespace url(http://www.w3.org/1999/xhtml); /* set default namespace to HTML */ or so in this file before the actual rule. Otherwise this will trigger for non-HTML <embed>, etc tags, at least in theory (they won't match :-moz-type-unsupported, but that's a separate issue). >Index: toolkit/themes/winstripe/mozapps/plugins/missingPlugin.css >+object > *|*, >+embed > *|*, >+applet > *|*{ ... >+a { Let's use the right namespace here too, for good measure. I don't think you want HTML as the default (since you have a * rule), but perhaps declare it with a prefix and use that prefix here? Also, space between "*|*" and '{', please. Note that width:inherit and height:inherit won't quite do the right thing because the <a> has botders and padding. So for example "height:100%" will make the <a> stick out of its parent. I'm not sure what we can do about that yet; perhaps a separate bug on this. r+sr=bzbarsky with those nits picked. Good to see all that C++ code go away!

> That said, is there anything we can do to make this event trusted? XBL runs with page privileges, does it not?

Yes. The real question is whether we can do anything to fire it from C++... I doubt there is. :(

yeah, I don't think so either. short of listening to mouseclicks in nsObjectLoadingContent and firing the event from there, but that'd suck.

(In reply to comment #20) > Should this selector really match any node that has a child <html:param > name="pluginurl">? I guess there's really no harm in that, eh? Yeah, I see no problem with that. This selector doesn't really make sense on non-<object>s anyway. But I don't feel it's worth any code to check for that. > Really? I guess if you made an exact copy of some other makefile... I copied the one from the parent dir and just added the EXTRA_COMPONENTS line... but I suppose there isn't really any real content, so I might as well use a new header. > Same here; I doubt this file was written by > >+ * Bill Law <law@netscape.com> > >+ * Scott MacGregor <mscott@netscape.com> Well, I did copy nsHelperAppDlg.js :-) But true, not much of it left. > Should we try harder to make sure that's a unique string (eg use a uuid here)? > Or is readability more important? We don't do that in other places... and note that xforms also uses a readable string here. If we want something unique, I'd go for using the URL as the name. > >Index: toolkit/mozapps/plugins/content/missingPlugin.xml > >+ xmlns:xbl="http://www.mozilla.org/xbl" > > I think this file would be more readable if this were the default namespace > here. Hmm, ok. > Also, space between "*|*" and '{', please. Oh, that must have got lost at some point.

Comment on attachment 203074 [details] [diff] [review] part II: XBL-ized pluginfinder, v2 ok, looks good to me, though I'm not 100% sure about the a11y part and whether we need to implement nsIAccessibleProvider here. Please mail aaronlev to confirm (doesn't have to be before checkin)

fixed on trunk. mail sent to aaronlev.

Linux luna Dep (SeaMonkey) Z:18.95MB Zdiff:-4244 (+481/-4725) mZ:11.89MB mZdiff:-4244 (+481/-4725)

Could this have caused Bug 322833 ?