728174 - Replace old synchronous favicons calls in the bookmarks HTML import

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Paolo Amadini]

See bug 713642 for background.

Comment 1

[Marco Bonardo [:mak]]

this should stay on-hold till bug 482911 lands, to avoid code conflicts.

Comment 2

[:Paolo Amadini]

Attachment: Reference patch

(In reply to Marco Bonardo [:mak] from comment #1)
> this should stay on-hold till bug 482911 lands, to avoid code conflicts.

Arrr... your guess that I was about to start working on this was right, but the
message reached me too late! In any case I looked at the other patch, and the
same changes can be applied to the JavaScript implementation as well as the
tests there.

Comment 3

[:Paolo Amadini]

By the way, in the C++ implementation of bookmarks export, we make a synchronous
call to get the favicon URI. That single call is nested in a series of "Write..."
calls that are all cascaded synchronously. I don't see an easy way to change that.

If we want to change that, we should probably do it in a separate bug, as it may
even be easier to just reimplement the export part with asynchronous callbacks in
JavaScript, like it was done for the import.

Comment 4

[:Paolo Amadini]

Also, have you considered changing "http://www.mozilla.org/2005/made-up-favicon/"
to something like "urn:moz-made-up-favicon:"? If we use a fake URI, we might as
well use one that doesn't hit the network if we fail to store the associated
favicon data in the database.

Comment 5

[Marco Bonardo [:mak]]

if we use replaceFaviconData doesn't matter.

Comment 6

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #5)
> if we use replaceFaviconData doesn't matter.

What if replaceFaviconData fails?

Comment 7

[Marco Bonardo [:mak]]

we hopefully bail out earlier. Btw, I have no preferences in the fake icons url, we may do that, if we don't find points against it.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 606232 [details] [diff] [review]
Reference patch

plase reflag me once bug 482911 and the patch is unbitrotted upon it

Comment 9

[Marco Bonardo [:mak]]

"once bug 482911 lands"

Comment 10

[:Paolo Amadini]

Attachment: The patch

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 610584 [details] [diff] [review]
The patch

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

::: browser/components/places/tests/unit/test_457441-import-export-corrupt-bookmarks-html.js
@@ +84,5 @@
>  
>    // Check that every bookmark is correct
>    // Corrupt bookmarks should not have been imported
> +  database_check(function () {
> +    waitForAsyncUpdates(function() {

since you now do an async database check, you can avoid the wait.

@@ +124,1 @@
>      waitForAsyncUpdates(do_test_finished);

as well as here, you can avoid the wait

@@ +230,5 @@
>    unfiledBookmarks.containerOpen = false;
>  
>    // favicons
> +  icos.getFaviconDataForPage(uri(TEST_FAVICON_PAGE_URL),
> +    function DC_onFaviconDataAvailable(aURI, aDataLen, aData, aMimeType) {

sanity check aURI is not null

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +621,5 @@
>      // worry about data
>      if (aIconURI) {
>        if (aIconURI.scheme == "chrome") {
> +        PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, aIconURI,
> +                                                       false);

may you please add a test for adding a chrome icon uri if we don't have one?

@@ +657,5 @@
> +      PlacesUtils.favicons.replaceFaviconDataFromDataURL(faviconURI, aData, 0);
> +    } catch (ex) {
> +      Cu.reportError(ex);
> +    }
> +    PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, faviconURI, false);

If we fail to store the data, there's no point in storing the icon, we'd just associate a new icon next time the page is visited, couldn't we just move this inside the try then, or early return?
And then fix the above comment about hitting the network

Comment 12

[:Paolo Amadini]

Attachment: Updated patch

Comment 13

[:Paolo Amadini]

Comment on attachment 611164 [details] [diff] [review]
Updated patch

>+      // aURI should never be null when aDataLen > 0.
>+      do_check_true(aURI);

Attached an old patch, this should read "do_check_neq(aURI, null)",
"do_check_true(!!aURI)" or "do_check_true(aURI != null)". We use all
these versions in the codebase. Preferences?

Comment 14

[Marco Bonardo [:mak]]

(In reply to Paolo Amadini [:paolo] from comment #13)
> Attached an old patch, this should read "do_check_neq(aURI, null)",

I like this one!

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 611164 [details] [diff] [review]
Updated patch

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

::: browser/components/places/tests/unit/test_bookmarks_html.js
@@ +106,5 @@
>  
> +/**
> + * Returns the base64-encoded version of the given string.
> + */
> +function xpcshell_btoa(aString) {

hm, move this to the head? sounds like useful. I'd avoid the xpcshell prefix though, may confuse mxr searches. maybe just base64Encode.

@@ +261,5 @@
> +  // 5. import bookmarks.exported.html
> +  // 6. run the test-suite
> +  // 7. remove the bookmark pointing to a chrome favicon.
> +  // 8. export to bookmarks.exported.html
> +  // 9. empty bookmarks db and continue

funny times eh. one day this should be splitted and cleaned up :(
Maybe once we'll have a new export as well.

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +651,4 @@
>      // This could fail if the favicon is bigger than defined limit, in such a
> +    // case neither the favicon URI nor the favicon data will be saved.  If the
> +    // bookmark is visited again later, the URI and data will be fetched.
> +    PlacesUtils.favicons.replaceFaviconDataFromDataURL(faviconURI, aData, 0);

the last param is optional, thus you can avoid it.

Comment 16

[:Paolo Amadini]

Attachment: Final patch

(In reply to Marco Bonardo [:mak] from comment #15)
> > +/**
> > + * Returns the base64-encoded version of the given string.
> > + */
> > +function xpcshell_btoa(aString) {
> 
> hm, move this to the head? sounds like useful. I'd avoid the xpcshell prefix
> though, may confuse mxr searches. maybe just base64Encode.

I've called it base64EncodeString, for the poor guys who'll try to figure out
where this magic function is located in our codebase :-)

Comment 17

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27616988a4a1

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/27616988a4a1