Last Comment Bug 811548 - Work - Moving browser/components/thumbnails/ into toolkit
: Work - Moving browser/components/thumbnails/ into toolkit
Status: RESOLVED FIXED
[feature=work]
:
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: Matt Brubeck (:mbrubeck)
:
:
Mentors:
Depends on: 844538
Blocks: 794028
  Show dependency treegraph
 
Reported: 2012-11-13 16:26 PST by Allison Naaktgeboren :ally
Modified: 2013-02-23 17:40 PST (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1/1 (94.38 KB, patch)
2012-11-21 10:33 PST, Allison Naaktgeboren :ally
blair: review-
Details | Diff | Splinter Review
part 1/1 now with git mv instead of git add/remove (94.42 KB, patch)
2012-11-27 12:21 PST, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review
now with hg mv, (13.59 KB, patch)
2012-12-03 22:03 PST, Allison Naaktgeboren :ally
dtownsend: review-
Details | Diff | Splinter Review
move w/adjusted toolkit-makefile.sh, based off of 6/dec/2012 tree (14.34 KB, patch)
2012-12-06 17:01 PST, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review
patch (18.75 KB, patch)
2013-02-07 21:52 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v2 (19.13 KB, patch)
2013-02-08 08:46 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
patch v3 (16.67 KB, patch)
2013-02-12 10:44 PST, Matt Brubeck (:mbrubeck)
ttaubert: review+
dtownsend: superreview+
Details | Diff | Splinter Review

Description Allison Naaktgeboren :ally 2012-11-13 16:26:28 PST
After discussing with Mossop & mbrubeck, it seems that the best way to share the thumbnail code between desktop firefox & metro firefox is to move it to toolkit. Metro firefox is a big priority for the firefox team, so this will need to happen (much) sooner rather than later. 

Mossop has suggested that the required superreview be assigned to him.

Volunteers?
Comment 1 Allison Naaktgeboren :ally 2012-11-21 10:33:08 PST
Created attachment 684094 [details] [diff] [review]
part 1/1

I understand that it is good practice to get an 'r' before bothering someone for an 'sr'.
Comment 2 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-11-22 00:36:05 PST
Comment on attachment 684094 [details] [diff] [review]
part 1/1

Review of attachment 684094 [details] [diff] [review]:
-----------------------------------------------------------------

If you use "hg mv" to move the browser/components/thumbnails directory, it'll keep all the file history - which also makes it *much* easier to see what actual changes happened to those files (it's rather difficult to do as-is).
Comment 3 Allison Naaktgeboren :ally 2012-11-25 22:29:24 PST
...oh dear. I'm in git >.>
Comment 4 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-11-26 02:20:17 PST
Ah, heh. AFAIK, it should work the same - "git mv <src> <dest>".
Comment 5 Allison Naaktgeboren :ally 2012-11-27 12:21:31 PST
Created attachment 685772 [details] [diff] [review]
part 1/1 now with git mv instead of git add/remove

should preserve history as requested.
Comment 6 Allison Naaktgeboren :ally 2012-12-03 22:03:40 PST
Created attachment 688112 [details] [diff] [review]
now with hg mv,

As requested. It seems git mv does not handle renaming in hg's opinion.
Comment 7 Dave Townsend [:mossop] 2012-12-06 09:57:51 PST
Comment on attachment 688112 [details] [diff] [review]
now with hg mv,

Review of attachment 688112 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't compile and even when I fix the toolkit-makefiles.sh syntax error it doesn't pass tests. Also update it to latest trunk as it doesn't apply right now. I'll continue the super-review separately.

::: toolkit/toolkit-makefiles.sh
@@ +552,5 @@
>    other-licenses/snappy/Makefile
>  "
>  
> +MAKEFILES_thumbnails="
> +  toolkit/thumbnails/Makefile

This doesn't need a separate section, just put it in MAKEFULES_xulapp. Please also add the tests Makefile later.
Comment 8 Dave Townsend [:mossop] 2012-12-06 13:01:03 PST
Comment on attachment 688112 [details] [diff] [review]
now with hg mv,

Review of attachment 688112 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty clean from an SR standpoint, I'd like to get some brief answers to these questions first though:

Who is responsible for initializing and uninitializing the PageThumbs service?

What happens if methods are called when PageThumbs isn't initialized?

Is PageThumbsStorage intended to be used by anyone other than PageThumbsProtocol?

