This is basically the patches in bug 558184, parts 1, 4.x, 5, but with some changes to introduce an nsIInternalPluginTag so we don't have to mess around with the nasty XPCOM-y nsIPluginTag API.

This is my planned replacement for the "make everyone use nsIPluginTag" approach we had before. The nsMimeTypeArray bits are basically as John had them, but the other stuff seems a bit cleaner.

The major change from John's stuff is that I have setter methods on nsIFakePluginTag and that nsFakePluginTag now multiply-inherits from nsIPluginTag (once via nsIFakePluginTag and once via nsIInternalPluginTag). If we're not happy with the multiple inheritance, I suggest we make nsIFakePluginTag not inherit from nsIPluginTag... There are still a lot of fixme comments, but I think the best way to fix most of those will in fact be to just go ahead and make it impossible to modify an nsIFakePluginTag, but make it possible to pass the relevant information when creating one.

This is not ready yet; it builds but I haven't really looked through it carefully. It does illustrate the annoying static_casts we need due to the multiple inheritance. Mostly posting this just so there's a bit more context for how the previous patches get used in practice; it's probably a bit annoying to review them otherwise. Note that I think some of the GetActive() bits in this patch can be de-COMified.

Comment on attachment 8628469 [details] [diff] [review] Introduce fake plugin tags Oh, and this still has the supersede bits, but I'm tempted to say we should nix those and just always treat as superseding unless caller explicitly says to skip jsplugins. I just haven't gotten far enough on the next patch(es) yet to definitively make that call.

Comment on attachment 8628469 [details] [diff] [review] Introduce fake plugin tags This is changing a good bit based on discussion Peter and I had.

I think thsi is pretty ready for review now. There are some fixme comments left, but I'm OK fixing them up once we do the thing we want to do on top of this stuff.

This is basically "part 4.2" from bug 558184. I have parts 5, 6, 7 more or less merged to at least apply on top of this stuff (though I haven't checked they compile yet); can attach those if you'd like. But I figured I should see what the review comments on this look like at this point.

Comment on attachment 8628468 [details] [diff] [review] Introduce nsIInternalPluginTag Review of attachment 8628468 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginHost.cpp @@ +195,5 @@ > {} > > // Helper to check for a MIME in a comma-delimited preference > static bool > +IsTypeInList(const nsCString &aMimeType, nsCString aTypeList) Maybe move the ampersand before the space. There's a couple of others like this.

Comment on attachment 8631973 [details] [diff] [review] Introduce fake plugin tags Review of attachment 8631973 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginTags.cpp @@ +151,5 @@ > } > > +static nsresult > +IsEnabledStateLockedForPlugin(nsIInternalPluginTag *aTag, > + bool *aIsEnabledStateLocked) * before space. @@ +204,5 @@ > { > } > > +bool > +nsIInternalPluginTag::HasExtension(const nsACString & aExtension, Remove the space before &. Here and elsewhere.

Comment on attachment 8631973 [details] [diff] [review] Introduce fake plugin tags Review of attachment 8631973 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsIPluginTag.idl @@ +60,5 @@ > out wstring aResults); > }; > + > +[scriptable, uuid(6d22c968-226d-4156-b230-da6ad6bbf6e8)] > +interface nsIFakePluginTag : nsIPluginTag We should probably explain what a fake plugin is here.

