713642 - (asyncFaviconCallers) Replace all old synchronous favicons calls in the codebase

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Tests and code should stop using the synchronous favicons api.

Comment 1

[:Paolo Amadini]

Attachment: 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]]

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)]

main thread io -> p1

Comment 4

[Marco Bonardo [:mak]]

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]

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]]

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]

(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]]

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]]

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: [e-mail]]]

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]]

(In reply to :Gavin Sharp (use [e-mail] 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]

(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]]

Raymond, any available time to look at this?

Comment 14

[Raymond Lee [:raymondlee]]

Filed bug 836624.  Will look into that.

Comment 15

[Marco Bonardo [:mak]]

Fixed by dependencies