Closed Bug 1110109 Opened 8 years ago Closed 8 years ago

Use Services.focus instead of Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager) in browser/base/

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 37

People

(Reporter: dao, Assigned: abdelrahman, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

Services.focus should be faster and is prettier than the XPCOM getService calls.

Affected files:

http://mxr.mozilla.org/mozilla-central/search?string=nsifocusman&find=%2Fbrowser%2Fbase%2F
Comment on attachment 8536519 [details] [diff] [review]
rev 1 - Using Services.focus instead of Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager)

>--- a/browser/base/content/browser-tabPreviews.js
>+++ b/browser/base/content/browser-tabPreviews.js
>@@ -128,18 +130,17 @@ var tabPreviewPanelHelper = {
>     if ("setupGUI" in host)
>       host.setupGUI();
>   },
>   _popuphiding: function (host) {
>     if ("suspendGUI" in host)
>       host.suspendGUI();
> 
>     if (host._prevFocus) {
>-      Cc["@mozilla.org/focus-manager;1"]
>-        .getService(Ci.nsIFocusManager)
>+      Services.focus
>         .setFocus(host._prevFocus, Ci.nsIFocusManager.FLAG_NOSCROLL);

nit: the last line should be indented further such that ".focus" and ".setFocus" line up.

This patch basically looks good, but please remove the calls to Components.utils.import. They are redundant here, utilityOverlay.js already has you covered: http://hg.mozilla.org/mozilla-central/annotate/5d6e0d038f95/browser/base/content/utilityOverlay.js#l7
Attachment #8536519 - Flags: review?(dao) → review-
Assignee: nobody → codo.abdo
Changes made after updating mozilla-central
Attachment #8537216 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #2)
> but please remove the calls to Components.utils.import.

These are still there in your new patch. Seems like you misunderstand my comment?

Also, can you please drop the utilityOverlay.js changes? They're off-topic for this bug.
Comment on attachment 8537364 [details] [diff] [review]
rev 3 - Using Services.focus instead of Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager)

Thanks!
Attachment #8537364 - Flags: review?(dao) → review+
Attachment #8537216 - Attachment is obsolete: true
Attachment #8537216 - Flags: review?(dao)
Attachment #8536519 - Attachment is obsolete: true
Abdelrhman, if you're interested in fixing slightly more involved bugs, you might want to take a look at bug 1112552 or bug 1112556 that I just filed.
Again, it looks like this bug wasn't updated when the patch was merged to mozilla-central.

https://hg.mozilla.org/mozilla-central/rev/1857b132f618
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(cbook)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.