Closed Bug 855611 Opened 7 years ago Closed 7 years ago

Convert navigator.plugins and navigator.mimeTypes to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jst, Assigned: jst)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

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...
OS: Linux → All
Hardware: x86_64 → All
Depends on: 855613
Attached patch Fix. (obsolete) — Splinter Review
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.
Attachment #731371 - Flags: review?(peterv)
Attached patch Remove dead interfaces/code. (obsolete) — Splinter Review
Attachment #731380 - Flags: review?(peterv)
Duplicate of this bug: 854243
Attached patch Fix. (obsolete) — Splinter Review
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.
Attachment #731371 - Attachment is obsolete: true
Attachment #731371 - Flags: review?(peterv)
Attachment #763873 - Flags: review?(peterv)
Updated to trunk.
Attachment #731380 - Attachment is obsolete: true
Attachment #731380 - Flags: review?(peterv)
Attachment #763875 - Flags: review?(peterv)
These patches will need a bit of merging to bug 885171, looks like.  :(  Sorry about that...
Assignee: jst → bzbarsky
Assignee: bzbarsky → jst
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.
Attachment #763873 - Flags: review?(peterv) → review-
(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...
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 :)
Attachment #763873 - Attachment is obsolete: true
Attachment #765749 - Attachment is obsolete: true
Attachment #768720 - Flags: review?(peterv)
Attachment #763875 - Flags: review?(peterv) → review+
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 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!
Attachment #768720 - Flags: review?(peterv) → review+
Pushed a follow-up to fix some sorting order issues: https://hg.mozilla.org/integration/mozilla-inbound/rev/5af7d6736c54
https://hg.mozilla.org/mozilla-central/rev/571787c97925
https://hg.mozilla.org/mozilla-central/rev/5af7d6736c54
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 896242
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.