Closed
Bug 1074976
Opened 10 years ago
Closed 10 years ago
Deduplicate makeNicePluginName()
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 35
People
(Reporter: gfritzsche, Assigned: bruteforks, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(1 file, 2 obsolete files)
8.26 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•10 years ago
|
||
Mike, i stumbled over this duplication via bug 847072 - would you be interested in taking this?
Flags: needinfo?(bruteforks)
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
Attachment #8497736 -
Flags: review?(georg.fritzsche)
Assignee | ||
Updated•10 years ago
|
Attachment #8497736 -
Flags: review?(georg.fritzsche) → feedback?(georg.fritzsche)
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.
Attachment #8497736 -
Flags: feedback?(georg.fritzsche) → feedback+
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
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.
Attachment #8497736 -
Attachment is obsolete: true
Attachment #8498094 -
Flags: review?(qamail.felipe)
Attachment #8498094 -
Flags: review?(felipc)
Reporter | ||
Updated•10 years ago
|
Attachment #8498094 -
Flags: review?(qamail.felipe)
Updated•10 years ago
|
Attachment #8498094 -
Flags: review?(felipc) → review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
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
Keywords: checkin-needed
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/|.
Assignee | ||
Comment 10•10 years ago
|
||
Ah, I think I see what was wrong. This seems to work correctly...
Attachment #8498094 -
Attachment is obsolete: true
Reporter | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=07819c13dcc0
Reporter | ||
Comment 12•10 years ago
|
||
Looks good here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=07819c13dcc0
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a86a4468774a
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a86a4468774a
Status: NEW → RESOLVED
Closed: 10 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.
Description
•