1769074 - Favicons are not imported when loading JSON bookmark backups

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Mark Banner (:standard8)]

Favicons are not imported from JSON bookmark backups even though there is code that is meant to.

The issue is that in the JSON backups, the icon URIs are saved as iconuri, but when we attempt to load them, we're checking for iconUri.

In addition to fixing this, we should add a test to ensure that icons are fetched when importing from JSON (we should probably make sure that HTML imports have a test for icons as well).

Comment 1

[James Teow [:jteow]]

Attachment: Bug 1769074 - Convert iconuri to iconUri for JSON bookmark imports - r?standard8

insertFaviconForNode() in BookmarkJSONUtils.jsm is the same as BookmarkHTMLUtils.jsm
so I made a modification in the translateTreeTypes since it translates JSON types
to Places compatible types.

The added test and test json file is a minute variant of test_bookmarks_json.js and
bookmarks.json since they are exhaustive.

Comment 2

[James Teow [:jteow]]

So I paused the review because when going through the tests again, I feel like the existing tests should've caught this because one of things both the HTML and JSON import tests do is export and re-import the tests, and then check await getFaviconDataForPage(aExpected.url) to see if it matches the expected icon.

During that export process, favicons should be re-exported as iconuri and ICON_URI in JSON/HTML respectively and then in the import, and the favicons associated with the bookmarks should be the same as the import files (at least based on what I've seen of the tests). I think the reason why the test didn't catch it was because upon the initial JSON import (which uses the icon property), it sets the favicons. The tests clear the bookmarks, but the uri's associated with the favicons remain, so future imports that end up not using iconuri don't matter.

I assumed setAndFetchFaviconForPage() would override an existing favicon uri, but I don't always think that is the case. Perhaps if a uri is properly associated with a favicon, nothing happens? In the test I linked to, I checked the favicon associated with the uri after that function was called and it was the same as the one that was originally entered. I also smoke tested the behavior by importing bookmarks into my browser, deleting them, changed the favicon uri's in the JSON file, and then re-imported them: I expected the favicons to change to the new uri's but they remained the same.

Comment 3

[James Teow [:jteow]]

The reason why the existing JSON import test kept passing was because while the tests clear the bookmarks they don't clear the history or favicons. So when the test re-imports the bookmarks it exported, iconuri doesn't get read but it's irrelevant because the uri associated with the favicon remains.

Clearing the history before re-importing causes the tests to break on the re-import, because uri of the favicons are replaced with references to fake-favicon-uri due to the fact that the initial import uses icon rather than iconuri.

If I were to apply the small tweak I made in my patch where I move node.iconuri to node.iconUri, make it so that node.icon imports don't use a fake-favicon-uri for the favicon import (use the base64 uri directly), and clear the history between import/export, the tests pass. On the other hand, that doesn't address Bug 523932.

I wonder if the tests as part of the patch are fine in so far as they re-create a plausible situation, which is users importing a JSON file from a clean state rather than importing a JSON file that had icon and then re-imported it later as iconuri?

Comment 4

[Mark Banner (:standard8)]

(In reply to James Teow [:jteow] from comment #3)

I wonder if the tests as part of the patch are fine in so far as they re-create a plausible situation, which is users importing a JSON file from a clean state rather than importing a JSON file that had icon and then re-imported it later as iconuri?

Yes, I think that is fine, and it is always good to have a specific test case where you're loading a saved import from scratch - it is more likely to catch issues if later changes are made such that the export/import round trip works but import from older might not.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jteow, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 6

[Mark Banner (:standard8)]

Leaving NI for James.

Comment 7

[James Teow [:jteow]]

I will land the approved patch later today. Since it's been a while, I just want to re-base locally and test on Try to confirm it's still good to go.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68d3bdb5ebc0
Convert iconuri to iconUri for JSON bookmark imports - r=Standard8

Comment 9

[bszekely]

https://hg.mozilla.org/mozilla-central/rev/68d3bdb5ebc0