Comment on attachment 8631978 [details] [diff] [review] Implement the pluginhost parts of fake plugin tags Review of attachment 8631978 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsIPluginHost.idl @@ +163,5 @@ > + > + /** > + * Create a fake plugin tag, register it, and return it. The > + * argument is a FakePluginTagInit dictionary. See documentation in > + * FakePluginTagInit.webdil for what it should look like. .webidl ::: dom/plugins/base/nsPluginHost.cpp @@ +1011,5 @@ > { > bool checkEnabled = aFilter & eExcludeDisabled; > + bool allowFake = !(aFilter & eExcludeFake); > + return FindNativePluginForType(aMimeType, checkEnabled) || > + (allowFake && FindFakePluginForType(aMimeType, checkEnabled)); This could just call FindPluginForType. @@ +1563,5 @@ > + FakePluginTagInit initDictionary; > + if (!initDictionary.Init(aCx, aInitDictionary)) { > + return NS_ERROR_FAILURE; > + } > + Trailing whitespace. @@ +1565,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + nsRefPtr<nsFakePluginTag> newTag; > + nsresult rv = nsFakePluginTag::Create(initDictionary, getter_AddRefs(newTag)); Do we need to guard against adding the same plugin twice? @@ +1647,5 @@ > > +// FIXME-jsplugins Is this method actually needed? > +NS_IMETHODIMP > +nsPluginHost::GetFakePlugin(const nsACString & aMimeType, > + nsIFakePluginTag** aResult) Can we make this call FindFakePluginForType? ::: dom/plugins/base/nsPluginTags.cpp @@ +792,5 @@ > +nsFakePluginTag::Create(const FakePluginTagInit& aInitDictionary, > + nsFakePluginTag** aPluginTag) > +{ > + NS_ENSURE_TRUE(!aInitDictionary.mMimeEntries.IsEmpty(), NS_ERROR_INVALID_ARG); > + Trailing whitespace.

Fixed the various whitespace nits. > We should probably explain what a fake plugin is here. /** * An interface representing a "fake" plugin: one implemented in JavaScript, not * as an NPAPI plug-in. See nsIPluginHost.registerFakePlugin and the * documetation for the FakePluginTagInit dictionary. */ > This could just call FindPluginForType. Yep, fixed. > Do we need to guard against adding the same plugin twice? You mean with the same URI but different type lists? Or different URIs for the same type? Something else? I wasn't really quite sure what to guard against here... I'm happy to do it if you have a concrete idea of what we should prevent. > Can we make this call FindFakePluginForType? Yes. Good catch.

OK, so two more things. First of all, the patch in bug 1180857 assumes that once createFakePlugin exists it should be used. So I should probably mark it [noscript] for now, until we land bug 1186577. Second, that patch should probably get updated to the API changes in this bug. The "Implement the pluginhost parts of fake plugin tags" patch has the new API; it should be a pretty simple tweak (and now we have unregistration!).

(In reply to Boris Zbarsky [:bz] from comment #12) > * as an NPAPI plug-in. See nsIPluginHost.registerFakePlugin and the s/an/a/ > * documetation for the FakePluginTagInit dictionary. s/documetation/documentation/ > You mean with the same URI but different type lists? Or different URIs for > the same type? Something else? I wasn't really quite sure what to guard > against here... I'm happy to do it if you have a concrete idea of what we > should prevent. The simplest is probably against registering twice with the same URI. I don't see a usecase for that anyway?

> The simplest is probably against registering twice with the same URI OK, did this right after creating newTag in RegisterFakePlugin: for (auto existingTag : mFakePlugins) { if (newTag->HandlerURIMatches(existingTag->HandlerURI())) { return NS_ERROR_UNEXPECTED; } } and added a HandlerURI accessor.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae321edfafcc https://hg.mozilla.org/integration/mozilla-inbound/rev/a73ee6e0fc92 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d96b3d42fd9

https://hg.mozilla.org/mozilla-central/rev/ae321edfafcc https://hg.mozilla.org/mozilla-central/rev/a73ee6e0fc92 https://hg.mozilla.org/mozilla-central/rev/4d96b3d42fd9

(In reply to On vacation August 4-25. Please mail manually if really needed. from comment #13) > OK, so two more things. First of all, the patch in bug 1180857 assumes that > once createFakePlugin exists it should be used. So I should probably mark > it [noscript] for now, until we land bug 1186577. > > Second, that patch should probably get updated to the API changes in this > bug. The "Implement the pluginhost parts of fake plugin tags" patch has the > new API; it should be a pretty simple tweak (and now we have > unregistration!). I fixed the Shumway to use new method signatures: https://github.com/mozilla/shumway/pull/2324 . I will update bug 1180857 to have this patch.