Philip Chee Reporter
	Description • 12 years ago

      No description provided.

	Philip Chee Reporter
	Comment 1 • 12 years ago

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.

	Philip Chee Reporter
	Comment 2 • 12 years ago

MXR Query:
http://mxr.mozilla.org/comm-central/search?string=.openCacheEntry(&find=%2Fsuite%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

	neil@parkwaycc.co.uk Assignee
	Comment 3 • 12 years ago

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.

	neil@parkwaycc.co.uk Assignee
	Comment 4 • 12 years ago

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.

	neil@parkwaycc.co.uk Assignee
	Comment 5 • 12 years ago

This is for page info but I'm not 100% sure about the image preview yet.

	neil@parkwaycc.co.uk Assignee
	Comment 7 • 12 years ago

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

	Ian Neal
	Comment 8 • 12 years ago

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.

	neil@parkwaycc.co.uk Assignee
	Comment 9 • 12 years ago

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

	Ian Neal
	Comment 10 • 12 years ago

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?

	neil@parkwaycc.co.uk Assignee
	Comment 11 • 12 years ago

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.

	neil@parkwaycc.co.uk Assignee
	Comment 12 • 12 years ago

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.

	Philip Chee Reporter
	Comment 13 • 12 years ago

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

	Philip Chee Reporter
	Comment 14 • 12 years ago

>   var numFrames = 0;
I think we might have to get rid of this as well due to Bug 713889.

	neil@parkwaycc.co.uk Assignee
	Comment 15 • 12 years ago

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

	neil@parkwaycc.co.uk Assignee
	Comment 16 • 12 years ago

[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

	neil@parkwaycc.co.uk Assignee
	Comment 17 • 12 years ago

By the way the patch needs a bit of fuzz to apply on beta because of harmless changes in context lines.

	neil@parkwaycc.co.uk Assignee
	Comment 18 • 12 years ago

Pushed comm-central changeset 2ca434d7cccd6.
https://hg.mozilla.org/releases/comm-aurora/rev/6c13bbdb2fff
https://hg.mozilla.org/releases/comm-beta/rev/ec317af35d42

	Philip Chee Reporter
	Comment 19 • 12 years ago

> Pushed comm-central changeset 2ca434d7cccd6.
This should be: comm-central changeset 2ca434d7ccd6

	neil@parkwaycc.co.uk Assignee
	Comment 20 • 12 years ago

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

	Ian Neal
	Comment 21 • 12 years ago

Comment on attachment 675873 [details] [diff] [review]
Followup fix

[Triage Comment]
As other patches landed on comm-aurora/beta a/r=me

	neil@parkwaycc.co.uk Assignee
	Comment 22 • 12 years ago

Comment on attachment 675873 [details] [diff] [review]
Followup fix

Pushed comm-central changeset 2204125b352d.

	neil@parkwaycc.co.uk Assignee
	Comment 23 • 12 years ago

Comment on attachment 675873 [details] [diff] [review]
Followup fix

http://hg.mozilla.org/releases/comm-aurora/rev/03544c781830
http://hg.mozilla.org/releases/comm-beta/rev/cb6d8a94c610

	neil@parkwaycc.co.uk Assignee
	Comment 25 • 12 years ago

(In reply to comment #22)
> Pushed comm-central changeset 2204125b352d.
Actually it was comm-central changeset 2dead7fe49dd.

	neil@parkwaycc.co.uk Assignee
	Comment 26 • 12 years ago

Comment on attachment 692024 [details] [diff] [review]
Third time lucky...

Pushed comm-central changeset edb9f86b87da.

	neil@parkwaycc.co.uk Assignee
	Comment 27 • 12 years ago

Comment on attachment 692024 [details] [diff] [review]
Third time lucky...

https://hg.mozilla.org/releases/comm-aurora/rev/90249d084a38
https://hg.mozilla.org/releases/comm-beta/rev/5579a7f2a3d5