Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 713642 (asyncFaviconCallers)

Replace all old synchronous favicons calls in the codebase

RESOLVED FIXED

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mak, Unassigned)

Tracking

(Blocks: 1 bug, {meta, perf})

Trunk
meta, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:p1])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Tests and code should stop using the synchronous favicons api.
(Reporter)

Updated

6 years ago
Keywords: meta, perf

Updated

6 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Alias: asyncFaviconCallers

Comment 1

6 years ago
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?
Attachment #596649 - Flags: feedback?(mak77)
(Reporter)

Comment 2

6 years ago
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.
Attachment #596649 - Flags: feedback?(mak77)

Updated

6 years ago
Depends on: 728140

Updated

6 years ago
Depends on: 728141

Updated

6 years ago
Depends on: 728142

Updated

6 years ago
Depends on: 728143

Updated

6 years ago
Depends on: 728168

Updated

6 years ago
Depends on: 728174

Updated

6 years ago
Whiteboard: [snappy]

Comment 3

6 years ago
main thread io -> p1
Whiteboard: [snappy] → [snappy:p1]
Blocks: 730752

Updated

6 years ago
Depends on: 731942

Updated

5 years ago
Depends on: 740468
No longer blocks: 730752
Blocks: 730752

Updated

5 years ago
Depends on: 760971
No longer blocks: 730752
Depends on: 730752
(Reporter)

Comment 4

5 years ago
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

5 years ago
I think the only remaining dependency is bug 728140, anyone able to test on mobile
should feel free to take the bug.
(Reporter)

Comment 6

5 years ago
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

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
Paolo, do you plan to work on the remaining parts, or should we kindly ask Appcoast guys to take care of them?
Flags: needinfo?(paolo.mozmail)
(Reporter)

Updated

5 years ago
Blocks: 834457
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.
(Reporter)

Comment 11

5 years ago
(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

5 years ago
(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!
Flags: needinfo?(paolo.mozmail)
(Reporter)

Comment 13

5 years ago
Raymond, any available time to look at this?
Assignee: paolo.mozmail → nobody
Flags: needinfo?(raymond)
Depends on: 836624
Filed bug 836624.  Will look into that.
Flags: needinfo?(raymond)
(Reporter)

Comment 15

5 years ago
Fixed by dependencies
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.