Replace old synchronous favicons calls in the bookmarks HTML import

RESOLVED FIXED in mozilla14

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:p1])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
See bug 713642 for background.
this should stay on-hold till bug 482911 lands, to avoid code conflicts.
(Assignee)

Comment 2

6 years ago
Created attachment 606232 [details] [diff] [review]
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.
Attachment #606232 - Flags: feedback?(mak77)
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 482911
if we use replaceFaviconData doesn't matter.
(Assignee)

Comment 6

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

What if replaceFaviconData fails?
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 on attachment 606232 [details] [diff] [review]
Reference patch

plase reflag me once bug 482911 and the patch is unbitrotted upon it
Attachment #606232 - Flags: feedback?(mak77)
"once bug 482911 lands"
(Assignee)

Updated

6 years ago
Summary: Replace old synchronous favicons calls in the Places import and export service → Replace old synchronous favicons calls in the bookmarks HTML import
(Assignee)

Comment 10

6 years ago
Created attachment 610584 [details] [diff] [review]
The patch
Attachment #606232 - Attachment is obsolete: true
Attachment #610584 - Flags: review?(mak77)
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
Attachment #610584 - Flags: review?(mak77)
(Assignee)

Comment 12

6 years ago
Created attachment 611164 [details] [diff] [review]
Updated patch
Attachment #610584 - Attachment is obsolete: true
Attachment #611164 - Flags: review?(mak77)
(Assignee)

Comment 13

6 years ago
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?
(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 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.
Attachment #611164 - Flags: review?(mak77) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 611747 [details] [diff] [review]
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 :-)
Attachment #611164 - Attachment is obsolete: true
Whiteboard: [snappy]

Updated

6 years ago
Whiteboard: [snappy] → [snappy:p1]
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/27616988a4a1
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/27616988a4a1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
No longer blocks: 730752
You need to log in before you can comment on or make changes to this bug.