Closed
Bug 728142
Opened 13 years ago
Closed 13 years ago
Replace old synchronous favicons calls in tabview
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 14
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
(Whiteboard: [snappy:p1])
Attachments
(1 file, 2 obsolete files)
4.85 KB,
patch
|
ttaubert
:
review+
raymondlee
:
feedback+
|
Details | Diff | Splinter Review |
See bug 713642 for background.
Assignee | ||
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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 3•13 years ago
|
||
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+
Comment 4•13 years ago
|
||
Paolo: getFaviconImageForPage() in favicon service should be replaced by getFaviconURLForPage + getFaviconLinkForIcon in async favicon service. What about getFaviconDataAsDataURL() and getFaviconLinkForIcon() in favicon service?
Updated•13 years ago
|
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 7•13 years ago
|
||
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)
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #606286 -
Flags: feedback?(raymond) → feedback?
Updated•13 years ago
|
Attachment #606286 -
Flags: feedback?
Assignee | ||
Comment 10•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
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 12•13 years ago
|
||
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 13•13 years ago
|
||
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+
Updated•13 years ago
|
Whiteboard: [snappy]
Updated•13 years ago
|
Whiteboard: [snappy] → [snappy:p1]
Assignee | ||
Comment 14•13 years ago
|
||
Target Milestone: --- → Firefox 14
Comment 15•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•