Closed
Bug 540765
Opened 15 years ago
Closed 13 years ago
[META]Favicon Service should be async
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
People
(Reporter: mak, Unassigned)
References
Details
Attachments
(8 obsolete files)
Favicon Service is sync, but based on most common operations with favicons (set a favicon, set and load a favicon, get a favicon) and the fact DB operations could be slower due to favicons being blobs, that's slow.
It is always bad for IO on the main thread, large numbers of favicons set/get can make that really bad.
This bug aims to convert most of favicons service API to be async.
Current plan is to have an optional callback to each method, called when the operation is successful, that can avoid to the implementer (And to us) the cost of registering an history observer.
Target for the change is 1.9.3 (likely FX4.0)
Reporter | ||
Comment 1•15 years ago
|
||
Looking at the API in its completeness, i this this kind of callback function could be good for all APIs (Apart ExpireAllFavicons and failedIconCache methods that can stay sync).
The plan is to add an optional input callback param to all APIs, that will be called on success. This will avoid implementers to have to register an expensive history observer.
The drawback about this is that we will just hide error conditions, since everything is async, the alternative would be to have a full callback object with an handleResult, but seriously it does not look like favicons errors have major impact on functionality, i see them more like a fire-and-forget.
All the APIs follow a common convention involving more between these steps:
* check current data for the passed in values
* fetch new icon (from network or db)
* assign icon to page
* notify observers
* callback
Not all of these points for all the APIs clearly, but they are always in this order, so could make sense to have internal helpers executing each step (where they can't be unified)
Reporter | ||
Comment 2•15 years ago
|
||
i meant handleError, not handleResult.
Comment 3•15 years ago
|
||
Comment on attachment 422631 [details] [diff] [review]
Part1: new callback interface and better idl docs
> /**
>- * Retrieves the given favicon data. Throws if we don't have data.
>+ * Retrieves the given favicon data. Throws if we don't have data.
> *
> * If there is no data but we have an entry for this favicon, aDataLen will
> * be 0 and aData will be NULL.
we should maybe deprecate this? It's sync...
looks good otherwise
Reporter | ||
Comment 4•15 years ago
|
||
well yes, (In reply to comment #3)
> > * If there is no data but we have an entry for this favicon, aDataLen will
> > * be 0 and aData will be NULL.
> we should maybe deprecate this? It's sync...
well yes, once this moves to async comments will need further tweaking, for now i've just made them shorter and clearer since i wanted to check they were not saying stupid things and i was aware of possible misses in the conversion.
Reporter | ||
Comment 5•15 years ago
|
||
So, the current plan is to implement something like a stepper, make simple tasks as steps, and let the stepper walk through them. What i have so far is something like:
nsCOMPtr<AsyncFaviconStepper> stepper = new AsyncFaviconStepper(aCallback);
// Set the data we have.
stepper->SetFaviconURI(aURI);
stepper->SetPageURI(aURI);
// Create steps, they auto register with the stepper.
nsCOMPtr<ExampleStep> step1 = new GetEffectivePageForIconStep(stepper);
nsCOMPtr<ExampleStep> step2 = new CheckIconExpirationStep(stepper);
nsCOMPtr<ExampleStep> step2 = new IconFetchStep(stepper);
...
// Init the party.
stepper->Step();
I've gone through 2 or 3 different implementations, but this one seems the simpler and cleaner, i can create generic and simple enough steps, and mix them in the needed order. Each step is async and callbacks the stepper when done, it can also save data in the stepper through setters. When the stepper is done will call the nsIFaviconDataCallback with the available data. If a step fails it will notify the stepper that will just unlink all steps and bail out.
Reporter | ||
Comment 6•15 years ago
|
||
I'm attaching some wip patch to get feedback. i'm still iterating a bit on design (for example i think instead of new step(stepper) i'll probably go for a stepper.addStep(new step)), i had to discard some nice code separation to avoid making the code bloat but still retainin some acceptable design. the single steps are much dedicated to a single task and that makes easier to maintain them, plus being full classes they can inherit from async statements or whatever needed.
Reporter | ||
Comment 7•15 years ago
|
||
Reporter | ||
Comment 8•15 years ago
|
||
Reporter | ||
Comment 9•15 years ago
|
||
asking for an approach-review or first-pass (depending on your time availability).
i've been able to refactor all doSetAndLoadFavicon steps into AsyncFaviconSteps, they are better separated and readable (for now i'm only copying code, removal of old sync code will come later). i still have to put them toghether to view the full picture (and ensure consistency), ideally this way i should be able to use the same approach for all the favicons service APIs. By the end of the week i should have a couple APIs converted to better evaluate what is missing.
Attachment #426658 -
Attachment is obsolete: true
Attachment #427484 -
Flags: review?(sdwilsh)
Reporter | ||
Comment 10•15 years ago
|
||
attaching a couple example steps
Attachment #426659 -
Attachment is obsolete: true
Reporter | ||
Comment 11•15 years ago
|
||
Attachment #426660 -
Attachment is obsolete: true
Reporter | ||
Comment 12•15 years ago
|
||
this ends up being (pseudo-code):
nsCOMPtr<AsyncFaviconStepper> stepper = new AsyncFaviconStepper(callback);
stepper->SetSomeKnownData();
stepper->AppendStep(new SomeStep());
stepper->AppendStep(new SomeOtherStep());
stepper->Start();
Reporter | ||
Comment 13•15 years ago
|
||
Apart some ovious needed cleanup and automated tests, doSetAndLoadFaviconForPage has been successfully converted, i see icons appearing on the bookmarks toolbar on visit, and it's nice.
I will probably have to add an aStatus param to the callback, and call it regardless on failure and cancelation, to ensure testability of the various paths. it should have default values like STATUS_OK, STATUS_FAILED, STATUS_CANCELED (even if ucanceling just means avoiding some useless work, so it could go away). will evaluate this while writing tests (starting test-driven dev now that the basic architecture is done, i need new tests both to avoid regressing old behaviors and to avoid regressing this stuff when converting other APIs).
Reporter | ||
Comment 14•15 years ago
|
||
This is an amalgamation of the patches, to get feedback on the full approach. Clearly review will be on separated patches, one per step.
Attachment #427484 -
Attachment is obsolete: true
Attachment #427485 -
Attachment is obsolete: true
Attachment #427486 -
Attachment is obsolete: true
Attachment #430341 -
Flags: feedback?(sdwilsh)
Attachment #427484 -
Flags: review?(sdwilsh)
Reporter | ||
Comment 15•15 years ago
|
||
part1 and the amalgamation should now apply cleanly to current trunk.
Notice:
1. this only converts doSetAndLoadFaviconForPage
2. the callback is probably going to get a new aReason short int param, for testing purposes (recognize when we did early cancel/fail fetching an icon).
Comment 16•15 years ago
|
||
Comment on attachment 430341 [details] [diff] [review]
amalgamation for feedback
Feedback given over irc/e-mail. This looks good I think and should be ready for review.
Attachment #430341 -
Flags: feedback?(sdwilsh) → feedback+
Reporter | ||
Comment 17•15 years ago
|
||
i'll post final patches along the week.
Comment 18•15 years ago
|
||
I should also add that we should remove the favicon stuff from the LAZY_ADD bits in this bug too since it'll all be async.
Reporter | ||
Comment 19•15 years ago
|
||
(In reply to comment #18)
> I should also add that we should remove the favicon stuff from the LAZY_ADD
> bits in this bug too since it'll all be async.
we can't. we have to add visit and LATER add favicon, since visits adding is not async, i can't kill lazy add here.
Reporter | ||
Comment 20•15 years ago
|
||
and a reason for comment 19 among other things is that we don't want to save icons for pages we don't have.
Comment 21•15 years ago
|
||
(In reply to comment #20)
> and a reason for comment 19 among other things is that we don't want to save
> icons for pages we don't have.
In what situations could that occur?
Reporter | ||
Comment 22•15 years ago
|
||
(In reply to comment #21)
> (In reply to comment #20)
> > and a reason for comment 19 among other things is that we don't want to save
> > icons for pages we don't have.
> In what situations could that occur?
i guess extensions, pages we can't (or don't want) to save.
There is also some risk that async expiration will collect favicons.
The code to add a page to history is not so straight forward. I should replicate it in favicons service to add a mock page entry. otherwise i should create a way to add an orphan icon, put the associate request somewhere, and after the page has been added serve the associate request.
It really looks like lot of work when we can just wait a bit more and completely kill LAZY_ADD with visits.
Reporter | ||
Updated•15 years ago
|
Attachment #422631 -
Attachment is obsolete: true
Reporter | ||
Updated•15 years ago
|
Attachment #430341 -
Attachment is obsolete: true
Reporter | ||
Comment 23•15 years ago
|
||
splitted bug 553489 to cover the first part of this work.
Reporter | ||
Comment 24•15 years ago
|
||
transforming into meta, implementation bugs should block this
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Summary: Favicon Service should be async → [META]Favicon Service should be async
Comment 25•13 years ago
|
||
Can someone please update:
https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIAsyncFavicons
with the implementation details (contract ID, service info) similar to:
https://developer.mozilla.org/en/nsIFaviconService
so that add-on developers can use this API?
I can't find the contract ID in the tree for this.
Reporter | ||
Comment 26•13 years ago
|
||
it's possible some bug lacked the dev-doc-needed keyword, adding it here as an additional coverage.
And marking the bug as fixed by dependencies since most APIs have an async alternative now, minor stuff can be handled apart.
Status: NEW → RESOLVED
Closed: 13 years ago
Depends on: 676906, asyncFavicons
Keywords: dev-doc-needed
Resolution: --- → FIXED
Comment 27•13 years ago
|
||
can you just give me the contract ID?
Reporter | ||
Comment 28•13 years ago
|
||
The old favicons service handles both the old and the new interface, the contract id didn't change, just have to QI to the interface you need (even if likely you don't even need that, since it implements ClassInfo)
Reporter | ||
Comment 29•13 years ago
|
||
and in js you can use PlacesUtils.favicons.
Comment 30•13 years ago
|
||
Moving dev-doc-needed to the blocking bugs where it should have been :)
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•