Create a mobile bookmarks root

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [sync-tracker])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Splitting this out of bug 647605. The query handling needs some more work, but we can still have Places create the root and let Sync manage the query for now.
Comment hidden (mozreview-request)
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

Does this seem reasonable? There are a couple of cases Mark and I talked about that probably won't happen in practice, but that this handles:

* Multiple folders with the "mobile/bookmarksRoot" anno. I see this was an issue in the past (bug 627519), but might have gone away now that Sync uses a query. This patch appends items from all folders with that anno to the new mobile root.

* Switching between channels (for example, moving from Nightly back to Aurora or Release) with the same profile. It's not clear to me this is something we support in practice, but this patch should let you do that by setting the "mobile/bookmarksRoot" anno on the new root...so old Sync code that still uses the anno for lookups should work.

Also:

* I haven't removed the mobile root creation code from Sync yet.

* This needs tests.

Mak, I'd love your feedback when you have a chance.
Attachment #8791440 - Flags: feedback?(markh)
Attachment #8791440 - Flags: feedback?(mak77)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

https://reviewboard.mozilla.org/r/78842/#review77514

This LGTM, although I think you also need to update Bookmarks.jsm (eg, the GUID should be listed there, remove() and eraseEverything() should check that folder ID too), and maybe even roll the sync changes into this patch (or a different patch in this bug) - there's be no need for Sync to continue using that anno once this lands.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

Now with less test breakage and SQL syntax errors.
Attachment #8791440 - Flags: feedback?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

Fixed an embarrassing typo. No migration tests yet; assuming the approach is good, here's what I'm thinking for those:

* Previous version without any mobile folder: make sure we create the new root and set its anno correctly.
* 1 mobile folder: move its children to the new root, then delete the old folder. Should also remove orphan annos.
* Multiple mobile folders (folders with mobile anno): merge their children, and delete the old folders.
* JSON import: with mobile root, without mobile root but with old mobile folder, multiple mobile folders, new root and folders with mobile anno.
Attachment #8791440 - Flags: feedback?(mak77)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8791822 [details]
Bug 1302901 - Remove mobile root creation from Sync.

https://reviewboard.mozilla.org/r/79082/#review78000
Attachment #8791822 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

Fixed remaining test failure, so that it doesn't show up in the other patches I'm working on.
Attachment #8791440 - Flags: feedback?(mak77)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

https://reviewboard.mozilla.org/r/78842/#review79958

