1061967 - Move checking for special-cased plugin types to a central spot

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

Splitting out from bug 558184 - various spots in the tree are checking mime type strings for java/flash, some less consistently than others.

Comment 1

[John Schoenick [:johns]]

Attachment: Move checking for special-cased plugin types to a central spot

Adds nsPluginHost::IsSpecialType() to check for all the various MIME types we sniff, removes a ton of said checks from various spots. Many checks were not checking all possible mime types.

Comment 2

[John Schoenick [:johns]]

Attachment: Bonus whitespace cleanup

Comment 3

[John Schoenick [:johns]]

https://tbpl.mozilla.org/?tree=Try&rev=10c9cfce49c7

Comment 4

[John Schoenick [:johns]]

Attachment: Move checking for special-cased plugin types to a central spot

New and improved, now compiles on OSes other than my own.

Comment 5

[John Schoenick [:johns]]

(In reply to John Schoenick [:johns] from comment #3)
> https://tbpl.mozilla.org/?tree=Try&rev=10c9cfce49c7

(In reply to John Schoenick [:johns] from comment #4)
> Created attachment 8486591 [details] [diff] [review]
> Move checking for special-cased plugin types to a central spot
> 
> New and improved, now compiles on OSes other than my own.

Let's try that again
https://tbpl.mozilla.org/?tree=Try&rev=a9f6fe815968

Comment 6

[John Schoenick [:johns]]

Attachment: Move checking for special-cased plugin types to a central spot

And then we found out that Preferences::GetCString() can return invalid strings that crash. Woo.

Attempt #3 went better

https://tbpl.mozilla.org/?tree=Try&rev=8222336cd7d4
https://tbpl.mozilla.org/?tree=Try&rev=96ea8a21e1c5

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8487354 [details] [diff] [review]
Move checking for special-cased plugin types to a central spot

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

That's a nice cleanup, adding some driveby commentary :)

::: content/base/src/nsObjectLoadingContent.cpp
@@ +115,5 @@
>  
>  static bool
> +IsJavaMIME(const nsACString & aMIMEType)
> +{
> +  return nsPluginHost::IsSpecialType(aMIMEType) == \

Nit: Left-over back-slash?

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +537,5 @@
>    // bug 108347, flash plugin on linux doesn't like window->width <=
>    // 0, but Java needs wants this call.
> +  if (window && window->type == NPWindowTypeWindow &&
> +      (window->width <= 0 || window->height <= 0) &&
> +      (nsPluginHost::IsSpecialType(nsDependentCString(mMIMEType)) != \

Nit: Left-over backslash?

::: dom/plugins/base/nsPluginHost.cpp
@@ +1577,5 @@
> +    return eSpecialType_Silverlight;
> +  }
> +
> +  if (aMIMEType.LowerCaseEqualsASCII("audio/x-pn-realaudio-plugin")) {
> +    NS_WARNING("You are loading real player");

Nit: Why does this plugin specifically get a warning?

::: dom/plugins/base/nsPluginHost.h
@@ +172,5 @@
> +                     // Native widget quirks
> +                     eSpecialType_PDF,
> +                     // Native widget quirks
> +                     eSpecialType_RealPlayer };
> +  static SpecialType IsSpecialType(const nsACString & aMIMEType);

Usually |IsFoo()| functions would be expected to return a boolean though, maybe |GetSpecialType()|?

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +830,5 @@
>  
>    // Flash invokes InvalidateRect for us.
>    const char* mime = nullptr;
> +  if (NS_SUCCEEDED(mInstance->GetMIMEType(&mime)) && mime &&
> +      nsPluginHost::IsSpecialType(nsDependentCString(mime)) == \

Nit: backslash

::: dom/plugins/base/nsPluginTags.cpp
@@ +162,5 @@
> +        break;
> +      case nsPluginHost::eSpecialType_Flash:
> +        mIsFlashPlugin = true;
> +        break;
> +      case nsPluginHost::eSpecialType_None:

This line seems redundant?

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

"You are loading real player" should probably be "You are loading RealPlayer".

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8487354 [details] [diff] [review]
Move checking for special-cased plugin types to a central spot

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

::: dom/plugins/base/nsPluginTags.cpp
@@ +155,5 @@
>        continue;
>      }
>  
>      // Look for certain special plugins.
> +    switch (nsPluginHost::IsSpecialType(mimeType)) {

Looking here again from the bug 1074822 context... can't we just store this as |nsPluginTag::mSpecialType| instead and get rid of mIsFlashPlugin & mIsJavaPlugin and the switching here?

We could still have helper functions for convenience if needed.

Comment 10

[Chris Peterson [:cpeterson]]

Boris: bsmedberg r+'d this bug's jsplugin-related patches before johns left, but they never landed. Should these patches land with your jsplugins work?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Probably, yes.  Thanks for the pointer!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1 - Move checking for special-cased plugin types to a central spot

Addresses the review comments, except comment 9: we can't really use nsPluginHost::SpecialType there, so needs a bit more thought on how best to store this

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2 - Bonus whitespace cleanup

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc3ebb021f01

Comment 15

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/469e07a25f44
   https://hg.mozilla.org/integration/mozilla-inbound/rev/234d306dfbba

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/469e07a25f44
https://hg.mozilla.org/mozilla-central/rev/234d306dfbba