The fact we have our thumbnail cache in sessionstore contributes a good deal to the size of that file... it would be more appropriate and efficient to store them in the browser's image cache. The patches in bug 497543 may be of help; even if we don't use them literally, they may point to how to do it.

Sounds perf-y. Sean, how do feel about tackling this?

Targeting beta 8. Sean, does that sound alright?

Current status on this: Thumbnails have been a bit broken, but I'm not sure for how long. First order of business is to fix their behavior. I've changed the bug title to reflect this. 1. They were only being saved via TabItems.saveAll(true), and this was only happening on app quit, and only if you were in TabView, which never happens anymore because exiting the app forces you out of TabView before you can actually quit. Fixed that. 2. They would often get saved before the TabCanvas was painted to. Fixed that by using the last saved image string until the first paint() is called on the TabCanvas. Note that I'll change this over to using the image cache when I get to that part. 3. Because _update() needs to function properly, I've set a dependency on 609685. 4. I'm currently trying to figure out why sometimes, even after painting the correctly loaded webpage bits to the tabitem's canvas, canvas.toDataURL() returns a big white PNG. A possibly related problem is that even when the thumbnails load from storage and display, they're quickly replaced by a white thumbnail during the "Connecting" phase of the page load. I'd like to detect this state and only update when it's a real page.

Please have a look at the patch for bug 597248. There are changes to the thumbnail code.

Thanks, Raymond. I hadn't seen that bug -- I'll make this a dependency upon that.

See also bug 615704 on a related topic.

Why are we saving thumbnails in sessionstore to begin with? That doesn't sound like a useful thing to do at all, so I'm quite surprised to hear that we're doing it...

