Marco Bonardo [:mak] Reporter
	Description • 13 years ago

Tests and code should stop using the synchronous favicons api.

	:Paolo Amadini
	Comment 1 • 13 years ago

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?

	Marco Bonardo [:mak] Reporter
	Comment 2 • 13 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.

	(dormant account)
	Comment 3 • 13 years ago

main thread io -> p1

	Marco Bonardo [:mak] Reporter
	Comment 4 • 12 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.

	:Paolo Amadini
	Comment 5 • 12 years ago

I think the only remaining dependency is bug 728140, anyone able to test on mobile
should feel free to take the bug.

	Marco Bonardo [:mak] Reporter
	Comment 6 • 12 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.

	:Paolo Amadini
	Comment 7 • 12 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.

	Marco Bonardo [:mak] Reporter
	Comment 8 • 12 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.

	Marco Bonardo [:mak] Reporter
	Comment 9 • 12 years ago

Paolo, do you plan to work on the remaining parts, or should we kindly ask Appcoast guys to take care of them?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 10 • 12 years ago

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.

	Marco Bonardo [:mak] Reporter
	Comment 11 • 12 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.

	:Paolo Amadini
	Comment 12 • 12 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!

	Marco Bonardo [:mak] Reporter
	Comment 13 • 12 years ago

Raymond, any available time to look at this?

	Raymond Lee [:raymondlee]
	Comment 14 • 12 years ago

Filed bug 836624.  Will look into that.

	Marco Bonardo [:mak] Reporter
	Comment 15 • 12 years ago

Fixed by dependencies