Closed Bug 1074976 Opened 5 years ago Closed 5 years ago

Deduplicate makeNicePluginName()

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: gfritzsche, Assigned: bruteforks, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

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
Mike, i stumbled over this duplication via bug 847072 - would you be interested in taking this?
Flags: needinfo?(bruteforks)
Georg, sure thing, I'll take this.
Flags: needinfo?(bruteforks)
Great, assigning to you.
Assignee: nobody → bruteforks
Attached patch bug1074976v1.0.patch (obsolete) — Splinter Review
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
Attachment #8497736 - Flags: review?(georg.fritzsche)
Attachment #8497736 - Flags: review?(georg.fritzsche) → feedback?(georg.fritzsche)
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.
Attachment #8497736 - Flags: feedback?(georg.fritzsche) → feedback+
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
Attached patch bug1074976v1.1.patch (obsolete) — Splinter Review
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.
Attachment #8497736 - Attachment is obsolete: true
Attachment #8498094 - Flags: review?(qamail.felipe)
Attachment #8498094 - Flags: review?(felipc)
Attachment #8498094 - Flags: review?(qamail.felipe)
Attachment #8498094 - Flags: review?(felipc) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
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/|.
Ah, I think I see what was wrong. This seems to work correctly...
Attachment #8498094 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/a86a4468774a
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a86a4468774a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.