Closed Bug 1126253 Opened 5 years ago Closed 5 years ago

Remove redundant NewTabUtils.jsm import from browser-thumbnails.js

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: dao, Assigned: curtisk, Mentored)

Details

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

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1125497 +++

At the top of browser/base/content/browser-thumbnails.js, we import NewTabUtils.jsm because some code in browser-thumbnails.js wants to use NewTabUtils. We can remove this import, because browser.js already imports NewTabUtils.jsm (http://hg.mozilla.org/mozilla-central/annotate/38e4719e71af/browser/base/content/browser.js#l29).
I've always wanted to land a bug and learn how this works and this seems like a good one to learn on. :dao are you the mentor?
Attachment #8555512 - Flags: review?(dao)
(In reply to Curtis Koenig [:curtisk (non-Moz)] from comment #1)
> I've always wanted to land a bug and learn how this works and this seems
> like a good one to learn on. :dao are you the mentor?

I am...
Comment on attachment 8555512 [details] [diff] [review]
Bug-1126253.patch

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -21,18 +21,16 @@ XPCOMUtils.defineLazyModuleGetter(this, 
> XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                   "resource://gre/modules/Task.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "CharsetMenu",
>                                   "resource://gre/modules/CharsetMenu.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "ShortcutUtils",
>                                   "resource://gre/modules/ShortcutUtils.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "GMPInstallManager",
>                                   "resource://gre/modules/GMPInstallManager.jsm");
>-XPCOMUtils.defineLazyModuleGetter(this, "NewTabUtils",
>-                                  "resource://gre/modules/NewTabUtils.jsm");

Sorry, that's the wrong way around. We need to remove the import from browser-thumbnails.js, not from browser.js.
Attachment #8555512 - Flags: review?(dao) → review-
so I think I have this figured out (what to change for the appropriate patch) but I am confused on MozReview on how to submit this patch and I can't seem to find you (:dao) on irc
Attachment #8556241 - Flags: review?(dao)
Comment on attachment 8556241 [details] [diff] [review]
bug_1126253.patch

looks good!
Attachment #8556241 - Flags: review?(dao) → review+
https://hg.mozilla.org/integration/fx-team/rev/a8feff978f82
Assignee: nobody → curtis.koenig+bz
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/a8feff978f82
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.