676906 - Add an async getFaviconDataForPage to mozIAsyncFavicons

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Brian R. Bondy [:bbondy]]

Currently mozIAsyncFavicons allows you to obtain a favicon URL async but not obtain a favicon's data async. 

We need the ability to obtain a favicon's data from the sqlite database if available async.

Comment 1

[Marco Bonardo [:mak]]

Yep, this would be useful and much appreciated. Please keep separate patch containing the API from patch containing implementation, it's easier for sr.

Just for information, notice that if you create a channel to a moz-anno:favicon:faviconurl, you'll get the data async (in our views for example we set image sources to that url). But in your case may not be that much useful.

Comment 2

[Brian R. Bondy [:bbondy]]

> keep separate patch containing the API from patch containing implementation

I think it is all API.  

I have a patch almost ready for this, the changes are in: 
AsyncFaviconHelpers.cpp, AsyncFaviconHelpers.h, mozIAsyncFavicons.idl, nsFaviconService.cpp

Is one patch ok? If not pls let me know which files to put in which patch.

Comment 3

[Marco Bonardo [:mak]]

yep, I meant to keep the .idl changes in a separate patch for easy sr.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch for interface of the added async functionality

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch for implementation of the added async functionality

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 551128 [details] [diff] [review]
Patch for interface of the added async functionality

Review of attachment 551128 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +118,5 @@
> +   *
> +   * @see nsIFaviconDataCallback in nsIFaviconService.idl.
> +   */
> +  void getFaviconForPage(in nsIURI aPageURI,
> +                         in nsIFaviconDataCallback aCallback);

So, the signature seems fine, but we have a getFaviconForPage in nsIFaviconService, and since both interfaces are implemented by the same object, that also implements classinfo, we are hosed.

We need another name, maybe just getFavicon or getFaviconDataForPage... probably the latter is more self-documenting. Unless you have alternative suggestions.

Comment 7

[Brian R. Bondy [:bbondy]]

I was aware of the other one, I thought we could overload it though since they have different signatures.  

I'll change the name to your second suggestion but please advise for my future reference if the same name is not possible.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 551129 [details] [diff] [review]
Patch for implementation of the added async functionality

Review of attachment 551129 [details] [diff] [review]:
-----------------------------------------------------------------

This badly needs a test for the review, xpcshell may be fine, there are other tests in toolkit/components/places/tests/unit/ involving icon APIs to take inspiration.

Also, while the global approach is correct, you can most likely reuse most code. First of all, you don't need FetchIconData, since you can reuse FetchIconInfo, that also gets the data (FetchIconURL had been added to avoid getting data when was useless). you may call FetchIconURL, then FetchIconInfo (this may be optimized in future to run a single query, in a not blocking follow-up bug though), and finally just dispatch NotifyIconObservers

::: toolkit/components/places/nsFaviconService.cpp
@@ +718,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsFaviconService::GetFaviconForPage(nsIURI *aPageURI,
> +                                    nsIFaviconDataCallback* aCallback)

we put pointers near type usually (Annoying but looks like each module has its own convention)

Comment 9

[Marco Bonardo [:mak]]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> I was aware of the other one, I thought we could overload it though since
> they have different signatures.  

unfortunately the overload concept doesn't work well in js world.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch for interface of the added async functionality

Fixed interface method name.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Patch for implementation of the added async functionality

I'll submit a 3rd patch before asking for check-in for review separately.

This patch implements the rest of your review comments.

I could get more code re-use by deriving AsyncGetFaviconDataForPage from AsyncGetFaviconURLForPage (or making a more common base type) but if we will eventually optimize to do one DB call only it seems not worth it.  Let me know if you want me to though.

Comment 12

[Brian R. Bondy [:bbondy]]

Sorry I meant to say in Comment 11 that the third patch will be for the xpcshell tst.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 551198 [details] [diff] [review]
Patch for interface of the added async functionality

Review of attachment 551198 [details] [diff] [review]:
-----------------------------------------------------------------

this needs an SR (either rstrong or gavin may do it)

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 551201 [details] [diff] [review]
Patch for implementation of the added async functionality

Review of attachment 551201 [details] [diff] [review]:
-----------------------------------------------------------------

it looks good, there is no need to go deeper in optimizations for now, that can be done at a later time if needed.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +992,5 @@
> +
> +  PageData pageData;
> +  pageData.spec.Assign(mPageSpec);
> +
> +  rv = FetchIconInfo(mFaviconSvc->mSyncStatements,  iconData);

double whitespace after comma

::: toolkit/components/places/nsFaviconService.cpp
@@ +718,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsFaviconService::GetFaviconDataForPage(nsIURI *aPageURI,
> +                                        nsIFaviconDataCallback* aCallback)

the first pointer decorator is still in the wrong positition here (should be near type)

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Patch for implementation of the added async functionality

Fixed double whitespace and pointer next to type

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch for xpcshell test for the added async functionality

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 551420 [details] [diff] [review]
Patch for xpcshell test for the added async functionality

Review of attachment 551420 [details] [diff] [review]:
-----------------------------------------------------------------

ugh, this test sucks, should be completely refactored, btw not your fault clearly.

::: toolkit/components/places/tests/unit/test_favicons.js
@@ +357,5 @@
> +      do_check_eq(icon1MimeType, out1MimeType.value);
> +      checkArrays(icon1Data, aData);
> +      do_check_eq(aDataLen, aData.length);
> +      finishedGetFaviconDataForPage = true;
> +      if (finishedGetFaviconURLForPage && finishedGetFaviconDataForPage) {

you shouldn't need all of these checks, all Storage queries are serialized, so it's guaranteed the getFaviconURLForPage request will be handled before the getFaviconDataForPage ones. you can just move do_test_finished to the latter and avoid all those state vars.

Comment 18

[Marco Bonardo [:mak]]

To clarify, I meant all Storage queries _on the same thread and connection_ are serialized, in this case both APIs use the same connection and are on the background thread.

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch for xpcshell test for the added async functionality v2

Implemented review comments.  I.e. removed extra flags that were used in case the asyncs were processed by multiple threads.

Comment 20

[Marco Bonardo [:mak]]

why this did not land yet?

Comment 21

[Brian R. Bondy [:bbondy]]

I actually just ran it through try last night and was asking if the results were ok on #developers.  I was literally 2 minutes away from pushing to m-i before you wrote that :) ( http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=670f4813f2da )

I have a few dozen patches backed up trying to get caught up on pushing them.
Will be pushed momentarily.

Comment 22

[Brian R. Bondy [:bbondy]]

Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/79f89328862d
http://hg.mozilla.org/integration/mozilla-inbound/rev/0b8b1ba3c97d
http://hg.mozilla.org/integration/mozilla-inbound/rev/93c4018253b1

Comment 23

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/93c4018253b1
http://hg.mozilla.org/mozilla-central/rev/0b8b1ba3c97d
http://hg.mozilla.org/mozilla-central/rev/79f89328862d