(In reply to comment #7) > Why are we saving thumbnails in sessionstore to begin with? That doesn't sound > like a useful thing to do at all, so I'm quite surprised to hear that we're > doing it... When we realized Panorama needed to cache thumbnails between sessions (which will become even more important once bug 595601 lands), sessionstore was the path of least resistance, since that's where all of the rest of the Panorama data is stored.

bug spam (moving to b10)

bug spam

Kyle Huey suggests in bug 615704 that we might use canvas.mozGetAsFile to turn the contents of a canvas into a File and then persist that. I don't know if that's better or worse than looking into the existing image cache, but I thought I'd mention it here.

Note that if we do this we'll need to take responsibility for security concerns associated with stored thumbnails. Say you go to your bank site and it's got your account number and balance, and we make a high-res thumbnail of it and store it to disk. At the very least, we need to make sure we remove the thumbnail as soon as the tab is closed, for instance. Also see bug 627239 and bug 627237.

Based on comment 12, tempted to punt this to the future.

Addressing those is not too much work I think. The session restore file is read at startup, so fixing this bug will likely improve startup time as well. Though it wouldn't show up in Talos, since we don't measure profiles like this.

bugspam. Moving b10 to b11

bugspam. Removing b10

Too late for this for Fx4

what? you can't store these images in sessionstore. that sounds ...insane.

Blocking on this for investigation. We must know whether profiles that actually have data are adversely affected by this or not.

why do we cache them at all?

I'd also like to mention, if this is not clear enough here, that this can potentially undo all of the hard work that has been put into making the user perceivable startup performance improvements made in Firefox 4. We do not need to, and should not, cache these images.

(In reply to comment #20) > why do we cache them at all? On a slow machine and/or connection and a number of tabs, it can take many minutes for all of the tabs to load up enough to render thumbnails for them. Once we land bug 595601, tabs in other groups won't even load until you go to them. In both cases, having cached thumbnails allows you to see your pages immediately in Panorama. So yes, there are a lot of good reasons to not store them in sessionstore, and there are also a lot of good reasons to store them somewhere (i.e. the image cache).

Are these thumbnails used when restoring a session?

(In reply to comment #23) > Are these thumbnails used when restoring a session? They are not... they're actually pretty low-res.

So we're getting started on this, but no one on the Panorama team knows anything about hooking into the browser's existing image cache. Does anyone on this bug have a recommendation for who to talk to?

the patch on bug 497543 uses the disk cache service. there might be a better way though, so it'd be worth asking in #developers, or digging through mxr to see how we do it for web images.

Can you first stop using session restore for this and then explore using the image cache as a second step?

Sent to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=7aefe32ff2ce

Comment on attachment 509797 [details] [diff] [review] v1 Looks good to me, but I'm not familiar with the cache routines. Could you get feedback from someone familiar with that domain? My comments: * Clearly this is a patch in progress, but once it's time to land, please clean out the dump and log commands. * Doesn't look like you need to pass a tab into saveTabThumb and loadTabThumb, so please omit that argument * Any idea what the perf implications are for this change?

Updated

Updated again. To answer your question about perf, loading from the cache feels faster (as it should be), but I haven't measured this.

(In reply to comment #32) > Created attachment 509829 [details] [diff] [review] > v3 > > Updated again. To answer your question about perf, loading from the cache feels > faster (as it should be), but I haven't measured this. Cool. Do you know who to ask for feedback on the patch? Joe? Someone else who gave you info about the cache system?

Comment on attachment 509829 [details] [diff] [review] v3 Shawn, would you mind feedbacking here? :)

Bjarne and Michal, can you guys have a look at the patch here and share thoughts on this approach?

Comment on attachment 509829 [details] [diff] [review] v3 >+ this._inputStreamFactory = Cc["@mozilla.org/scriptableinputstream;1"]; In general, you want to use Components.constructor for this, not what you are doing (however, you don't need this). >+ saveThumbnail: function Storage_saveThumbnail(url, imageData) { >+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url, >+ Ci.nsICache.ACCESS_WRITE, true); >+ let outputStream = entry.openOutputStream(0); >+ outputStream.write(imageData, imageData.length); >+ outputStream.close(); >+ entry.close(); Noooooo! This is synchronous I/O on the GUI thread (my most hated enemy)! What you really want to do here is use NetUtil.asyncCopy. Your sink will be the output stream, and your source will have to be an input string that you create. >+ loadThumbnail: function Storage_loadThumbnail(url) { >+ Utils.log("loading tab thumb"); >+ let imageChunks = []; >+ let entry = null; >+ try { >+ entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url, >+ Ci.nsICache.ACCESS_READ, true); >+ } catch (e) { >+ Utils.log(e); >+ } >+ if(entry) { >+ let nativeInputStream = entry.openInputStream(0); >+ let inputStream = this._inputStreamFactory. >+ createInstance(Ci.nsIScriptableInputStream); >+ inputStream.init(nativeInputStream); >+ try { >+ while (true) { >+ // read a megabyte at a time if needed >+ let chunk = inputStream.read(1024*1024); >+ if (chunk.length == 0) >+ break; >+ imageChunks.push(chunk); >+ } >+ } catch (e) { >+ Utils.log(e); >+ } >+ entry.close(); >+ } >+ let imageData = imageChunks.join(""); >+ return imageData; >+ }, There's no good way to do this async, but I'm going to go file the bug to add a method to NetUtil to make your life easier. We should be able to take that for 2.0 too! You'll want to get feedback from zpao too on the session store bits.

There is some ongoing work in initializing the cache-service which may be relevant here. See bug #630420, comment #17 in particular. Relevance depends on what time in the startup-sequence this stuff is supposed to happen.

(In reply to comment #36) > There's no good way to do this async, but I'm going to go file the bug to add a > method to NetUtil to make your life easier. We should be able to take that for > 2.0 too! Filed bug 632227, and it has a patch up you can use too. The callers to loadThumbnail are going to need to be changed to handle the fact that it'll be async, but getting the data is just going to be: NetUtil.asyncFetch(entry.openInputStream(0), callbackfunction);

(In reply to comment #38) > Filed bug 632227, and it has a patch up you can use too. The callers to > loadThumbnail are going to need to be changed to handle the fact that it'll be > async, but getting the data is just going to be: > NetUtil.asyncFetch(entry.openInputStream(0), callbackfunction); I've chosen to use asyncOpenCacheEntry() on the session instead, since the above would have just opened it synchronously and done the copy async (which isn't as big of a win, since this isn't a huge amount of data). Also adding mitcho for feedback, since the load behavior is now async, and I want a second set of eyes to make sure I've covered my bases.

Er, I mean ian. Added Ian for feedback :)

Comment on attachment 510770 [details] [diff] [review] v4 >+++ b/browser/base/content/tabview/storage.js > init: function Storage_init() { > this._sessionStore = > Cc["@mozilla.org/browser/sessionstore;1"]. > getService(Ci.nsISessionStore); >+ >+ // Create stream-based cache session for tabview >+ let cacheService = >+ Cc["@mozilla.org/network/cache-service;1"]. >+ getService(Ci.nsICacheService); >+ this._cacheSession = cacheService.createSession( >+ this.CACHE_CLIENT_IDENTIFIER, Ci.nsICache.STORE_ON_DISK, true); > }, > > // ---------- > // Function: uninit > uninit: function Storage_uninit () { > this._sessionStore = null; I strongly suspect you want to null out the new things you do in init here as well >+ saveThumbnail: function Storage_saveThumbnail(url, imageData) { >+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url, >+ Ci.nsICache.ACCESS_WRITE, true); I had completely forgotten about asyncOpenCacheEntry (and partially how the disk cache works all together). After a brief refresher on the code, I see that this also does synchronous disk I/O. Fun times! I think what you'll want to do is make a generic cache listener that will give you a callback when the entry is open. At that point, you'll want to do the work in each method, so something like this: let listener = new CacheListener(function(result, entry) { if (Components.isSuccessCode(result)) { // current saveThumbnail code goes here } }); this._cacheSession.asyncOpenCacheEntry(...) >+ let StringInputStream = Components.Constructor( >+ "@mozilla.org/io/string-input-stream;1", "nsIStringInputStream"); note: using Components.constructor is really only useful if you move it up (either as a property on the Storage object or globally in the file). Otherwise, it's less expensive to just do Cc["..."].createInstance(...) >+ loadThumbnail: function Storage_loadThumbnail(url, callback) { >+ let entry = null; >+ try { >+ function CacheListener() { >+ } >+ CacheListener.prototype = { >+ QueryInterface : function(iid) { >+ if (iid.equals(Ci.nsICacheListener)) >+ return this; >+ throw Components.results.NS_NOINTERFACE; >+ }, never implement QI yourself, this is what XPCOMUtils.generateQI is for! >+ onCacheEntryAvailable: function(entry, access, status) { >+ let imageChunks = []; >+ if(entry) { >+ let nativeInputStream = entry.openInputStream(0); >+ let ScriptableInputStream = Components.Constructor( >+ "@mozilla.org/scriptableinputstream;1", >+ "nsIScriptableInputStream", "init"); >+ let inputStream = new ScriptableInputStream(nativeInputStream); >+ try { >+ while (true) { >+ // read up to a megabyte at a time >+ let chunk = inputStream.read(1024*1024); >+ if (chunk.length == 0) >+ break; >+ imageChunks.push(chunk); >+ } >+ } catch (e) { >+ Utils.log(e); >+ } >+ entry.close(); >+ } When you get this callback, it actually just means that the cache entry is opened, but it didn't do any reading from the disk for you. You'll still want to read from the disk asynchronously (I know you filed bug 632238, but this cannot land if it's adding synchronous disk I/O).

All above issues should be addressed now.

> + saveThumbnail: function Storage_saveThumbnail(url, imageData) { > + let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url, > + Ci.nsICache.ACCESS_WRITE, true); You should use asyncOpenCacheEntry() when saving thumbnail too.

Renominating for hardblocking. This bug increases I/O, memory usage and can really hurt startup (sessionstore file can become huge). See also comment 21.

I'm blocking on this defensively - there are no numbers at all in this bug. Assertions like the ones comment #44 make might be correct...to a degree that's surprising in one way or the other. We can't know without some profiling. How much bigger is sessionstore.js with and without these images? And how does that effect startup time? (Mine is 5mb fwiw) How does this change effect Panorama thumbnail display performance? (they fill in pretty slowly for me now fwiw) We can't just throw code at the tree and see what happens. Please do some measurements, even if it's just dump()ing times, to see what the effect of these changes are. For startup, you can use about:startup now. I'd be glad to provide my megasessionstore.js if you'd like it for testing. Also, does the patch clean out old thumbnail data from pre-existing sessions?

Here are some numbers, using Win7 x64. Test of about:startup times: (main, firstPain, sessionRestored) Half of the samples were taken with TabView visible on startup, the other half taken with the browser visible. Without patch: Tabview visible: 118,869,1723 132,888,1755 31,784,1627 Browser visible: 39,801,1663 106,816,1670 659,1361,2199 With patch: Tabview visible: 342,1101,1944 81,845,1698 36,788,1628 Browser visible: 116,843,1681 470,1211,2048 107,826,1657 Test of sessionstore.js sizes, 100 random pages from metafilter.com. Stacked to largest size before showing the group: Without patch: 2,763kB With patch: 1,783kB Group maximized to window (2 sample sets): Without patch: 3,026kB, 3,246kB With patch: 2,138kB, 2,363kB Analysis: Startup times, given noise in samples, does not appear to be affected by this patch. Image data takes up ~10kB on average per tab. For 400 tabs (our current upper bar), this would increase the session store by ~4MB.

about:startup's values for each datapoint are ms since the event occurred, so something's not right with the numbers above - they're in various different orders. Also, does the patch clean old thumbnail data from the session restore data?

Filesize results look great, down ~1mb for that sample.

Yes, the patch cleans out old thumbnail data, which is why the file sizes were down :) Can you explain a bit better why the numbers don't look correct to you? Those numbers are from the process start to the first event of each. Each data point is from a separate process. What order should they be in?

Re: cleaning - wasn't clear whether you tested w/ new profile, or just ran the same profile without-and-then-with the patch to get those numbers. Thanks for clarifying. Re: about:startup > Test of about:startup times: (main, firstPain, sessionRestored) > Without patch: > Tabview visible: > 118,869,1723 > 132,888,1755 > 31,784,1627 The above indicates that session restored occurred before main, which isn't possible :)

Let's look at the first data line: main 118, firstPaint 869, sessionRestored 1723 This is saying that main was hit 118ms after process start. firstPaint was 869ms after process start. sessionRestored was 1723ms after process start. So, this all makes sense to me, in that sessionRestored happened 1605ms after main. Am I interpreting this wrong?

Sean, ha I was reading it as 3 lines of really huge times. Never mind, makes sense now!

Fixed all test failures. I had to change the async load code to load what it can from the sessionstore, and defer the thumbnail as a separate update. The reason for this was that going into private browsing mode and back does not play nicely if the tabs are not loaded in a synchronous manner. The thumbnails are still fine as async loaders, though. Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d28fbe4e5447

(In reply to comment #53) > Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=d28fbe4e5447 You should really use the try-chooser syntax (http://people.mozilla.org/~lsblakk/trychooser/trychooser.html) and get performance data on Ts and post the results in the bug (http://perf.snarkfest.net/compare-talos/index.html?tests=ts,ts_shutdown). Also, getting tp4 and ts numbers from your local machine (https://wiki.mozilla.org/StandaloneTalos) with a profile with panorama data might be a good idea too.

Sent to try, with Talos enabled: http://tbpl.mozilla.org/?tree=MozillaTry&rev=8ec3dcb3eeff

Perf comparison results. Not finished yet, but this link should remain valid: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b57524d6706f&newRev=8ec3dcb3eeff&tests=ts,ts_shutdown&submit=true

sdwilsh, it looks like the delay% is up a few percent, down for others, so I'm assuming that this is within reason. What else do you need for an r+ so we can move forward to approval?

(In reply to comment #57) > sdwilsh, it looks like the delay% is up a few percent, down for others, so I'm > assuming that this is within reason. What else do you need for an r+ so we can > move forward to approval? I need to sit down and actually review it, which will probably happen Monday (I have been out of town Wed - Fri).

Apart Ts, Tp4_pbytes and Tp4_rss seem to show a slight improvement too. Unfortunately all of Talos tests don't have a sized sessionstore, so the results are mostly near noise

Given the numbers that we've got now, I propose this be dropped from hardblocker.

(In reply to comment #59) > Apart Ts, Tp4_pbytes and Tp4_rss seem to show a slight improvement too. > Unfortunately all of Talos tests don't have a sized sessionstore, so the > results are mostly near noise We can run with an arbitrary profile with standalone talos.

(In reply to comment #56) With more baseline data points: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b57524d6706f,da205ca44184,409d5db222eb&newRev=8ec3dcb3eeff&tests=ts,ts_shutdown&submit=true

(In reply to comment #53) > Fixed all test failures. I had to change the async load code to load what it > can from the sessionstore, and defer the thumbnail as a separate update. The > reason for this was that going into private browsing mode and back does not > play nicely if the tabs are not loaded in a synchronous manner. The thumbnails > are still fine as async loaders, though. This should be fine. AFAIK, session store keeps it's data in memory.

Comment on attachment 511699 [details] [diff] [review] v6 >+++ b/browser/base/content/tabview/storage.js >+ // Function: saveThumbnail >+ // Save passed in image data for given url to cache >+ saveThumbnail: function Storage_saveThumbnail(url, imageData) { >+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url, >+ Ci.nsICache.ACCESS_WRITE, true); >+ let entryOutStream = entry.openOutputStream(0); >+ let inputStream = new this.StringInputStream(imageData, imageData.length); >+ NetUtil.asyncCopy(inputStream, entryOutStream, function(){ >+ entry.close(); >+ }); >+ }, You seem to have missed my comments in comment 41 about this method. > saveTab: function Storage_saveTab(tab, data) { > Utils.assert(tab, "tab"); > >+ if (data != null) { >+ let imageData = data.imageData; >+ // Remove imageData from payload >+ delete data.imageData; >+ if(imageData != null) { >+ this.saveThumbnail(data.url, imageData); >+ // Notify subscribers >+ tab._tabViewTabItem._sendToSubscribers("savedCachedImageData"); Do you really want to notify this before we have written the cached image data to disk? saveThumbnail is async now, so it hasn't actually happened when we dispatch this. >+ // Load tab data from session store and return it, but pass >+ // imageDataCB on to the thumbnail loader to run asynchronously. >+ getTabData: function Storage_getTabData(tab, imageDataCb) { What is imageDataCb? The variable doesn't tell me what it is, and the comments don't help either. Please be more descriptive and/or add more comments. >+// ########## >+// Class: CacheListener >+// Generic CacheListener for feeding to asynchronous cache calls. >+function CacheListener(callback) { >+ this.callback = callback; Describe what the callback gets here please. In general, the comments for methods need to be more descriptive than what has been done in the past. I shouldn't have to read a bunch of code to figure out this information if we provide comments like this. >+ onCacheEntryAvailable: function(entry, access, status) { >+ if (this.callback) { When would callback be null? >+ this.callback(entry, access, status); In the callback you've written above, you do not check that status is succesful, nor do you check that the access you were granted was what you asked for. You need to do both of these things (and you can then stop checking entry). >+++ b/browser/base/content/tabview/tabview.js > Cu.import("resource:///modules/tabview/AllTabs.jsm"); > Cu.import("resource:///modules/tabview/utils.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); >+Cu.import("resource://gre/modules/NetUtil.jsm"); Does this load at startup? If so, you should turn this into a lazy getter. Also, it should be noted that I'm not a browser peer, so while my review is useful, it's not enough to land this.

(In reply to comment #64) > Comment on attachment 511699 [details] [diff] [review] > Also, it should be noted that I'm not a browser peer, so while my review is > useful, it's not enough to land this. Shawn, thank you for all your help on this bug! Who do you think should be doing the final review?

Taking until Sean is back.

(In reply to comment #66) > Taking until Sean is back. If you have other blockers to work on, I can steal this too.

(In reply to comment #67) > If you have other blockers to work on, I can steal this too. I have no other blockers at the moment, sorry :) But I have one question right away: I modified the patch like you said in comment #41 so that saveThumbnail() works async now. With this I get a lot of errors in the console: JavaScript Error: "attempt to run compile-and-go script on a cleared scope" I think this is only happens when our DOMWindowClosed observer is notified and calls TabItems.saveAll(). So the window is recycled and the storage code tries to call already GCed listeners. So we should save thumbnails synchronously when the DOM window is closed, right?

Copying brendan and gal for more looking into the "attempt to run compile-and-go script on a cleared scope" error above.

(In reply to comment #68) > I think this is only happens when our DOMWindowClosed observer is notified and > calls TabItems.saveAll(). So the window is recycled and the storage code tries > to call already GCed listeners. So we should save thumbnails synchronously when > the DOM window is closed, right? To be clear, are you talking about when calling saveTab? saveThumbnail doesn't notify any observers. Don't the observers release themselves when their windows goes away? If they do, I am a bit confused as to how we'd be notifying them if the window went away.

function domWinClosedObserver(subject, topic, data) { if (topic == "domwindowclosed" && subject == gWindow) { if (self.isTabViewVisible()) GroupItems.removeHiddenGroups(); TabItems.saveAll(true); self._save(); } } This is where we save all tab thumbnails when the parent chrome window gets closed. Now that tab thumbnails are stored asynchronously we call cache.asyncOpenCacheEntry() and pass a cache listener. After the window got GC'ed the cache entry is available and the cache listener is notified. Because the scope has already been cleared we get the error mentioned above. The execution order must be something like: 1.) parent chrome windows is about to be closed ("domwindowclosed") 2.) TabItems.saveAll() -> cache.asyncOpenCacheEntry() 3.) parent chrome window is closed and GC'ed 4.) the cache entry is now available and the service tries to notify the cache listener 5.) JavaScript Error: "attempt to run compile-and-go script on a cleared scope" (This is what I assume happens.)

(In reply to comment #71) That (unfortunately) makes sense. I suppose the right fix here is to move this saving code to a JSM so that it lives longer, but I'm not sure we want to do that for Firefox 4.0. The tradeoff is doing synchronous disk I/O every time we save a tab thumbnail. Do we save thumbnails even if the user has not opened panorama?

The exception is new but what you are trying to do there didn't work before either. After a window is closed you can't run code on it. Thats a bug we will try to fix for 5.

(In reply to comment #73) > The exception is new but what you are trying to do there didn't work before > either. After a window is closed you can't run code on it. Thats a bug we will > try to fix for 5. I'm ok with that limitation :) (In reply to comment #72) > (In reply to comment #71) > That (unfortunately) makes sense. I suppose the right fix here is to move this > saving code to a JSM so that it lives longer, but I'm not sure we want to do > that for Firefox 4.0. The tradeoff is doing synchronous disk I/O every time we > save a tab thumbnail. What about switching to synchronous thumbnail saving _after_ having received "domwindowclosed"? I think that could work and keep the window alive until we finished.

(In reply to comment #73) > The exception is new but what you are trying to do there didn't work before > either. After a window is closed you can't run code on it. Thats a bug we will > try to fix for 5. So comment 71 step 3 in Tim's diagnosis should say "parent window is closed", not "... and GC'ed". The parent is not garbage as it is referenced from live objects. This mandatory "scope clearing" is ancient and it compensates for leaks, which (Andreas is noting here) we'll fix after fx4. /be

(In reply to comment #64) > >+ // Save passed in image data for given url to cache > >+ saveThumbnail: function Storage_saveThumbnail(url, imageData) { > >+ }, Works now asnychronously. After receiving "domwindowclosed" this method switches to synchronous mode. > Do you really want to notify this before we have written the cached image data > to disk? saveThumbnail is async now, so it hasn't actually happened when we > dispatch this. Fixed. > >+ // Load tab data from session store and return it, but pass > >+ // imageDataCB on to the thumbnail loader to run asynchronously. > >+ getTabData: function Storage_getTabData(tab, imageDataCb) { > What is imageDataCb? The variable doesn't tell me what it is, and the comments > don't help either. Please be more descriptive and/or add more comments. Added and corrected comments (on other places, too). > >+ onCacheEntryAvailable: function(entry, access, status) { > >+ if (this.callback) { > When would callback be null? Removed. > >+ this.callback(entry, access, status); > In the callback you've written above, you do not check that status is > succesful, nor do you check that the access you were granted was what you asked > for. You need to do both of these things (and you can then stop checking > entry). Now handled in Storage._openCacheEntry(). > >+Cu.import("resource://gre/modules/NetUtil.jsm"); > Does this load at startup? If so, you should turn this into a lazy getter. Fixed. Additionally: adapted tests and added a new one ensuring the new thumbnail storage works as expected.

Comment on attachment 513215 [details] [diff] [review] patch v7 >+++ b/browser/base/content/tabview/storage.js >+ _openCacheEntry: function Storage__openCacheEntry(url, access, successCallback, errorCallback) { >+ let onCacheEntryAvailable = function (entry, accessGranted, status) { >+ if (entry && access == accessGranted && status == 0) status is an nsresult. Use Components.isSuccessCode instead. >+ try { >+ this._cacheSession.asyncOpenCacheEntry(this.CACHE_PREFIX + url, >+ access, new CacheListener(onCacheEntryAvailable)); >+ } catch (e) { >+ errorCallback(); >+ } When would this throw? >+ saveThumbnail: function Storage_saveThumbnail(url, imageData, callback) { >+ // save thumbnail using synchronous IO if the window is about to close >+ if (UI.isDOMWindowClosing) { >+ let entry = this._cacheSession.openCacheEntry(this.CACHE_PREFIX + url, >+ Ci.nsICache.ACCESS_WRITE, true); >+ if (!entry) { >+ callback(false); >+ return; >+ } It seems like you really want to move the opening too _openCacheEntry and have it call the sync or async method depending on the dom window closing too. >+ gNetUtil.asyncCopy(inputStream, entryOutStream, function () { >+ entry.close(); >+ callback(true); The asyncCopy could have failed. In the callback, it gives you an nsresult indicating success or failure that you should be checking and passing true or false here accordingly. >+ // 65k chunks with a max of PR_UINT32_MAX >+ let storageStream = new self.StorageStream(0x10000, 0xFFFFFFFF, null); Please pull both of these magic numbers out into constants. >+ gNetUtil.asyncCopy(nativeInputStream, storageOutStream, function (result) { >+ // check if the parent windows is closed or about to close >+ if (typeof UI == "object" && !UI.isDOMWindowClosing) { >+ let storageInStream = storageStream.newInputStream(0); >+ let str = gNetUtil.readInputStreamToString(storageInStream, >+ storageInStream.available()); >+ callback(true, str); You also don't check result here with Components.isSuccessCode > saveTab: function Storage_saveTab(tab, data) { > Utils.assert(tab, "tab"); > >+ if (data != null) { >+ let imageData = data.imageData; >+ // Remove imageData from payload >+ delete data.imageData; >+ if (imageData != null) { >+ this.saveThumbnail(data.url, imageData, function (status) { >+ // Notify subscribers >+ tab._tabViewTabItem._sendToSubscribers("savedCachedImageData"); >+ }); >+ } >+ } >+ > this._sessionStore.setTabValue(tab, this.TAB_DATA_IDENTIFIER, > JSON.stringify(data)); In what instances would data be null? Why do we still want to call into sessionStore when it is null? >+ if (existingData) >+ this.loadThumbnail(existingData.url, function (status, imageData) { >+ callback(imageData); >+ // Notify subscribers >+ tab._tabViewTabItem._sendToSubscribers("loadedCachedImageData"); You don't actually check status here, although it could easily be false. >+++ b/browser/base/content/tabview/tabitems.js >+ let currentUrl = self.tab.linkedBrowser.currentURI.spec; >+ // If we have a cached image, then show it if the loaded URL matches >+ // what the cache is from, OR the loaded URL is blank, which means >+ // that the page hasn't loaded yet. >+ if (tabData.imageData && (tabData.url == currentUrl || >+ currentUrl == 'about:blank')) { I am going to suggest formatting it like this so it's easier to read: >+ if (tabData.imageData && >+ (tabData.url == currentUrl || currentUrl == 'about:blank')) { Can we test the case where it's about:blank too? >+++ b/browser/base/content/tabview/tabview.js >+XPCOMUtils.defineLazyGetter(this, "gNetUtil", function() { >+ Cu.import("resource://gre/modules/NetUtil.jsm"); >+ return NetUtil; >+}); I believe you actually want to import that into a local object, and return the NetUtil property off of that: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/NetworkHelper.jsm#60 I didn't look at the tests yet.

(In reply to comment #77) > status is an nsresult. Use Components.isSuccessCode instead. Fixed. > >+ try { > >+ this._cacheSession.asyncOpenCacheEntry(this.CACHE_PREFIX + url, > >+ access, new CacheListener(onCacheEntryAvailable)); > >+ } catch (e) { > >+ errorCallback(); > >+ } > When would this throw? Never, removed. > It seems like you really want to move the opening too _openCacheEntry and have > it call the sync or async method depending on the dom window closing too. Of course, fixed. > The asyncCopy could have failed. In the callback, it gives you an nsresult > indicating success or failure that you should be checking and passing true or > false here accordingly. Fixed. > >+ // 65k chunks with a max of PR_UINT32_MAX > >+ let storageStream = new self.StorageStream(0x10000, 0xFFFFFFFF, null); > Please pull both of these magic numbers out into constants. Fixed. > > saveTab: function Storage_saveTab(tab, data) { > > Utils.assert(tab, "tab"); > > > >+ if (data != null) { > >+ let imageData = data.imageData; > >+ // Remove imageData from payload > >+ delete data.imageData; > >+ if (imageData != null) { > >+ this.saveThumbnail(data.url, imageData, function (status) { > >+ // Notify subscribers > >+ tab._tabViewTabItem._sendToSubscribers("savedCachedImageData"); > >+ }); > >+ } > >+ } > >+ > > this._sessionStore.setTabValue(tab, this.TAB_DATA_IDENTIFIER, > > JSON.stringify(data)); > In what instances would data be null? Why do we still want to call into > sessionStore when it is null? We call saveTab(tab, null) when wiping session store data (e.g. the tab gets closed). > >+ if (existingData) > >+ this.loadThumbnail(existingData.url, function (status, imageData) { > >+ callback(imageData); > >+ // Notify subscribers > >+ tab._tabViewTabItem._sendToSubscribers("loadedCachedImageData"); > You don't actually check status here, although it could easily be false. Fixed. > >+ let currentUrl = self.tab.linkedBrowser.currentURI.spec; > >+ // If we have a cached image, then show it if the loaded URL matches > >+ // what the cache is from, OR the loaded URL is blank, which means > >+ // that the page hasn't loaded yet. > >+ if (tabData.imageData && (tabData.url == currentUrl || > >+ currentUrl == 'about:blank')) { > I am going to suggest formatting it like this so it's easier to read: Fixed. > >+ if (tabData.imageData && > >+ (tabData.url == currentUrl || currentUrl == 'about:blank')) { > > Can we test the case where it's about:blank too? browser_tabview_bug597248.js tests now whether the thumbnail for about:blank is restored, too. > >+XPCOMUtils.defineLazyGetter(this, "gNetUtil", function() { > >+ Cu.import("resource://gre/modules/NetUtil.jsm"); > >+ return NetUtil; > >+}); > I believe you actually want to import that into a local object, and return the > NetUtil property off of that: Fixed.

Comment on attachment 513559 [details] [diff] [review] patch v8 >+++ b/browser/base/content/tabview/storage.js >+ let stringInputStream = this.StringInputStream; Capitalize 'string' please so it looks like other JS objects >+ let chunkSize = 0x10000; // 65k I never did ask how we came up with this number... >+ let chunkSize = 0x10000; // 65k >+ let maxBytes = 0xFFFFFFFF; // PR_UINT32_MAX I actually meant const for both of these, and using PR_UINT32_MAX for the name of the second one is preferred. >+++ b/browser/base/content/test/tabview/browser_tabview_bug597248.js >-let gTabsProgressListener = { >+/*let gTabsProgressListener = { > onStateChange: function(browser, webProgress, request, stateFlags, status) { > // ensure about:blank doesn't trigger the code > if ((stateFlags & Ci.nsIWebProgressListener.STATE_STOP) && > (stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) && > browser.currentURI.spec != "about:blank") { > if (newTabOne.linkedBrowser == browser) > restoredNewTabOneLoaded = true; > else if (newTabTwo.linkedBrowser == browser) >@@ -226,18 +175,9 @@ function updateAndCheck() { > ok(!tabItem.isShowingCachedData(), > "Tab item is not showing cached data anymore. " + > tabItem.tab.linkedBrowser.currentURI.spec); > }); > > // clean up and finish > restoredWin.close(); > finish(); >-} >- >-function load(tab, url, callback) { >- tab.linkedBrowser.addEventListener("load", function (event) { >- tab.linkedBrowser.removeEventListener("load", arguments.callee, true); >- callback(); >- }, true); >- tab.linkedBrowser.loadURI(url); >-} >- >+}*/ Hmm, what's this about? r=sdwilsh with comments/questions addressed/answered.

(In reply to comment #79) > >+ let chunkSize = 0x10000; // 65k > I never did ask how we came up with this number... I don't know either :/ Is there a reasonable chunk size to use when reading cache data? > >+++ b/browser/base/content/test/tabview/browser_tabview_bug597248.js > >-let gTabsProgressListener = { > >+/*let gTabsProgressListener = { > > onStateChange: function(browser, webProgress, request, stateFlags, status) { > > // ensure about:blank doesn't trigger the code > > if ((stateFlags & Ci.nsIWebProgressListener.STATE_STOP) && > > (stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) && > > browser.currentURI.spec != "about:blank") { > > if (newTabOne.linkedBrowser == browser) > > restoredNewTabOneLoaded = true; > > else if (newTabTwo.linkedBrowser == browser) > >@@ -226,18 +175,9 @@ function updateAndCheck() { > > ok(!tabItem.isShowingCachedData(), > > "Tab item is not showing cached data anymore. " + > > tabItem.tab.linkedBrowser.currentURI.spec); > > }); > > > > // clean up and finish > > restoredWin.close(); > > finish(); > >-} > >- > >-function load(tab, url, callback) { > >- tab.linkedBrowser.addEventListener("load", function (event) { > >- tab.linkedBrowser.removeEventListener("load", arguments.callee, true); > >- callback(); > >- }, true); > >- tab.linkedBrowser.loadURI(url); > >-} > >- > >+}*/ > Hmm, what's this about? These parts of the test were disabled some time ago (because of intermittent oranges) by commenting out the calls to these functions. I just commented out the whole part to make sure nothing of this is still called in the test - and to gain some more overview :)

Comment on attachment 513559 [details] [diff] [review] patch v8 Passed try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=d1251450e3a7

Per comment 64 this still needs review from a browser peer, no?

Yeah and the attached patch is also not up-to-date because I'm waiting for response.

(In reply to comment #82) > Per comment 64 this still needs review from a browser peer, no? Actually, we're good now. Ian is the sub module owner of panorama, and he r+ the patch, so it's good to go.

(In reply to comment #79) > >+++ b/browser/base/content/tabview/storage.js > >+ let stringInputStream = this.StringInputStream; > Capitalize 'string' please so it looks like other JS objects Fixed. > >+ let chunkSize = 0x10000; // 65k > I never did ask how we came up with this number... I didn't change the number for now. If you have a reasonable value for this please tell me :) > >+ let chunkSize = 0x10000; // 65k > >+ let maxBytes = 0xFFFFFFFF; // PR_UINT32_MAX > I actually meant const for both of these, and using PR_UINT32_MAX for the name > of the second one is preferred. Fixed.

If this is ready for checkin, please say so explicitly and I'll see if I have time to push it today.

(In reply to comment #84) > Actually, we're good now. Ian is the sub module owner of panorama, and he r+ > the patch, so it's good to go. Ok, cool :) (In reply to comment #86) > If this is ready for checkin, please say so explicitly and I'll see if I have > time to push it today. Thanks in advance!

(In reply to comment #85) > I didn't change the number for now. If you have a reasonable value for this > please tell me :) I don't. This is probably fine.