Last Comment Bug 728142 - Replace old synchronous favicons calls in tabview
: Replace old synchronous favicons calls in tabview
Status: RESOLVED FIXED
[snappy:p1]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 14
Assigned To: :Paolo Amadini
:
Mentors:
Depends on: 678374 737133
Blocks: asyncFaviconCallers 740807
  Show dependency treegraph
 
Reported: 2012-02-17 01:05 PST by :Paolo Amadini
Modified: 2016-04-12 14:00 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
For feedback (6.10 KB, patch)
2012-02-17 01:43 PST, :Paolo Amadini
raymond: feedback+
Details | Diff | Review
More feedback (10.12 KB, patch)
2012-03-15 10:46 PDT, :Paolo Amadini
no flags Details | Diff | Review
The patch (4.85 KB, patch)
2012-03-29 07:36 PDT, :Paolo Amadini
ttaubert: review+
raymond: feedback+
Details | Diff | Review

Description :Paolo Amadini 2012-02-17 01:05:19 PST
See bug 713642 for background.
Comment 1 :Paolo Amadini 2012-02-17 01:43:49 PST
Created attachment 598174 [details] [diff] [review]
For feedback

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).
Comment 2 Tim Taubert [:ttaubert] 2012-02-20 04:13:12 PST
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.
Comment 3 Raymond Lee [:raymondlee] 2012-02-21 05:57:05 PST
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.
Comment 4 Raymond Lee [:raymondlee] 2012-02-21 20:47:34 PST
Paolo: getFaviconImageForPage() in favicon service should be replaced by getFaviconURLForPage + getFaviconLinkForIcon in async favicon service.  What about getFaviconDataAsDataURL() and getFaviconLinkForIcon() in favicon service?
Comment 5 Marco Bonardo [::mak] 2012-02-22 03:13:28 PST
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.
Comment 6 Marco Bonardo [::mak] 2012-02-22 03:14:37 PST
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
Comment 7 :Paolo Amadini 2012-03-15 10:46:19 PDT
Created attachment 606286 [details] [diff] [review]
More feedback

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!
Comment 8 :Paolo Amadini 2012-03-15 10:49:16 PDT
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.
Comment 9 Raymond Lee [:raymondlee] 2012-03-15 22:12:19 PDT
(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.
Comment 10 :Paolo Amadini 2012-03-19 12:40:26 PDT
(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.
Comment 11 :Paolo Amadini 2012-03-29 07:36:23 PDT
Created attachment 610542 [details] [diff] [review]
The patch

On top bug 737133 and bug 678374, the patch is now much simpler.
Comment 12 Raymond Lee [:raymondlee] 2012-03-30 00:17:07 PDT
Comment on attachment 610542 [details] [diff] [review]
The patch

Looks ok to me.  Requested tim to review it.
Comment 13 Tim Taubert [:ttaubert] 2012-03-30 05:22:32 PDT
Comment on attachment 610542 [details] [diff] [review]
The patch

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

Thanks, looks good to me!
Comment 15 Marco Bonardo [::mak] 2012-04-13 03:58:17 PDT
https://hg.mozilla.org/mozilla-central/rev/308e3b7254b3

Note You need to log in before you can comment on or make changes to this bug.