Remove synchronous calls to nsICacheSession::openCacheEntry in SeaMonkey

RESOLVED FIXED in seamonkey2.17

Status

SeaMonkey
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Philip Chee, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
seamonkey2.17
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.14 fixed, seamonkey2.15 fixed, seamonkey2.16 fixed)

Details

Attachments

(6 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 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.
Depends on: 695399, 737615
(Reporter)

Comment 2

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

Comment 3

5 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.
(Assignee)

Comment 4

5 years ago
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.
Attachment #663154 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #663154 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 669270 [details] [diff] [review]
WIP

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

Comment 6

5 years ago
Created attachment 669276 [details] [diff] [review]
Pr
(Assignee)

Comment 7

5 years ago
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.
Assignee: nobody → neil
Attachment #669270 - Attachment is obsolete: true
Attachment #669276 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #669283 - Flags: review?(iann_bugzilla)

Comment 8

5 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.
Attachment #669283 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 9

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

Comment 10

5 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?
(Assignee)

Comment 11

5 years ago
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.
Attachment #671239 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 12

5 years ago
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.
Attachment #669283 - Attachment is obsolete: true
Attachment #671248 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #671248 - Flags: review?(iann_bugzilla) → review+
(Reporter)

Updated

5 years ago
Attachment #663154 - Attachment description: Possible patch → Possible patch for metadata.js
(Reporter)

Updated

5 years ago
Attachment #671248 - Attachment description: Fixed patch → Fixed patch (pageInfo)
(Reporter)

Updated

5 years ago
Attachment #671239 - Attachment description: Proposed patch → Proposed patch (tabbrowser)
(Reporter)

Comment 13

5 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
(Reporter)

Comment 14

5 years ago
>   var numFrames = 0;
I think we might have to get rid of this as well due to Bug 713889.
(Assignee)

Comment 15

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

Updated

5 years ago
Attachment #671239 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 16

5 years ago
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
Attachment #674386 - Flags: review+
Attachment #674386 - Flags: approval-comm-beta?
Attachment #674386 - Flags: approval-comm-aurora?

Updated

5 years ago
Attachment #674386 - Flags: approval-comm-beta?
Attachment #674386 - Flags: approval-comm-beta+
Attachment #674386 - Flags: approval-comm-aurora?
Attachment #674386 - Flags: approval-comm-aurora+
(Assignee)

Comment 17

5 years ago
By the way the patch needs a bit of fuzz to apply on beta because of harmless changes in context lines.
(Assignee)

Comment 18

5 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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-seamonkey2.14: --- → fixed
status-seamonkey2.15: --- → fixed
status-seamonkey2.16: --- → fixed
Resolution: --- → FIXED
(Reporter)

Comment 19

5 years ago
> Pushed comm-central changeset 2ca434d7cccd6.
This should be: comm-central changeset 2ca434d7ccd6
(Reporter)

Updated

5 years ago
Depends on: 804972
(Assignee)

Comment 20

5 years ago
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).
Attachment #675873 - Flags: review?(iann_bugzilla)
(Reporter)

Updated

5 years ago
Depends on: 806144

Comment 21

5 years ago
Comment on attachment 675873 [details] [diff] [review]
Followup fix

[Triage Comment]
As other patches landed on comm-aurora/beta a/r=me
Attachment #675873 - Flags: review?(iann_bugzilla)
Attachment #675873 - Flags: review+
Attachment #675873 - Flags: approval-comm-beta+
Attachment #675873 - Flags: approval-comm-aurora+
(Assignee)

Comment 22

5 years ago
Comment on attachment 675873 [details] [diff] [review]
Followup fix

Pushed comm-central changeset 2204125b352d.
(Assignee)

Comment 23

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

Comment 24

4 years ago
Created attachment 692024 [details] [diff] [review]
Third time lucky...
Attachment #692024 - Flags: review?
Attachment #692024 - Flags: approval-comm-beta?
Attachment #692024 - Flags: approval-comm-aurora?

Updated

4 years ago
Attachment #692024 - Flags: review?
Attachment #692024 - Flags: review+
Attachment #692024 - Flags: approval-comm-beta?
Attachment #692024 - Flags: approval-comm-beta+
Attachment #692024 - Flags: approval-comm-aurora?
Attachment #692024 - Flags: approval-comm-aurora+
(Assignee)

Comment 25

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

Comment 26

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

Pushed comm-central changeset edb9f86b87da.
(Assignee)

Comment 27

4 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

Updated

4 years ago
Target Milestone: --- → seamonkey2.17
You need to log in before you can comment on or make changes to this bug.