Last Comment Bug 792735 - Remove synchronous calls to nsICacheSession::openCacheEntry in SeaMonkey
: Remove synchronous calls to nsICacheSession::openCacheEntry in SeaMonkey
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.17
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 695399 737615 804972 806144
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-20 02:58 PDT by Philip Chee
Modified: 2013-01-08 08:39 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
Possible patch for metadata.js (6.97 KB, patch)
2012-09-20 13:55 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
WIP (8.81 KB, patch)
2012-10-08 13:51 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Pr (8.99 KB, patch)
2012-10-08 14:04 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Proposed patch (8.99 KB, patch)
2012-10-08 14:11 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Proposed patch (tabbrowser) (3.57 KB, patch)
2012-10-14 11:48 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Fixed patch (pageInfo) (9.74 KB, patch)
2012-10-14 12:36 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Branch-safe version (13.63 KB, patch)
2012-10-23 14:32 PDT, neil@parkwaycc.co.uk
neil: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review
Followup fix (775 bytes, patch)
2012-10-27 12:11 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review
Third time lucky... (691 bytes, patch)
2012-12-13 14:31 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Philip Chee 2012-09-20 02:58:41 PDT

    
Comment 1 Philip Chee 2012-09-20 02:59:48 PDT
References:
Bug 695399 - Remove synchronous calls to nsICacheSession::openCacheEntry in pageInfo.js
Bug 695399 - Remove calls to nsICacheSession::openCacheEntry on the main thread
Bug 737615 - Remove use of synchronous cache API from unit tests.
Comment 3 neil@parkwaycc.co.uk 2012-09-20 03:02:14 PDT
I looked at metadata.js and I think it can be simplified because images never get stored in the FTP cache. I haven't looked at any of the other uses though.
Comment 4 neil@parkwaycc.co.uk 2012-09-20 13:55:22 PDT
Created attachment 663154 [details] [diff] [review]
Possible patch for metadata.js

For metadata.js only. I also added a placeholder string to metadata.xul/dtd but this is optional. I had also considered renaming imageSizeUnknown to imageSizeNotCached and then adding the placeholder in script instead.
Comment 5 neil@parkwaycc.co.uk 2012-10-08 13:51:51 PDT
Created attachment 669270 [details] [diff] [review]
WIP

This is for page info but I'm not 100% sure about the image preview yet.
Comment 6 neil@parkwaycc.co.uk 2012-10-08 14:04:19 PDT
Created attachment 669276 [details] [diff] [review]
Pr
Comment 7 neil@parkwaycc.co.uk 2012-10-08 14:11:12 PDT
Created attachment 669283 [details] [diff] [review]
Proposed patch

* Converts reading the document size to using asyncOpenCacheEntry. I check both caches although we very rarely use the FTP cache. Note that the page size will briefly show as "Not specified" until the cache entry is opened.
* Creates a cache listener class for reading the cache entries for the media on the page, although I call it imageCacheListener for hysterical raisins.
* Fetches the cache entry device ID while reading the image size so that it can be used later for the image preview.
Comment 8 Ian Neal 2012-10-14 08:30:24 PDT
Comment on attachment 669283 [details] [diff] [review]
Proposed patch

>+function addImage(url, type, alt, elem, isBg)
>+{
>+  if (url) try {
>+    var listener = new imageCacheListener(url, type, alt, elem, isBg);
>+    httpCacheSession.asyncOpenCacheEntry(url, ACCESS_READ, listener);
>+  }
>+  catch (ex) { }
>+}
Don't need to try ftp here?

