Closed Bug 728142 Opened 12 years ago Closed 12 years ago

Replace old synchronous favicons calls in tabview

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: [snappy:p1])

Attachments

(1 file, 2 obsolete files)

See bug 713642 for background.
Attached patch For feedback (obsolete) — Splinter Review
We're moving to the asynchronous API for favicons, and we're not able to set
the source attributes on image elements at a known point in time anymore.

Tim, can you take a look to this initial overview of the changes in tabview
and tell me if you have any suggestions about the approach? The tricky part
is the fact that the "src" attribute is read in some parts of the code, and
I don't know if it's been already written at that point or not.

In addition, I don't know whether the image elements are reused; if so, we
have to make sure that older requests don't overwrite the latest icon (as the
requests can be completed in any order).
Attachment #598174 - Flags: feedback?(ttaubert)
Comment on attachment 598174 [details] [diff] [review]
For feedback

Redirecting the first feedback to Raymond as he is pretty familiar with the favicon code in Panorama.
Attachment #598174 - Flags: feedback?(ttaubert) → feedback?(raymond)
Comment on attachment 598174 [details] [diff] [review]
For feedback

It looks fine.

>   getAppTabFavIconUrl: function GroupItems_getAppTabFavIconUrl(xulTab) {

We can add second param to callback function so when the fav url is returned, that callback should be invoked.

>    faviconURLOf: function TabUtils_faviconURLOf(tab) {
> +    // TODO: Getting the favicon URI becomes asynchronous. This means we
> +    //       should find a way to properly synchronize reading $favImage[0].
>     return tab.image != undefined ? tab.image : tab.$favImage[0].src;
>   },

Is tab.image going to be available after switching to async fav icon service?  

>  getFavIconUrlForTab: function UI_getFavIconUrlForTab(tab) {
We should add callback function as second param and update all callers.


> +++ b/browser/components/tabview/test/browser_tabview_bug600645.js
> @@ -38,16 +38,18 @@ function onTabViewWindowLoaded() {
>  
>      // since the browser code and test code are invoked when an error event is 
>      // fired, a delay is used here to avoid the test code run before the browser 
>      // code.
>      executeSoon(function() {
>        let iconSrc = $icon.attr("src");
>        let hasData = true;
>        try {
> +        // TODO: Not directly supported by asynchronous API. Understand what's
> +        //       being tested here and update the test accordingly.
>          fi.getFaviconDataAsDataURL(iconSrc);
>        } catch(e) {

This part is to test that when an error event is fired by a tab (tab icon fails to load), the icon src should not  return any data by fi.getFaviconDataAsDataURL and should display a default icon.
Attachment #598174 - Flags: feedback?(raymond) → feedback+
Blocks: 678374
Paolo: getFaviconImageForPage() in favicon service should be replaced by getFaviconURLForPage + getFaviconLinkForIcon in async favicon service.  What about getFaviconDataAsDataURL() and getFaviconLinkForIcon() in favicon service?
No longer blocks: 678374
Depends on: 678374
getFaviconLinkForIcon is just an helper that converts a uri to another uri, doesn't cause IO, so for now it is surviving.

getFaviconDataAsDataURL has no replacement, so either the caller does the conversion or we have to provide one.
btw, fwiw I don't think it's the scope of the favicon service to convert data to different formats, once you get the icon data making a datauri should be trivial
Attached patch More feedback (obsolete) — Splinter Review
Hi Raymond, I continued a bit on this patch, it's far from finished but I hoped
you could give me more input, in particular on why we need to call faviconURLOf
(and if there's still a reason why its parameter is "overloaded"), and how you
think we can resolve the fact that getFaviconURLForPage will never invoke the
callback if the page has no associated favicon.

Thanks!
Attachment #598174 - Attachment is obsolete: true
Attachment #606286 - Flags: feedback?(raymond)
I also see that there is a bit of overlapping with the work done in bug 678374,
though I wonder if the considerations here are also relevant for that code.
(In reply to Paolo Amadini from comment #7)
> Hi Raymond, I continued a bit on this patch, it's far from finished but I
> hoped you could give me more input, in particular on why we need to call
> faviconURLOf
> (and if there's still a reason why its parameter is "overloaded"), 

User can have more than one window and the search feature in Panorama can allow user to search other tabs from other window.  Here are the steps you can try:
1) In your current window, open a tab with http://www.mozilla.org
2) Open a new window, go into Panorama
3) Active the search mode and type mozilla
4) You should see a bar appear at the bottom with mozilla fav icon.

That's the reason why we have faviconURLOf to get the fav icon from other window's tabs. 

> and how you
> think we can resolve the fact that getFaviconURLForPage will never invoke the
> callback if the page has no associated favicon.

Two possible solutions:
1) getFaviconURLForPage would invoke the callback after a certain time even the page has no associated favicon.
2) Set a placeholder (the default fav icon) for new appTab/tab and when the url of tab is changing before calling UI_getFavIconUrlForTab(). Hence, even getFaviconURLForPage doesn't return anything, user can still see a default fav icon.


I have checked and there are some overlappings in your patch and the one in bug 678374.  The patch in bug 678374 has already landed in fx-team branch so please based on that to create your patch.
Attachment #606286 - Flags: feedback?(raymond) → feedback?
Attachment #606286 - Flags: feedback?
Depends on: 737133
(In reply to Raymond Lee [:raymondlee] from comment #9)
> User can have more than one window and the search feature in Panorama can
> allow user to search other tabs from other window.  Here are the steps you
> can try:
> 1) In your current window, open a tab with http://www.mozilla.org
> 2) Open a new window, go into Panorama
> 3) Active the search mode and type mozilla
> 4) You should see a bar appear at the bottom with mozilla fav icon.

Thank you!

> > and how you
> > think we can resolve the fact that getFaviconURLForPage will never invoke the
> > callback if the page has no associated favicon.
> 
> Two possible solutions:

I've filed bug 737133 to change the API so that we get notified in all cases.
Attached patch The patchSplinter Review
On top bug 737133 and bug 678374, the patch is now much simpler.
Attachment #606286 - Attachment is obsolete: true
Attachment #610542 - Flags: review?(raymond)
Comment on attachment 610542 [details] [diff] [review]
The patch

Looks ok to me.  Requested tim to review it.
Attachment #610542 - Flags: review?(ttaubert)
Attachment #610542 - Flags: review?(raymond)
Attachment #610542 - Flags: feedback+
Comment on attachment 610542 [details] [diff] [review]
The patch

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

Thanks, looks good to me!
Attachment #610542 - Flags: review?(ttaubert) → review+
Whiteboard: [snappy]
Whiteboard: [snappy] → [snappy:p1]
https://hg.mozilla.org/integration/mozilla-inbound/rev/308e3b7254b3
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/308e3b7254b3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: