Allison Naaktgeboren :ally Reporter
	Description • 12 years ago

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?

	Allison Naaktgeboren :ally Reporter
	Comment 1 • 12 years ago

I understand that it is good practice to get an 'r' before bothering someone for an 'sr'.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 2 • 12 years ago

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).

	Allison Naaktgeboren :ally Reporter
	Comment 3 • 12 years ago

...oh dear. I'm in git >.>

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 4 • 12 years ago

Ah, heh. AFAIK, it should work the same - "git mv <src> <dest>".

	Allison Naaktgeboren :ally Reporter
	Comment 5 • 12 years ago

should preserve history as requested.

	Allison Naaktgeboren :ally Reporter
	Comment 6 • 12 years ago

As requested. It seems git mv does not handle renaming in hg's opinion.

	Dave Townsend [:mossop]
	Comment 7 • 12 years ago

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.

	Dave Townsend [:mossop]
	Comment 8 • 12 years ago

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.

	Allison Naaktgeboren :ally Reporter
	Comment 9 • 12 years ago

(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?

	Dave Townsend [:mossop]
	Comment 10 • 12 years ago

(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

	Dave Townsend [:mossop]
	Comment 12 • 12 years ago

Comment on attachment 688112 [details] [diff] [review]
now with hg mv,

Waiting on answers to comment 8

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 13 • 12 years ago

(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!

	Dave Townsend [:mossop]
	Comment 14 • 12 years ago

(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.

	David Teller [:Yoric] - still alive but not very active
	Comment 15 • 12 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 16 • 12 years ago

I think Mercurial handles re-basing across no-change renames just fine, FWIW.

	Mozilla RelEng Bot
	Comment 17 • 12 years ago

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

	Matt Brubeck (:mbrubeck) Assignee
	Comment 18 • 11 years ago

(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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 19 • 11 years ago

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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 20 • 11 years ago

(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 :)

	Matt Brubeck (:mbrubeck) Assignee
	Comment 21 • 11 years ago

(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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 22 • 11 years ago

(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.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 23 • 11 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 24 • 11 years ago

FWIW I don't think we should shy away from putting "browser-specific" code in toolkit/.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 25 • 11 years ago

Sorry for waffling -- I decided based on bug 840287 comment 3 and 4 that it's better to keep the initialization explicit here too.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 26 • 11 years ago

Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=1012164665a7

	Matt Brubeck (:mbrubeck) Assignee
	Comment 27 • 11 years ago

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

	Matt Brubeck (:mbrubeck) Assignee
	Comment 28 • 11 years ago

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

	Matt Brubeck (:mbrubeck) Assignee
	Comment 29 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/89c0a82b70e7

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/89c0a82b70e7