855611 - Convert navigator.plugins and navigator.mimeTypes to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Johnny Stenback (:jst)]

navigator.plugins and navigator.mimeTypes uses XPCOM IDL interfaces and are exposed to the web through XPConnect. We should convert them to use WebIDL and the new bindings mechanism to expose them instead. This will let us clean up a bunch of code as well.

I have a patch in the works...

Comment 1

[Johnny Stenback (:jst)]

Attachment: Fix.

This makes us use new bindings for navigator.plugins and navigator.mimeTypes and the stuff that hangs off of that as well. I'll also attach a followup patch to remove the old interfaces and some other dead code that's left around. This builds on top of the patch in bug 855613.

Comment 2

[Johnny Stenback (:jst)]

Attachment: Remove dead interfaces/code.

Comment 4

[Johnny Stenback (:jst)]

Attachment: Fix.

Updated to tip. This is the same patch as the old version, except it's merged to trunk. The biggest change that happened to trunk is that the nsPluginArray class is now an nsIObserver instead of Navigator being an observer, the rest is the same except some type changes etc.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Remove dead interfaces/code.

Updated to trunk.

Comment 6

[Boris Zbarsky [:bzbarsky]]

These patches will need a bit of merging to bug 885171, looks like.  :(  Sorry about that...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Maybe this is merged right?

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 763873 [details] [diff] [review]
Fix.

Review of attachment 763873 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. -'ed mostly for the edges missing in the CC stuff and the bugs in nsMimeTypeArray::NamedGetter.

::: dom/base/Navigator.cpp
@@ +594,2 @@
>  
> +  *aReturn = mimeType && !!mimeType->GetEnabledPlugin();

No need for !!, the && will normalize the value.

::: dom/base/nsMimeTypeArray.cpp
@@ +19,5 @@
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsMimeTypeArray)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsMimeTypeArray)

This should traverse/unlink mMimeTypes.

