1379611 - Use Bookmarks.jsm in editBookmarkOverlay.js

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P1)

Description

[Marco Bonardo [:mak]]

We should convert synchronous bookmarks usage in editBookmarkOverlay.js

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1379611 - Avoid GetFolderIdForItem() synchronous API in editBookmarkOverlay.js.

Review commit: https://reviewboard.mozilla.org/r/156728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156728/

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1379611 - Avoid GetBookmarkURI() synchronous API in editBookmarkOverlay.js.

Review commit: https://reviewboard.mozilla.org/r/156730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156730/

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1379611 - Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js.

Review commit: https://reviewboard.mozilla.org/r/156732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156732/

Comment 4

[Marco Bonardo [:mak]]

This may need an unbitrot against the patches currently landing. But overall it's ready for a first look.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8885940 [details]
Bug 1379611 - Avoid GetBookmarkURI() synchronous API in editBookmarkOverlay.js.

https://reviewboard.mozilla.org/r/156730/#review161996

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8885939 [details]
Bug 1379611 - Avoid GetFolderIdForItem() synchronous API in editBookmarkOverlay.js.

https://reviewboard.mozilla.org/r/156728/#review161990

::: browser/components/places/content/editBookmarkOverlay.js:1167
(Diff revision 1)
>          this._initLoadInSidebar();
>        break;
>      }
>    },
>  
> -  onItemMoved(aItemId, aOldParent, aOldIndex,
> +  onItemMoved(id, oldParentId, oldIndex, newParentId, bewIndex, type, guid,

typo: bewIndex

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8885941 [details]
Bug 1379611 - Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js.

https://reviewboard.mozilla.org/r/156732/#review161998

FYI there's a little bit of bitrot in browser-places.js but it doesn't seem hard to fix.

::: browser/components/places/content/editBookmarkOverlay.js:34
(Diff revision 1)
>      // properties of the target folder.
>      let itemId = node ? node.itemId : -1;
> -    let itemGuid = PlacesUIUtils.useAsyncTransactions && node ?
> +    let itemGuid = node ? PlacesUtils.getConcreteItemGuid(node) : null;
> -                     PlacesUtils.getConcreteItemGuid(node) : null;
>      let isItem = itemId != -1;
> -    let isFolderShortcut = isItem &&
> +    let isFolderShortcut = isItem && node &&

I think we still need the node check here - for legacy add-ons?

::: npm-shrinkwrap.json:770
(Diff revision 1)
>        "integrity": "sha1-eCA6TRwyiuHYbcpkYONptX9AVa4="
>      },
>      "minimatch": {
>        "version": "3.0.4",
>        "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.0.4.tgz",
> -      "integrity": "sha512-yJHVQEhyqPLUTgt9B83PXu6W3rx4MvvHvSUvToogpwoGDOUQ+yDrR0HRot+yOCdCO7u4hX3pWft6kWBBcqh0UA==",
> +      "integrity": "sha1-UWbihkV/AzBgZL5Ul+jbsMPTIIM=",

Strange, I wonder how the shas are different style. Still we probably don't want to commit this with this bug.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8885941 [details]
Bug 1379611 - Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js.

https://reviewboard.mozilla.org/r/156732/#review161998

> I think we still need the node check here - for legacy add-ons?

I added a node check indeed... but it's not needed, the isItem check already guarantees a node.
But maybe I'm misreading your comment?

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8885939 [details]
Bug 1379611 - Avoid GetFolderIdForItem() synchronous API in editBookmarkOverlay.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156728/diff/1-2/

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8885940 [details]
Bug 1379611 - Avoid GetBookmarkURI() synchronous API in editBookmarkOverlay.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156730/diff/1-2/

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8885941 [details]
Bug 1379611 - Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156732/diff/1-2/

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8885941 [details]
Bug 1379611 - Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156732/diff/2-3/

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8885941 [details]
Bug 1379611 - Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156732/diff/3-4/

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8885941 [details]
Bug 1379611 - Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js.

https://reviewboard.mozilla.org/r/156732/#review161998

> I added a node check indeed... but it's not needed, the isItem check already guarantees a node.
> But maybe I'm misreading your comment?

Looking back at the diff, I think I must have been miss-reading. You have certainly added a node check there, so I must have been mixing up which way around things were happening.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/422dc84cfcdc
Avoid GetFolderIdForItem() synchronous API in editBookmarkOverlay.js. r=standard8
https://hg.mozilla.org/integration/autoland/rev/675b048911ab
Avoid GetBookmarkURI() synchronous API in editBookmarkOverlay.js. r=standard8
https://hg.mozilla.org/integration/autoland/rev/c6ea57f05ae9
Avoid GetItemTitle() synchronous API in editBookmarkOverlay.js. r=standard8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/422dc84cfcdc
https://hg.mozilla.org/mozilla-central/rev/675b048911ab
https://hg.mozilla.org/mozilla-central/rev/c6ea57f05ae9