Georg Fritzsche [:gfritzsche] Reporter
	Description • 10 years ago

Currently we have two implementations of makeNicePluginName:
http://dxr.mozilla.org/mozilla-central/search?q=makeNicePluginName&case=true

We should move this to BrowserUtils and make the two locations use that instead:
http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/BrowserUtils.jsm

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 1 • 10 years ago

Mike, i stumbled over this duplication via bug 847072 - would you be interested in taking this?

	Mike Quested Assignee
	Comment 2 • 10 years ago

Georg, sure thing, I'll take this.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 3 • 10 years ago

Great, assigning to you.

	Mike Quested Assignee
	Comment 4 • 10 years ago

Ok, here is the patch. It works fine as far as I know.

I think I have done the module imports correctly, but you will have to check for me.

Cheers

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 5 • 10 years ago

Comment on attachment 8497736 [details] [diff] [review]
bug1074976v1.0.patch

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

That looks good, did you check that the click-to-play plugin notification and the permissions section in Tools->"Page Info" still get proper plugin names?

::: browser/base/content/pageinfo/permissions.js
@@ +274,5 @@
>      }
>      for (let mimeType of plugin.getMimeTypes()) {
>        let permString = pluginHost.getPermissionStringForType(mimeType);
>        if (!permissionMap.has(permString)) {
> +        var name = BrowserUtils.makeNicePluginName(plugin.name);

Lets make the |var| a |let| while we are here.

::: toolkit/modules/BrowserUtils.jsm
@@ +178,5 @@
>        return originalTarget;
>  
>      return "_blank";
>    },
> +  

Nit: trailing whitespace

@@ +181,5 @@
>    },
> +  
> +  /**
> +   * Map the plugin's name to a filtered version more suitable for user UI.
> +   * 

Trailing whitespace.

@@ +184,5 @@
> +   * Map the plugin's name to a filtered version more suitable for user UI.
> +   * 
> +   * @param aName The full-length name string for the plugin.
> +   * @return the simplified name string.
> +   */ 

Trailing whitespace.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 6 • 10 years ago

Going by the history of BrowserUtils.jsm [0], Felipe Gomes is good for review here.

[0] http://hg.mozilla.org/mozilla-central/filelog/14665b1de5ee/toolkit/modules/BrowserUtils.jsm

	Mike Quested Assignee
	Comment 7 • 10 years ago

Made suggested changes.

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> That looks good, did you check that the click-to-play plugin notification
> and the permissions section in Tools->"Page Info" still get proper plugin
> names?

Yes, the simplified plugin name appears in those places.

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 10 years ago

Please run this through Try first so we can make sure it won't break any existing tests :)
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 9 • 10 years ago

Comment on attachment 8498094 [details] [diff] [review]
bug1074976v1.1.patch

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

::: browser/modules/PluginContent.jsm
@@ +808,5 @@
>      }
>  
>      // For non-GMP plugins, remap the plugin name to a more user-presentable form.
>      if (!gmpPlugin) {
> +      pluginName = BrowserUtils.makeNicePluginName(pluginTag.name);

Doing a quick test-run locally, this fails because there is no local |pluginTag|. 
Please fix and try running |mach mochitest-browser browser/base/content/test/plugins/|.

	Mike Quested Assignee
	Comment 10 • 10 years ago

Ah, I think I see what was wrong. This seems to work correctly...

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 11 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=07819c13dcc0

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 12 • 10 years ago

Looks good here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=07819c13dcc0

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/a86a4468774a

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a86a4468774a