Last Comment Bug 840287 - Work - Move NewTabUtils.jsm from /browser/modules to /toolkit/modules
: Work - Move NewTabUtils.jsm from /browser/modules to /toolkit/modules
Status: RESOLVED FIXED
[feature=work]
:
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
Depends on:
Blocks: 1148979 808770
  Show dependency treegraph
 
Reported: 2013-02-11 14:43 PST by Matt Brubeck (:mbrubeck)
Modified: 2015-03-30 07:58 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.79 KB, patch)
2013-02-11 21:15 PST, Matt Brubeck (:mbrubeck)
ttaubert: review+
Details | Diff | Splinter Review
patch v2 (5.74 KB, patch)
2013-02-15 11:36 PST, Matt Brubeck (:mbrubeck)
mbrubeck: review+
gavin.sharp: superreview+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2013-02-11 14:43:29 PST
For bug 808770, we would like to use NewTabUtils.jsm in Metro Firefox.  I think the simplest way to do this is to move it into /toolkit/content.  Any objections, comments, alternatives?

See also the discussion in similar bug 811548.
Comment 1 Matt Brubeck (:mbrubeck) 2013-02-11 21:15:02 PST
Created attachment 712798 [details] [diff] [review]
patch
Comment 2 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2013-02-12 03:45:04 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #0)
> Any objections

None from me - there's nothing explicitly specific to desktop Firefox in there.

Without having thought about it too deeply, I'd say its probably worth looking into whether it should init itself on first use, like what's being done in bug 811548.
Comment 3 Matt Brubeck (:mbrubeck) 2013-02-12 09:36:52 PST
(In reply to Blair McBride [:Unfocused] from comment #2)
> Without having thought about it too deeply, I'd say its probably worth
> looking into whether it should init itself on first use, like what's being
> done in bug 811548.

NewTabUtils.init adds some telemetry probes, and an expiration filter.  We probably want both to happen at startup; otherwise we risk missing some Telemetry and expiring useful thumbnails, for users who don't open the new tab page in time.

Should we keep the explicit init() as a reminder of this, or is it enough to just Cu.import() the module at startup and have it init itself?
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-12 09:51:39 PST
I think we should generally avoid implicit initialization (i.e. side effects to import()) for JS modules.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-14 13:17:39 PST
Can you move this to toolkit/modules instead? See bug 828116.
Comment 6 Matt Brubeck (:mbrubeck) 2013-02-15 11:36:38 PST
Created attachment 714522 [details] [diff] [review]
patch v2

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> Can you move this to toolkit/modules instead? See bug 828116.

Sure!  (And thanks, I was wondering how to decide which subdirectory was more appropriate.)
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-02-17 04:15:08 PST
https://hg.mozilla.org/mozilla-central/rev/0e3aca33a039

Note You need to log in before you can comment on or make changes to this bug.