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

RESOLVED FIXED in Firefox 37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dao, Assigned: abdelrhman, Mentored)

Tracking

Trunk
Firefox 37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 8536519 [details] [diff] [review]
rev 1 - Using Services.focus instead of Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager)
Attachment #8536519 - Flags: review?(dao)
(Reporter)

Comment 2

4 years ago
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-
(Reporter)

Updated

4 years ago
Assignee: nobody → codo.abdo
(Assignee)

Comment 3

4 years ago
Created attachment 8537216 [details] [diff] [review]
rev 2 - Using Services.focus instead of Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager)

Changes made after updating mozilla-central
Attachment #8537216 - Flags: review?(dao)
(Reporter)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 8537364 [details] [diff] [review]
rev 3 - Using Services.focus instead of Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager)
Attachment #8537364 - Flags: review?(dao)
(Reporter)

Comment 6

4 years ago
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+
(Reporter)

Updated

4 years ago
Attachment #8537216 - Attachment is obsolete: true
Attachment #8537216 - Flags: review?(dao)
(Reporter)

Updated

4 years ago
Attachment #8536519 - Attachment is obsolete: true
(Reporter)

Comment 8

4 years ago
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.
(Reporter)

Comment 9

4 years ago
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
Last Resolved: 4 years ago
Flags: needinfo?(cbook)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37

Updated

4 years ago
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.