The default bug view has changed. See this FAQ.

Work - Moving browser/components/thumbnails/ into toolkit

RESOLVED FIXED in mozilla21

Status

()

Toolkit
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ally, Assigned: mbrubeck)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [feature=work])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

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

Updated

4 years ago
Whiteboard: [metro-mvp?] → [metro-mvp?][metro-it1][LOE:?]
(Reporter)

Updated

4 years ago
Whiteboard: [metro-mvp?][metro-it1][LOE:?] → [metro-mvp?][metro-it1][LOE:2]
(Reporter)

Updated

4 years ago
Assignee: nobody → ally
(Reporter)

Comment 1

4 years ago
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'.
Attachment #684094 - Flags: review?(bmcbride)
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).
Attachment #684094 - Flags: review?(bmcbride) → review-
(Reporter)

Comment 3

4 years ago
...oh dear. I'm in git >.>
Ah, heh. AFAIK, it should work the same - "git mv <src> <dest>".
(Reporter)

Comment 5

4 years ago
Created attachment 685772 [details] [diff] [review]
part 1/1 now with git mv instead of git add/remove

should preserve history as requested.
Attachment #684094 - Attachment is obsolete: true
Attachment #685772 - Flags: review?(bmcbride)
(Reporter)

Updated

4 years ago
Attachment #685772 - Flags: review?(bmcbride)
(Reporter)

Comment 6

4 years ago
Created attachment 688112 [details] [diff] [review]
now with hg mv,

As requested. It seems git mv does not handle renaming in hg's opinion.
Attachment #685772 - Attachment is obsolete: true
Attachment #688112 - Flags: superreview?(dtownsend+bugmail)
Attachment #688112 - Flags: review?(dtownsend+bugmail)

Updated

4 years ago
Whiteboard: [metro-mvp?][metro-it1][LOE:2] → [metro-mvp][metro-it1][LOE:2]
Attachment #688112 - Attachment is patch: true
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.
Attachment #688112 - Flags: review?(dtownsend+bugmail) → review-
(Reporter)

Updated

4 years ago
Whiteboard: [metro-mvp][metro-it1][LOE:2] → [metro-mvp][metro-it1][LOE:2][metro-it2]
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.
(Reporter)

Comment 9

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

Comment 11

4 years ago
Created attachment 689477 [details] [diff] [review]
move w/adjusted toolkit-makefile.sh, based off of 6/dec/2012 tree
(Reporter)

Updated

4 years ago
Assignee: ally → nobody
Comment on attachment 688112 [details] [diff] [review]
now with hg mv,

Waiting on answers to comment 8
Attachment #688112 - Flags: superreview?(dtownsend+bugmail)
(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!
(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.
(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.
I think Mercurial handles re-basing across no-change renames just fine, FWIW.

Comment 17

4 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

Updated

4 years ago
Summary: Moving browser/components/thumbnails/ into toolkit → Work - Moving browser/components/thumbnails/ into toolkit
Whiteboard: [metro-mvp][metro-it1][LOE:2][metro-it2] → feature=work
(Assignee)

Updated

4 years ago
Assignee: nobody → mbrubeck
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 18

4 years ago
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.
Attachment #688112 - Attachment is obsolete: true
Attachment #689477 - Attachment is obsolete: true
Attachment #711695 - Flags: review?(dtownsend+bugmail)
(Assignee)

Updated

4 years ago
Whiteboard: feature=work → [feature=work][has patch]
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.
(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 :)
(Assignee)

Comment 21

4 years ago
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.
Attachment #711695 - Attachment is obsolete: true
Attachment #711695 - Flags: review?(dtownsend+bugmail)
Attachment #711858 - Flags: superreview?(dtownsend+bugmail)
Attachment #711858 - Flags: review?(ttaubert)
(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.
(Assignee)

Comment 23

4 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.
FWIW I don't think we should shy away from putting "browser-specific" code in toolkit/.
(Assignee)

Comment 25

4 years ago
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.
Attachment #711858 - Attachment is obsolete: true
Attachment #711858 - Flags: superreview?(dtownsend+bugmail)
Attachment #711858 - Flags: review?(ttaubert)
Attachment #712997 - Flags: superreview?(dtownsend+bugmail)
Attachment #712997 - Flags: review?(ttaubert)
Attachment #712997 - Flags: review?(ttaubert) → review+
Attachment #712997 - Flags: superreview?(dtownsend+bugmail) → superreview+
(Assignee)

Comment 26

4 years ago
Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=1012164665a7
(Assignee)

Comment 27

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

Comment 28

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

Comment 29

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c0a82b70e7
Status: NEW → ASSIGNED
Whiteboard: [feature=work][has patch] → [feature=work]
https://hg.mozilla.org/mozilla-central/rev/89c0a82b70e7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 844538
You need to log in before you can comment on or make changes to this bug.