Marco Bonardo [:mak] Assignee
	Description • 7 years ago

We should convert synchronous bookmarks usage in editBookmarkOverlay.js

	Marco Bonardo [:mak] Assignee
	Comment 4 • 7 years ago

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

	Mark Banner (:standard8)
	Comment 5 • 7 years ago
mozreview-review

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

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

	Mark Banner (:standard8)
	Comment 6 • 7 years ago
mozreview-review

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

	Mark Banner (:standard8)
	Comment 7 • 7 years ago
mozreview-review

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.

	Marco Bonardo [:mak] Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

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?

	Mark Banner (:standard8)
	Comment 14 • 7 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 15 • 7 years ago

Pushed by mak77@bonardo.net:
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

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 7 years ago
bugherder

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