Please file a bug on switching to async OS.file usage instead of a custom worker.

Please add documentation for addExpirationFilter and removeExpirationFilter.
Comment 9 Allison Naaktgeboren :ally 2012-12-06 16:51:04 PST
(In reply to Dave Townsend (:Mossop) from comment #7)
> Comment on attachment 688112 [details] [diff] [review]
> now with hg mv,
> 
> Review of attachment 688112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch doesn't compile and even when I fix the toolkit-makefiles.sh
> syntax error it doesn't pass tests. Also update it to latest trunk as it
> doesn't apply right now. I'll continue the super-review separately.

?! It is entirely possible that I forgot one last hg qref, but mine compiles & runs cleanly with vs2012 on win8 using pymake. I've updated & reapplied to this morning's trunk no problem. Could their be a build config issue?
Comment 10 Dave Townsend [:mossop] 2012-12-06 16:53:32 PST
(In reply to :Ally Naaktgeboren from comment #9)
> (In reply to Dave Townsend (:Mossop) from comment #7)
> > Comment on attachment 688112 [details] [diff] [review]
> > now with hg mv,
> > 
> > Review of attachment 688112 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch doesn't compile and even when I fix the toolkit-makefiles.sh
> > syntax error it doesn't pass tests. Also update it to latest trunk as it
> > doesn't apply right now. I'll continue the super-review separately.
> 
> ?! It is entirely possible that I forgot one last hg qref, but mine compiles
> & runs cleanly with vs2012 on win8 using pymake. I've updated & reapplied to
> this morning's trunk no problem. Could their be a build config issue?

The patch here is missing a quotation mark in toolkit-makefiles.sh. After correcting that you can see the test failures here: https://tbpl.mozilla.org/?tree=Try&rev=6e3ec38a2def
Comment 11 Allison Naaktgeboren :ally 2012-12-06 17:01:44 PST
Created attachment 689477 [details] [diff] [review]
move w/adjusted toolkit-makefile.sh, based off of 6/dec/2012 tree
Comment 12 Dave Townsend [:mossop] 2012-12-11 12:52:19 PST
Comment on attachment 688112 [details] [diff] [review]
now with hg mv,

Waiting on answers to comment 8
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-12 13:06:18 PST
(In reply to Dave Townsend (:Mossop) from comment #8)
> Who is responsible for initializing and uninitializing the PageThumbs
> service?

This happens in Firefox in nsBrowserGlue, so I guess it's up to the consumer. 

> What happens if methods are called when PageThumbs isn't initialized?

It looks like most work, but history clearing and expiration won't be active. Seems like it would probably be better to fix PageThumbs to initialize itself on first use, rather than requiring explicit initialization?

> Is PageThumbsStorage intended to be used by anyone other than
> PageThumbsProtocol?

Aside from tests, I think not. It would probably be nice to encapsulate that better somehow (or change its name so that that intent is clear).

> Please file a bug on switching to async OS.file usage instead of a custom
> worker.

Bug 753768 is changing this around some - I think it depends on other bugs like bug 801598 and bug 815339. David Teller is already on this!
Comment 14 Dave Townsend [:mossop] 2012-12-12 14:32:44 PST
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13)
> (In reply to Dave Townsend (:Mossop) from comment #8)
> > Who is responsible for initializing and uninitializing the PageThumbs
> > service?
> 
> This happens in Firefox in nsBrowserGlue, so I guess it's up to the
> consumer. 
> 
> > What happens if methods are called when PageThumbs isn't initialized?
> 
> It looks like most work, but history clearing and expiration won't be
> active. Seems like it would probably be better to fix PageThumbs to
> initialize itself on first use, rather than requiring explicit
> initialization?

I agree, presumably as easy as putting PageThumbs.Init() at the bottom of the jsm.

> > Is PageThumbsStorage intended to be used by anyone other than
> > PageThumbsProtocol?
> 
> Aside from tests, I think not. It would probably be nice to encapsulate that
> better somehow (or change its name so that that intent is clear).

We could always export as _PageThumbsStorage I guess. Not something we've done in the past. Maybe just PageThumbs._storage would be better.

> > Please file a bug on switching to async OS.file usage instead of a custom
> > worker.
> 
> Bug 753768 is changing this around some - I think it depends on other bugs
> like bug 801598 and bug 815339. David Teller is already on this!

Awesome. Doesn't need to hold up this bug.
Comment 15 David Teller [:Yoric] (please use "needinfo") 2012-12-12 17:25:44 PST
(In reply to Dave Townsend (:Mossop) from comment #14)
> > Bug 753768 is changing this around some - I think it depends on other bugs
> > like bug 801598 and bug 815339. David Teller is already on this!
> 
> Awesome. Doesn't need to hold up this bug.

Well, if I could avoid having too many conflicts while my patches are waiting for review, this would be great.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-12 19:09:30 PST
I think Mercurial handles re-basing across no-change renames just fine, FWIW.
Comment 17 Mozilla RelEng Bot 2012-12-21 15:22:02 PST
Try run for 6e3ec38a2def is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6e3ec38a2def
Results (out of 129 total builds):
    success: 97
    warnings: 30
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dtownsend@mozilla.com-6e3ec38a2def
Comment 18 Matt Brubeck (:mbrubeck) 2013-02-07 21:52:29 PST
Created attachment 711695 [details] [diff] [review]
patch

(In reply to Dave Townsend (:Mossop) from comment #8)
> Who is responsible for initializing and uninitializing the PageThumbs
> service?

PageThumbs.js now inits and uninits itself, as suggested above.

> Is PageThumbsStorage intended to be used by anyone other than
> PageThumbsProtocol?

As mentioned above, it's also used by the thumbnail tests.  Since this is basically a private interface, should I rename the exposed symbol to _PageThumbsStorage or something to indicate that?  I'm open to other possibilities too if anyone has suggestions.

> Please add documentation for addExpirationFilter and removeExpirationFilter.

Done.
Comment 19 Tim Taubert [:ttaubert] 2013-02-08 07:59:24 PST
Comment on attachment 711695 [details] [diff] [review]
patch

Review of attachment 711695 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +35,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "webappsUI",
>                                    "resource:///modules/webappsUI.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "PageThumbs",
> +                                  "resource://gre/modules/PageThumbs.jsm");

I guess you can remove the whole line now that we don't use it here anymore.
Comment 20 Tim Taubert [:ttaubert] 2013-02-08 08:02:15 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #18)
> (In reply to Dave Townsend (:Mossop) from comment #8)
> > Who is responsible for initializing and uninitializing the PageThumbs
> > service?
> 
> PageThumbs.js now inits and uninits itself, as suggested above.

Doesn't look like it inits itself?


BTW, we put PageThumbs.init() in nsBrowserGlue because we needed it to migrate the thumbnails before they are used (after the storage got updated or whatever). That's okay as we'll now just do it when the JSM loads which means before anyone should want to display or capture a thumbnail.

The second thing initiated at startup is the history observer. If PageThumbs initialized itself we will not purge thumbnails if the JSM hasn't been loaded since Firefox started and the user chooses to clear their history. I think the best solution here would be to move this out of the PageThumbs.jsm as it's clearly Firefox specific. browser-thumbnails.js could add its own listener and just call PageThumbs.removeThumbnail() or PageThumbs.wipeStorage() if needed.

The last thing being initialized is the expiration part. While it's very unlikely that PageThumbs isn't initialized at all for a very long time after Firefox started it's theoretically possible and we end up not expiring thumbnails at all. I wouldn't really worry about this though because we load the JSM as soon as we open a new tab page. For people that don't use the new tab page there should be no reason to really expire thumbnails. If they OTOH use something (an add-on maybe) that uses thumbnails the JSM will be initialized and we should be good here (sorry, just thinking out loud).

> As mentioned above, it's also used by the thumbnail tests.  Since this is
> basically a private interface, should I rename the exposed symbol to
> _PageThumbsStorage or something to indicate that?  I'm open to other
> possibilities too if anyone has suggestions.

PageThumbsStorage is used by the PageThumbsProtocol, that's why it's exported. I'm not really happy about this, though. Maybe we can make this a PageThumbs method instead of exposing the whole storage.

> > Please add documentation for addExpirationFilter and removeExpirationFilter.
> 
> Done.

Thanks for that :)
Comment 21 Matt Brubeck (:mbrubeck) 2013-02-08 08:46:52 PST
Created attachment 711858 [details] [diff] [review]
patch v2

(In reply to Tim Taubert [:ttaubert] from comment #19)
> I guess you can remove the whole line now that we don't use it here anymore.

Done.  Thanks for the comments!

(In reply to Tim Taubert [:ttaubert] from comment #20)
> Doesn't look like it inits itself?

Oops!  I don't know how I lost that line of the patch.  Here it is.

> BTW, we put PageThumbs.init() in nsBrowserGlue because we needed it to
> migrate the thumbnails before they are used (after the storage got updated
> or whatever). That's okay as we'll now just do it when the JSM loads which
> means before anyone should want to display or capture a thumbnail.

Indeed; in fact in Firefox it will still happen during startup, because PageThumbs.jsm is loaded by gBrowserThumbnails.init(), which is called from gBrowserInit._delayedStartup.

> The second thing initiated at startup is the history observer. If PageThumbs
> initialized itself we will not purge thumbnails if the JSM hasn't been
> loaded since Firefox started and the user chooses to clear their history.

This sees like a potential reason to require an explicit init during startup, rather than doing it when the JSM is loaded.  I'm not sure which trade-off we should make here...

> I think the best solution here would be to move this out of the PageThumbs.jsm
> as it's clearly Firefox specific. browser-thumbnails.js could add its own
> listener and just call PageThumbs.removeThumbnail() or
> PageThumbs.wipeStorage() if needed.

Really?  It seems like the history observer should be useful in Metrofox too, as well as other code that may use this module in the future.  I don't see why it is Firefox-specific.
Comment 22 Tim Taubert [:ttaubert] 2013-02-08 09:02:58 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #21)
> > I think the best solution here would be to move this out of the PageThumbs.jsm
> > as it's clearly Firefox specific. browser-thumbnails.js could add its own
> > listener and just call PageThumbs.removeThumbnail() or
> > PageThumbs.wipeStorage() if needed.
> 
> Really?  It seems like the history observer should be useful in Metrofox
> too, as well as other code that may use this module in the future.  I don't
> see why it is Firefox-specific.

Hmmm.. true. Maybe I'm thinking of toolkit too much as generic XULRunner code instead of "our different versions of Firefox" :) So... we clearly should have that code for all browsers instead of re-implementing it. This would then require all browsers to initialize the PageThumbs module rather early and explicitly.

I don't have another idea. If we don't want the module to load before something actually happens we would need to observer the history service in every browser. If not, looks like we need to load the whole module. Unless we split that into two modules but I don't know if that's really justified here.
Comment 23 Matt Brubeck (:mbrubeck) 2013-02-08 09:09:16 PST
(In reply to Tim Taubert [:ttaubert] from comment #22)
> > Really?  It seems like the history observer should be useful in Metrofox
> > too, as well as other code that may use this module in the future.  I don't
> > see why it is Firefox-specific.
> 
> Hmmm.. true. Maybe I'm thinking of toolkit too much as generic XULRunner
> code instead of "our different versions of Firefox" :)

I've considered creating a "/browser/common" (or something) that would be built for Fennec, Firefox, and Metrofox but not to other toolkit apps.  I'm not sure of the exact mechanics of this, but I've come across a few uses cases like this one.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-08 12:14:01 PST
FWIW I don't think we should shy away from putting "browser-specific" code in toolkit/.
Comment 25 Matt Brubeck (:mbrubeck) 2013-02-12 10:44:33 PST
Created attachment 712997 [details] [diff] [review]
patch v3

Sorry for waffling -- I decided based on bug 840287 comment 3 and 4 that it's better to keep the initialization explicit here too.
Comment 26 Matt Brubeck (:mbrubeck) 2013-02-15 14:03:57 PST
Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=1012164665a7
Comment 27 Matt Brubeck (:mbrubeck) 2013-02-15 15:26:39 PST
Try tells me I missed a change in package-manifest.in and didn't fully rebase after bug 239254.  Try again: https://tbpl.mozilla.org/?tree=Try&rev=f95cb66ab63d
Comment 28 Matt Brubeck (:mbrubeck) 2013-02-16 07:33:30 PST
Argh, missed some more paths.  Which is weird, because I thought I had all these tests green locally.  Maybe I didn't do a full clobber build.  Anyway, take 3:
https://tbpl.mozilla.org/?tree=Try&rev=ed95d0484f30
Comment 30 Ryan VanderMeulen [:RyanVM] 2013-02-17 04:15:21 PST
https://hg.mozilla.org/mozilla-central/rev/89c0a82b70e7

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