Closed Bug 676906 Opened 8 years ago Closed 8 years ago

Add an async getFaviconDataForPage to mozIAsyncFavicons

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 4 obsolete files)

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.
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.
> 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.
yep, I meant to keep the .idl changes in a separate patch for easy sr.
Attachment #551128 - Flags: review?(mak77)
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.
Attachment #551128 - Flags: review?(mak77) → feedback-
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 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)
Attachment #551129 - Flags: review?(mak77)
(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.
Fixed interface method name.
Attachment #551128 - Attachment is obsolete: true
Attachment #551198 - Flags: review?(mak77)
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.
Attachment #551129 - Attachment is obsolete: true
Attachment #551201 - Flags: review?(mak77)
Sorry I meant to say in Comment 11 that the third patch will be for the xpcshell tst.
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)
Attachment #551198 - Flags: review?(mak77) → review+
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)
Attachment #551201 - Flags: review?(mak77) → review+
Flags: in-testsuite?
Summary: Add an async getFaviconForPage to mozIAsyncFavicons → Add an async getFaviconDataForPage to mozIAsyncFavicons
Attachment #551198 - Flags: superreview?(gavin.sharp)
Fixed double whitespace and pointer next to type
Attachment #551364 - Flags: review+
Attachment #551201 - Attachment is obsolete: true
Attachment #551198 - Flags: superreview?(gavin.sharp) → superreview+
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.
Attachment #551420 - Flags: review?(mak77) → review-
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.
Implemented review comments.  I.e. removed extra flags that were used in case the asyncs were processed by multiple threads.
Attachment #551420 - Attachment is obsolete: true
Attachment #553587 - Flags: review?(mak77)
Attachment #553587 - Flags: review?(mak77) → review+
why this did not land yet?
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.
http://hg.mozilla.org/mozilla-central/rev/93c4018253b1
http://hg.mozilla.org/mozilla-central/rev/0b8b1ba3c97d
http://hg.mozilla.org/mozilla-central/rev/79f89328862d
Status: NEW → RESOLVED
Closed: 8 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
Blocks: 540765
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.