@@ +132,5 @@
> +  // Now we check whether we can really claim to support this type
> +  nsHandlerInfoAction action = nsIHandlerInfo::saveToDisk;
> +  mimeInfo->GetPreferredAction(&action);
> +  if (action == nsIMIMEInfo::handleInternally) {
> +    return nullptr;

This is different and seems wrong. We should return a nsMimeType here.

@@ +138,5 @@
>  
> +  bool hasHelper = false;
> +  mimeInfo->GetHasDefaultHandler(&hasHelper);
> +  if (hasHelper) {
> +    return nullptr;

Same here.

@@ +144,5 @@
>  
> +  nsCOMPtr<nsIHandlerApp> helper;
> +  mimeInfo->GetPreferredApplicationHandler(getter_AddRefs(helper));
> +  if (helper) {
> +    return nullptr;

Same here.

@@ +183,2 @@
>  {
> +  aRetval.Clear();

No need for the Clear.

@@ +201,2 @@
>  
> +  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));

Null-check win?

@@ +217,5 @@
> +
> +  nsTArray<nsRefPtr<nsPluginElement> > plugins;
> +  pluginArray->GetPlugins(plugins);
> +
> +  for (uint32_t i = 0; i < plugins.Length(); i++) {

Nit: ++i

@@ +221,5 @@
> +  for (uint32_t i = 0; i < plugins.Length(); i++) {
> +    nsPluginElement *plugin = plugins[i];
> +
> +    for (uint32_t j = 0; j < plugin->Length(); j++) {
> +      mMimeTypes.AppendElement(plugin->Item(j));

Given that plugin is a nsPluginElement it might make sense to do something like |mMimeTypes.AppendElements(plugin->MimeTypes());|?

::: dom/base/nsMimeTypeArray.h
@@ +61,3 @@
>  };
>  
> +class nsMimeType MOZ_FINAL : public nsISupports,

This doesn't really need to inherit from nsISupports, you could mark it as 'nativeOwnership': 'refcounted', use the _NATIVE CC macros and NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING.

@@ +89,5 @@
> +  // MimeType WebIDL methods
> +  void GetDescription(nsString& retval) const;
> +  nsPluginElement *GetEnabledPlugin() const;
> +  void GetSuffixes(nsString& retval) const;
> +  void GetType(nsString& retval) const;

Maybe inline these?

::: dom/base/nsPluginArray.cpp
@@ +63,3 @@
>  
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsPluginArray)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

This should traverse/unlink mPlugins.

@@ +83,3 @@
>  
>    if (!AllowPlugins())
> +    return;

Add braces.

@@ +116,4 @@
>  
> +  // NS_ERROR_PLUGINS_PLUGINSNOTCHANGED on reloading plugins indicates
> +  // that plugins did not change and was not reloaded
> +  bool pluginsChanged = true;

Drop pluginsChanged.

@@ +118,5 @@
> +  // that plugins did not change and was not reloaded
> +  bool pluginsChanged = true;
> +
> +  // currentPluginCount is as reported by nsPluginHost. mPluginCount is
> +  // essentially a cache of this value, and may be out of date.

This comment looks obsolete?

@@ +131,5 @@
> +    // notified for every plugin enabling/disabling event that
> +    // happens, and therefore the lengths will be in sync only when
> +    // the both arrays contain the same plugin tags (though as
> +    // different types).
> +    pluginsChanged = newPluginTags.Length() != mPlugins.Length();

Just return early here if newPluginTags.Length() == mPlugins.Length()

@@ +168,3 @@
>  
> +  if (!AllowPlugins())
> +    return nullptr;

Add braces.

@@ +196,2 @@
>    if (!AllowPlugins())
> +    return nullptr;

Add braces.

@@ +201,3 @@
>    }
>  
> +  for (uint32_t i = 0; i < mPlugins.Length(); i++) {

++i

@@ +218,5 @@
> +uint32_t
> +nsPluginArray::Length()
> +{
> +  if (!AllowPlugins())
> +    return 0;

Add braces.

@@ +233,5 @@
>  {
> +  aRetval.Clear();
> +
> +  if (!AllowPlugins())
> +    return;

Add braces.

@@ +279,4 @@
>  
> +  // need to wrap each of these with a nsPluginElement, which is
> +  // scriptable.
> +  for (uint32_t i = 0; i < pluginTags.Length(); i++) {

++i

@@ +292,3 @@
>  
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsPluginElement)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

This should traverse/unlink mMimeTypes.

@@ +315,2 @@
>  {
> +  for (uint32_t i = 0; i < mMimeTypes.Length(); i++) {

++i

@@ +367,2 @@
>  
> +  return aIndex >= mMimeTypes.Length() ? nullptr : mMimeTypes[aIndex];

Use SafeElementAt(aIndex).

@@ +413,5 @@
>  
> +void
> +nsPluginElement::GetSupportedNames(nsTArray< nsString >& retval)
> +{
> +  retval.Clear();

No need for the Clear.

@@ +417,5 @@
> +  retval.Clear();
> +
> +  EnsureMimeTypes();
> +
> +  for (uint32_t i = 0; i < mMimeTypes.Length(); i++) {

++i

@@ +429,5 @@
> +  if (!mMimeTypes.IsEmpty()) {
> +    return;
> +  }
> +
> +  for (uint32_t i = 0; i < mPluginTag->mMimeTypes.Length(); i++) {

++i

::: dom/base/nsPluginArray.h
@@ +69,3 @@
>  };
>  
> +class nsPluginElement MOZ_FINAL : public nsISupports,

Also doesn't need to inherit from nsISupports.

Comment 9

[Johnny Stenback (:jst)]

(In reply to Peter Van der Beken [:peterv] from comment #8)
[...]
> > +class nsPluginElement MOZ_FINAL : public nsISupports,
> 
> Also doesn't need to inherit from nsISupports.

BindingGen.py says it does:

TypeError: We don't support non-nsISupports native classes for proxy-based bindings yet (Plugin)

bz, thanks for the merge help! New patches coming once they've been through try...

Comment 10

[Johnny Stenback (:jst)]

Attachment: Fixes for peterv's review comments.

This inter diff I believe fixes all of peterv's review comments except for the inheritance, and I opted not to inline the MimeType WebIDL methods since they're not all trivial, performance doesn't really matter here, and I like consistency :)

Comment 11

[Johnny Stenback (:jst)]

Attachment: Complete patch (including dead code removal), merged to current trunk.

Comment 12

[Andrew McCreight [:mccr8]]

You are going to have to update the CC stuff again, on top of the patch from bug 886213 that I just landed.

Delete the line in Unlink
  // mMimeTypes isn't cycle collected
because you are now clearing that in Invalidate().

Also replace the lines in Navigator's Traverse:
  // mMimeTypes isn't cycle collected
  // mPlugins isn't cycle collected
with
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMimeTypes)
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMimePlugins)

That should be it.

Comment 13

[Johnny Stenback (:jst)]

Attachment: Complete patch update to trunk (including the CC stuff that mccr8 pointed out).

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 768720 [details] [diff] [review]
Fixes for peterv's review comments.

Review of attachment 768720 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPluginArray.cpp
@@ +122,5 @@
>      // happens, and therefore the lengths will be in sync only when
>      // the both arrays contain the same plugin tags (though as
>      // different types).
> +    if (newPluginTags.Length() == mPlugins.Length())
> +      return;

Braces!

Comment 15

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/571787c97925

Comment 16

[(no longer active)]

Pushed a follow-up to fix some sorting order issues: https://hg.mozilla.org/integration/mozilla-inbound/rev/5af7d6736c54

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/571787c97925
https://hg.mozilla.org/mozilla-central/rev/5af7d6736c54