looks like the patch is missing tests. What I mean is that various code points where you add mobile root to other roots are likely covered by some test (that for example checks we don't remove one of those roots from removeItem or remove) and the test should cover also the mobile root.
Same for JSON import/export, there's not test of the mobile root handling.
there's no test for the eraseEverything behavior on mobile root
there's no test for the migration in the unit/migration/ test folder

::: toolkit/components/places/BookmarkJSONUtils.jsm:450
(Diff revision 4)
> +          let isMobileFolder = aData.annos &&
> +                               aData.annos.some(anno => anno.name == PlacesUtils.MOBILE_ROOT_ANNO);
> +          let offset = 0;
> +          if (isMobileFolder) {
> +            // For mobile bookmarks folders, append their children to the root
> +            // instead of creating multiple mobile folders.

Could you please better explain this? Why should we create multiple mobile folders, we don't create multiple instances of the other roots, right, cause we call importJSONNode once per root.
So why would we end up creating multiple mobile roots?
Are mobile bookmarks not stored in a container in the first level of the json object? can we have other "mobile" folders around?
Is this a bug in the export path?

::: toolkit/components/places/Database.cpp:1871
(Diff revision 4)
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT b.id FROM moz_items_annos a "
> +    "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id "
> +    "JOIN moz_bookmarks b ON b.id = a.item_id "
> +    "WHERE n.name = :anno_name AND "
> +          "b.guid <> :guid"

where is :guid bound to the stmt? I can't find it.

and FWIW, looks like in a bunch of places you need to convert from guid to id, so at this point wouldn't be simpler in CreateMobileRoot to fetch the id after the insert or ignore and reuse (also return) it? it would simplify of bunch of other queries...

::: toolkit/components/places/Database.cpp:1882
(Diff revision 4)
> +  for (;;) {
> +    rv = mobileFoldersStmt->ExecuteStep(&hasMore);
> +    if (NS_FAILED(rv)) return rv;
> +    if (!hasMore) {
> +      break;
> +    }

while (NS_SUCCEEDED(mobileFoldersStmt->ExecuteStep(&hasMore)) && hasMore) ?

The c ases where the executeStep here can fail are likely to fail any other query, so swallowing the rv shouldn't matter much.

::: toolkit/components/places/Database.cpp:1894
(Diff revision 4)
> +      "UPDATE moz_bookmarks "
> +      "SET parent = (SELECT id FROM moz_bookmarks WHERE guid = :guid), "
> +          "position = position + ("
> +            "SELECT MAX(position) FROM moz_bookmarks b "
> +            "JOIN moz_bookmarks p ON p.id = b.parent "
> +            "WHERE p.guid = :guid)"

position can be 0, won't we potentially end up with 2 max(position) entries? Looks like this wants a +1?
also, what if max(position) is NULL? NULL + 1 is still NULL.

::: toolkit/components/places/Database.cpp:1923
(Diff revision 4)
> +        "SELECT b.id "
> +        "FROM moz_items_annos a "
> +        "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id "
> +        "JOIN moz_bookmarks b ON b.id = a.item_id "
> +        "WHERE n.name = :anno_name AND "
> +              "b.guid <> :guid"

couldn't you collect the ids at the previous step?
It sounds simpler to run the select, collect all the ids and then move stuff and cleanup in a loop.

SELECT old mobile root ids
for each (root ids) {
  move children
  remove root id
  remove root id annotations
}

::: toolkit/components/places/Database.cpp:1941
(Diff revision 4)
> +    rv = stmt->Execute();
> +    if (NS_FAILED(rv)) return rv;
> +  }
> +
> +  // Clean up orphan annotations for the removed folders.
> +  rv = RemoveOrphanAnnotations();

since you have the ids, it should be trivial to do in the loop instead.

::: toolkit/components/places/Database.cpp:2028
(Diff revision 4)
> +         "WHERE n.name = :anno_name AND "
> +               "b.guid = :guid"
> +        "), (SELECT id FROM moz_bookmarks WHERE guid = :guid),"
> +        "(SELECT id FROM moz_anno_attributes WHERE name = :anno_name), "
> +        "1, 0, :expiration, :type, :timestamp, :timestamp"
> +      ")"

You think you could probably rewrite as
INSERT OR IGNORE...
SELECT
  (SELECT a.id FROM moz_items_annos a
   JOIN moz_anno_attributes n ON n.name = :anno_name  
   WHERE item_id = b.id),
  id ,
  (SELECT id FROM moz_anno_atrributes WHERE name = :anno_name),
  1, 0, :expiration, :type, :timestamp, :timestamp"
FROM moz_bookmarks WHERE guid = :guid

that would save a couple subqueries to fetch the id from the guid.

::: toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js:67
(Diff revision 4)
>    Assert.equal(frecencyForUrl("http://example.com/"), frecencyForExample);
>    Assert.equal(frecencyForUrl("http://example.com/"), frecencyForMozilla);
>  
>    // Check there are no orphan annotations.
>    let conn = yield PlacesUtils.promiseDBConnection();
> -  let rows = yield conn.execute(`SELECT * FROM moz_items_annos`);
> +  let annoAttrs = yield conn.execute(`SELECT id, name FROM moz_anno_attributes`);

we'll want a followup bug to remove the anno once we are past 2 or 3 versions from the one where this lands. And here a TODO comment pointing to that bug #

::: toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js:102
(Diff revision 4)
>    let bm2 = yield PlacesUtils.bookmarks.remove(bm1.guid);
>    checkBookmarkObject(bm2);
>  
>    // Check there are no orphan annotations.
>    let conn = yield PlacesUtils.promiseDBConnection();
> -  let rows = yield conn.execute(`SELECT * FROM moz_items_annos`);
> +  let annoAttrs = yield conn.execute(`SELECT id, name FROM moz_anno_attributes`);

ditto

::: toolkit/components/places/tests/expiration/test_annos_expire_session.js:64
(Diff revision 4)
>        `SELECT id FROM moz_annos
>         UNION ALL
> -       SELECT id FROM moz_items_annos`
> +       SELECT a.id FROM moz_items_annos a
> +       LEFT JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id
> +       WHERE n.name <> :mobileRootAnno`
>      );

looks like the query instead should filter on only SESSION annotations, mobile is not one of those. it's just too broad

Updated

2 years ago
Attachment #8791440 - Flags: feedback?(mak77)
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

https://reviewboard.mozilla.org/r/78842/#review79958

Thanks, Mak! I didn't add tests yet because I wanted to see if the approach was sensible. From your comments, it sounds like this is the way to go, so I've added some in this revision: everything in comment 9, plus the ones you pointed out.

One thing I didn't think about is queries that point to the old mobile folders. The restore code handles this in `fixupQuery`, but we don't have anything like that for migrations, so we'll invalidate queries made by Sync and any copies the user made. I wanted to use `nsNavHistoryQuery` for this, but its `AppendFolder` method ends up round-tripping back to the bookmarks service, and then we're in a world of pain trying to use it before we've finished migrating.

So, I hacked up a `FixupFolderQuery` function that does a "simple" (58-line, because C++) string replacement, and another function that replaces old queries with the new one. Can you think of a better way to do this? ("Delete them" or "just orphan them" are OK, too).

> Could you please better explain this? Why should we create multiple mobile folders, we don't create multiple instances of the other roots, right, cause we call importJSONNode once per root.
> So why would we end up creating multiple mobile roots?
> Are mobile bookmarks not stored in a container in the first level of the json object? can we have other "mobile" folders around?
> Is this a bug in the export path?

We don't want to create multiple mobile folders. But, if we have multiple folders with that anno, we want to merge them into the new root. In theory, we shouldn't have more than one. As I said in comment 2, though, it sounds like old versions of Sync may have done the wrong thing and created multiple folders with that anno, or exposed a folder instead of a query in the UI. That's wrong.

But I don't know if it's something we should fix, or if we should just take the first folder with that anno (that's a child of the Places root) and ignore the others.

> where is :guid bound to the stmt? I can't find it.
> 
> and FWIW, looks like in a bunch of places you need to convert from guid to id, so at this point wouldn't be simpler in CreateMobileRoot to fetch the id after the insert or ignore and reuse (also return) it? it would simplify of bunch of other queries...

Done. Great idea, this really simplified the other queries.

> position can be 0, won't we potentially end up with 2 max(position) entries? Looks like this wants a +1?
> also, what if max(position) is NULL? NULL + 1 is still NULL.

Fixed with an IFNULL. And yes, I definitely want max(position) + 1. Thanks!

> couldn't you collect the ids at the previous step?
> It sounds simpler to run the select, collect all the ids and then move stuff and cleanup in a loop.
> 
> SELECT old mobile root ids
> for each (root ids) {
>   move children
>   remove root id
>   remove root id annotations
> }

Good suggestion! Done.

> You think you could probably rewrite as
> INSERT OR IGNORE...
> SELECT
>   (SELECT a.id FROM moz_items_annos a
>    JOIN moz_anno_attributes n ON n.name = :anno_name  
>    WHERE item_id = b.id),
>   id ,
>   (SELECT id FROM moz_anno_atrributes WHERE name = :anno_name),
>   1, 0, :expiration, :type, :timestamp, :timestamp"
> FROM moz_bookmarks WHERE guid = :guid
> 
> that would save a couple subqueries to fetch the id from the guid.

Fetching the root ID got rid of the GUID subquery, but I still rewrote it as you suggested to get rid of another subquery for the anno attribute ID.

> we'll want a followup bug to remove the anno once we are past 2 or 3 versions from the one where this lands. And here a TODO comment pointing to that bug #

Filed bug 1306445.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

Answered feedback in comment 15. In particular, I'd like your thoughts on the folder query issue.
Attachment #8791440 - Flags: feedback?(mak77)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

https://reviewboard.mozilla.org/r/78842/#review82510

It looks better, just some details to look into, but it should be landable then.

::: toolkit/components/places/BookmarkJSONUtils.jsm:359
(Diff revision 5)
>              let oldURI = PlacesUtils.bookmarks.getBookmarkURI(aId);
>              let uri = fixupQuery(oldURI, folderIdMap);
>              if (!uri.equals(oldURI)) {
>                PlacesUtils.bookmarks.changeBookmarkURI(aId, uri, this._source);
>              }
> -          });
> +          }, this);

may this just become a for...of?

::: toolkit/components/places/Database.cpp:313
(Diff revision 5)
> +
> +// Rewrites |aQuery| referencing |aOldFolderId| to point to |aNewFolderId|
> +// instead. Returns false if |aQuery| doesn't point to |aOldFolderId|;
> +// otherwise, returns true and updates |_newQuery|.
> +bool
> +FixupFolderQuery(const nsACString& aQuery, int64_t aOldFolderId,

Ok, regarding this, before I look into the implementation... What's the downside to just throwing these queries away?

I mean, this is not exactly "dataloss" since this is only a shortcut, it's about the same as losing a file symlink. It's also easy to recreate it, so we never really cared that much about this kind of loss.
But maybe Sync has other reasons for it?

::: toolkit/components/places/Database.cpp:2152
(Diff revision 5)
> +  if (NS_FAILED(rv)) return -1;
> +
> +  nsXPIDLString rootTitle;
> +  rv = bundle->GetStringFromName(u"MobileBookmarksFolderTitle",
> +                                 getter_Copies(rootTitle));
> +  if (NS_FAILED(rv)) return -1;

The title is not critical for functionality. We don't expose it anywhere, the frontend exposes shortcuts and fetches the titles by itself.
We do that in PlacesUIUtils... but the mobile root is still handled apart right? The reason to handle this in th frontend is that if the user changes localization, we translate the frontend shortcuts. I don't know/recall if we are doing that for the mobile root.

Since we are in the critical path and every failure may cause us to throw away the db as corrupt, I'd ignore if we can't fetch a title or the bundle itself and just use "mobile" as a fallback.
Then ensure the frontend code sets an appropriate title for the shortcut.

::: toolkit/components/places/tests/migration/xpcshell.ini:24
(Diff revision 5)
>    places_v30.sqlite
>    places_v31.sqlite
>    places_v32.sqlite
>    places_v33.sqlite
>    places_v34.sqlite
> +  places_v35.sqlite

Just to be sure, how did you generate places_v35.sqlite?
Did you rememeber to VACUUM it?
(In reply to Kit Cambridge [:kitcambridge] from comment #18)
> Answered feedback in comment 15. In particular, I'd like your thoughts on
> the folder query issue.

I'd throw them away unless we have critical reasons to not do that.

Updated

2 years ago
Attachment #8791440 - Flags: feedback?(mak77) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 years ago
mozreview-review-reply
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

https://reviewboard.mozilla.org/r/78842/#review82510

Thanks for looking at this!

> Ok, regarding this, before I look into the implementation... What's the downside to just throwing these queries away?
> 
> I mean, this is not exactly "dataloss" since this is only a shortcut, it's about the same as losing a file symlink. It's also easy to recreate it, so we never really cared that much about this kind of loss.
> But maybe Sync has other reasons for it?

If we never really cared about this kind of loss, I'm happy to get rid of the query rewriting. Sync will recreate it, anyway. Added a comment and updated the migration test.

> The title is not critical for functionality. We don't expose it anywhere, the frontend exposes shortcuts and fetches the titles by itself.
> We do that in PlacesUIUtils... but the mobile root is still handled apart right? The reason to handle this in th frontend is that if the user changes localization, we translate the frontend shortcuts. I don't know/recall if we are doing that for the mobile root.
> 
> Since we are in the critical path and every failure may cause us to throw away the db as corrupt, I'd ignore if we can't fetch a title or the bundle itself and just use "mobile" as a fallback.
> Then ensure the frontend code sets an appropriate title for the shortcut.

Makes sense. Since we don't expose this title in the UI, do we even need to get the bundle from the string at all? (I copied `CreateBookmarkRoots`, which does use the string). Can we just call it "mobile" in all cases?

Also, I'm not sure if we translate the mobile shortcut when the locale changes. How do we handle that for the others? If we delete and recreate the entire left pane root, Sync will do the right thing and create the shortcut with the new locale on the next sync. If not, I'll file a bug.

> Just to be sure, how did you generate places_v35.sqlite?
> Did you rememeber to VACUUM it?

I ran test_current_from_v34.js on places_v34.sqlite, then added a throwaway task to copy the migrated places.sqlite out of the test profile. I VACUUMed it in the latest patch.
(In reply to Kit Cambridge [:kitcambridge] from comment #23)
> Makes sense. Since we don't expose this title in the UI, do we even need to
> get the bundle from the string at all? (I copied `CreateBookmarkRoots`,
> which does use the string). Can we just call it "mobile" in all cases?

Ok I was wrong, sorry, I was going by memory. This is what actually happens:
1. The roots have a title.
2. The shortcuts to the roots don't have a title, they inherit the title from the root (shortcuts always inherit title from underlying folder IIRC).
3. PlacesUIUtils::leftPaneFolderId when is requested to fetch/build the left pane folder reads the roots titles from the bundle, compares it to the roots titles, if any title differs, it updates the root title through the usual bookmarks API (it is still using the old setItemTitle I think).

So, you need the bundle and set a title to the root. in the frontend when you fetch the mobile folder you should check its title is still correct for the bundle or update it.
If you have that frontend piece of code, you can ignore properly setting a title in the backend, otherwise you must also make the bundle part critical.
Sorry for saying a different thing before, I have verified now to be sure.
Well, I guess based on comment 24 you may want to updated the patch
Flags: needinfo?(kcambridge)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Cool, thanks for the explanation! Since we do have frontend code in Sync to set the title, and we also call `UpdateBookmarkRootTitles` after migration, I'm going to use "mobile" and remove the bundle part.
Flags: needinfo?(kcambridge)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8791440 [details]
Bug 1302901 - Create a Places mobile bookmarks root.

https://reviewboard.mozilla.org/r/78842/#review83466
Attachment #8791440 - Flags: review?(mak77) → review+

Comment 30

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0759241c66d
Create a Places mobile bookmarks root. r=mak
https://hg.mozilla.org/integration/autoland/rev/bf54d3ff51da
Remove mobile root creation from Sync. r=markh

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c0759241c66d
https://hg.mozilla.org/mozilla-central/rev/bf54d3ff51da
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Duplicate of this bug: 1266829
You need to log in before you can comment on or make changes to this bug.