> function makePreview(row)
> {
>+  var [url, type, sizeText, alt, count, item, isBG, pageSize, deviceID] = gImageView.data[row];
[Snip]
> 
>   // find out the file size
>   var sizeText;
>+  if (pageSize) {
>     var kbSize = Math.round(pageSize / 1024 * 100) / 100;
>     sizeText = gBundle.getFormattedString("generalSize",
>                                           [formatNumber(kbSize),
>                                            formatNumber(pageSize)]);
>   }
>   setItemValue("imagesizetext", sizeText);
Don't we already know what sizeText is from gImageView.data[row]?

r=me with those addressed/fixed.
Comment 9 neil@parkwaycc.co.uk 2012-10-14 10:01:09 PDT
(In reply to Ian Neal from comment #8)
> (From update of attachment 669283 [details] [diff] [review])
> >+function addImage(url, type, alt, elem, isBg)
> Don't need to try ftp here?
No, apparently we never cache FTP images. In fact as far as I can tell we only ever cache XUL FTP directory listings.

> >   setItemValue("imagesizetext", sizeText);
> Don't we already know what sizeText is from gImageView.data[row]?
That's formatted using the "mediaFileSize" string; we want the "generalSize" string here.
Comment 10 Ian Neal 2012-10-14 10:10:35 PDT
Comment on attachment 669283 [details] [diff] [review]
Proposed patch

> function makePreview(row)
> {
>+  var [url, type, sizeText, alt, count, item, isBG, pageSize, deviceID] = gImageView.data[row];
[Snip]

>   // find out the file size
>   var sizeText;
Presumably we don't need to re-declare sizeText?
Comment 11 neil@parkwaycc.co.uk 2012-10-14 11:48:44 PDT
Created attachment 671239 [details] [diff] [review]
Proposed patch (tabbrowser)

This fixes tabbrowser.xml by switching to the favicon service.

Someone else must already be calling QI on the favicon service but I added it just to be sure.
Comment 12 neil@parkwaycc.co.uk 2012-10-14 12:36:07 PDT
Created attachment 671248 [details] [diff] [review]
Fixed patch (pageInfo)

Attachment 669283 [details] [diff] didn't compute the image type correctly for background images. This version fixes that. I also updated the patch as per comment #10.
Comment 13 Philip Chee 2012-10-16 06:24:39 PDT
Comment on attachment 671248 [details] [diff] [review]
Fixed patch (pageInfo)

After applying this patch I tested it with the test case from
Bug 801930 - Exception in makePreview (pageinfo.js) (imgIRequest.image NS_ERROR_FAILURE)

And indeed I got the same error:
Tue Oct 16 2012 21:21:54
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [imgIRequest.image]
Source file: chrome://navigator/content/pageinfo/pageInfo.js
Line: 1246
Comment 14 Philip Chee 2012-10-16 06:37:38 PDT
>   var numFrames = 0;
I think we might have to get rid of this as well due to Bug 713889.
Comment 15 neil@parkwaycc.co.uk 2012-10-17 12:09:54 PDT
(In reply to Philip Chee from comment #14)
> >   var numFrames = 0;
> I think we might have to get rid of this as well due to Bug 713889.
This involves a string change so I would be better off doing it in a separate bug.
Comment 16 neil@parkwaycc.co.uk 2012-10-23 14:32:58 PDT
Created attachment 674386 [details] [diff] [review]
Branch-safe version

[Approval Request Comment]
Regression caused by (bug #): 695399
User impact if declined: Page Info claims no images are cached. Properties unable to display image sizes. Tabbrowser keeps trying to load missing favicons.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch: Removed for branch-safe version
Comment 17 neil@parkwaycc.co.uk 2012-10-23 14:48:24 PDT
By the way the patch needs a bit of fuzz to apply on beta because of harmless changes in context lines.
Comment 18 neil@parkwaycc.co.uk 2012-10-23 15:20:08 PDT
Pushed comm-central changeset 2ca434d7cccd6.
https://hg.mozilla.org/releases/comm-aurora/rev/6c13bbdb2fff
https://hg.mozilla.org/releases/comm-beta/rev/ec317af35d42
Comment 19 Philip Chee 2012-10-24 03:12:54 PDT
> Pushed comm-central changeset 2ca434d7cccd6.
This should be: comm-central changeset 2ca434d7ccd6
Comment 20 neil@parkwaycc.co.uk 2012-10-27 12:11:06 PDT
Created attachment 675873 [details] [diff] [review]
Followup fix

Oops, I thought I had tested this, but I noticed that I forgot that aURI was a string and I need to convert it (as per loadFavIcon).
Comment 21 Ian Neal 2012-10-28 10:34:49 PDT
Comment on attachment 675873 [details] [diff] [review]
Followup fix

[Triage Comment]
As other patches landed on comm-aurora/beta a/r=me
Comment 22 neil@parkwaycc.co.uk 2012-10-28 11:06:25 PDT
Comment on attachment 675873 [details] [diff] [review]
Followup fix

Pushed comm-central changeset 2204125b352d.
Comment 24 neil@parkwaycc.co.uk 2012-12-13 14:31:05 PST
Created attachment 692024 [details] [diff] [review]
Third time lucky...
Comment 25 neil@parkwaycc.co.uk 2012-12-14 13:48:27 PST
(In reply to comment #22)
> Pushed comm-central changeset 2204125b352d.
Actually it was comm-central changeset 2dead7fe49dd.
Comment 26 neil@parkwaycc.co.uk 2012-12-14 13:50:14 PST
Comment on attachment 692024 [details] [diff] [review]
Third time lucky...

Pushed comm-central changeset edb9f86b87da.

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