Last Comment Bug 713642 - (asyncFaviconCallers) Replace all old synchronous favicons calls in the codebase
(asyncFaviconCallers)
: Replace all old synchronous favicons calls in the codebase
Status: RESOLVED FIXED
[snappy:p1]
: meta, perf
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Marco Bonardo [::mak]
Mentors:
Depends on: asyncFavicons 713269 728140 728141 728142 728143 728168 728174 730752 731942 740468 760971 836624
Blocks: StorageJank 834457
  Show dependency treegraph
 
Reported: 2011-12-27 06:25 PST by Marco Bonardo [::mak]
Modified: 2013-02-06 03:30 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Overview of the changes (33.17 KB, patch)
2012-02-13 04:08 PST, :Paolo Amadini
no flags Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2011-12-27 06:25:15 PST
Tests and code should stop using the synchronous favicons api.
Comment 1 :Paolo Amadini 2012-02-13 04:08:13 PST
Created attachment 596649 [details] [diff] [review]
Overview of the changes

This summarizes the changes needed to remove the synchronous calls from the
codebase, using a simple approach that tries to minimize changes on the callers.

Marco, can you take a look and give me an indication about which items have
higher priority, and if we don't need to work on some items right now?
Comment 2 Marco Bonardo [::mak] 2012-02-13 12:27:20 PST
Comment on attachment 596649 [details] [diff] [review]
Overview of the changes

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

I'd say to split by modules: browser, toolkit, mobile, panorama sounds like a good separation.
Make a separate bug for each so we can get feedback/reviews in a cleaner way.

If you need more feedback feel free to ping me on specific points

::: browser/components/feeds/src/WebContentConverter.js
@@ +457,1 @@
>        var notificationIcon = fis.getFaviconLinkForIcon(uri);

good question, though I see this code landed without any sort of test, so it may likely be just broken. it would work only for chrome:// urls
Should be filed and handled apart as a dependency

::: browser/components/places/tests/unit/test_457441-import-export-corrupt-bookmarks-html.js
@@ +212,5 @@
>    do_check_eq(unfiledBookmarks.childCount, 1);
>    unfiledBookmarks.containerOpen = false;
>  
>    // favicons
> +  // TODO: Convert to getFaviconDataForPage.

you may split importexport tests to a separate bug, those may be picky

::: browser/components/tabview/groupitems.js
@@ +1189,5 @@
>    //                    called.
>    addAppTab: function GroupItem_addAppTab(xulTab, options) {
>      let self = this;
>  
> +    // TODO: Becomes asynchronous.

panorama changes should be filed apart so we can get better feedback from tim

::: mobile/xul/chrome/content/TabsPopup.js
@@ +104,5 @@
>            icon = iconURI.spec;
>          }
>        }
> +      // TODO: Replace getFaviconImageForPage with getFaviconURLForPage, and set
> +      //       this attribute asynchronously.

as well as mobile changes should be filed in a bug apart. even if this is xul build that will soon be unsupported but we should fix it.

::: toolkit/components/places/nsPlacesImportExportService.cpp
@@ +1239,1 @@
>      faviconSpec.AssignLiteral("http://www.mozilla.org/2005/made-up-favicon/");

this is a fake url, replaceFaviconDataFromDataURL should force setAndFetchFaviconForPage to NOT hit the network.
A pretty comment will help clarifying that.

::: toolkit/components/places/nsPlacesImportExportService.h
@@ +41,1 @@
>      nsCOMPtr<nsIFaviconService> mFaviconService;

these are just stupid hacks to ensure the service is alive, convert it mozIAsyncFavicons and will be fine

::: toolkit/components/places/tests/unit/test_404630.js
@@ +36,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
>  function run_test() {
> +  // TODO: Convert this test to the asynchronous API?

sure, setAndFetch is just an alias to setAndLoad, so we don't lose test coverage.
We should replace any sync api that may cause IO or has a clear replacement.
Comment 3 (dormant account) 2012-02-21 16:23:10 PST
main thread io -> p1
Comment 4 Marco Bonardo [::mak] 2013-01-16 03:07:29 PST
if anyone is interested in taking this bug, the patch should be splitted into the single components, tests can be splitted by parent folder.
Comment 5 :Paolo Amadini 2013-01-17 04:26:11 PST
I think the only remaining dependency is bug 728140, anyone able to test on mobile
should feel free to take the bug.
Comment 6 Marco Bonardo [::mak] 2013-01-17 04:36:46 PST
mobile/xul is dead btw, though nobody removed it cause Metro was taking code from it...
Whoever works on this should take APIs in nsIFaviconService and search mxr for them, looking for remaining calls, I could find some.
Comment 7 :Paolo Amadini 2013-01-17 06:37:27 PST
(In reply to Marco Bonardo [:mak] from comment #6)
> mobile/xul is dead btw, though nobody removed it cause Metro was taking code
> from it...

Do you mean it's not built anymore, and we can mark bug 728140 as WONTFIX?
That would close our last dependency! :-)

> Whoever works on this should take APIs in nsIFaviconService and search mxr
> for them, looking for remaining calls, I could find some.

You're right, it looks like "getFaviconForPage" and "getFaviconDataAsDataURL"
are still called in "test_bookmarks_html.js". But this is the only file, apart
from "test_favicons.js", that is going away when the APIs are removed.

The following calls have been eradicated:
* setFaviconUrlForPage
* setFaviconData
* getFaviconData
* getFaviconForPage
* setAndLoadFaviconForPage (present in mobile/xul)
* getFaviconImageForPage (present in mobile/xul)

The other functions of nsIFaviconService do not have an asynchronous version.
Comment 8 Marco Bonardo [::mak] 2013-01-17 06:46:25 PST
just asked in bug 831236, but I think the answer is positive. Once we have an answer we can wontfix the mobile/xul bug and just handle the remaining calls here.
Comment 9 Marco Bonardo [::mak] 2013-01-24 13:22:49 PST
Paolo, do you plan to work on the remaining parts, or should we kindly ask Appcoast guys to take care of them?
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-01-25 14:37:31 PST
Is there any relevant code on elm to remove? Bug 747347 will be merging it to m-c, and much of it is copied from mobile/xul.
Comment 11 Marco Bonardo [::mak] 2013-01-25 14:39:26 PST
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)
> Is there any relevant code on elm to remove? Bug 747347 will be merging it
> to m-c, and much of it is copied from mobile/xul.

it's possible, I think elm deprecate Places API usage is something to track as a dependency of bug 834457.  Will file a bug there.
Comment 12 :Paolo Amadini 2013-01-30 00:56:25 PST
(In reply to Marco Bonardo [:mak] from comment #9)
> Paolo, do you plan to work on the remaining parts, or should we kindly ask
> Appcoast guys to take care of them?

Since bug 728140 is now closed, as per comment 7, the only work that has to be
done here is rewriting the calls in the "test_bookmarks_html.js" file while
removing the APIs.

This should not be too difficult, so feel free to take the bug!
Comment 13 Marco Bonardo [::mak] 2013-01-30 04:48:34 PST
Raymond, any available time to look at this?
Comment 14 Raymond Lee [:raymondlee] 2013-01-30 20:23:57 PST
Filed bug 836624.  Will look into that.
Comment 15 Marco Bonardo [::mak] 2013-02-06 03:30:21 PST
Fixed by dependencies

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