1131491 - Remove legacy places transactions code

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1131491 - Remove browser.places.useAsyncTransactions preference - async transactions are now the only version.

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

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1131491 - Remove old synchronous Place's transactions.

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

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8944439 [details]
Bug 1131491 - Remove browser.places.useAsyncTransactions preference - async transactions are now the only version.

https://reviewboard.mozilla.org/r/214640/#review220350

::: browser/components/places/PlacesUIUtils.jsm:260
(Diff revision 1)
>  
>    getString: function PUIU_getString(key) {
>      return bundle.GetStringFromName(key);
>    },
>  
>    get _copyableAnnotations() {

Looks like this can be removed since it's now unused... That makes me guess how we handle non-copiable annotations in PlacesTransactions. For example, the smart bookmarks annotations or the leftpane query annotations should not be copied because they are specific to the original bookmark. I suspect there may be a bug lying around and should be verified.

::: browser/components/places/PlacesUIUtils.jsm
(Diff revision 1)
> -    let feedURI, siteURI;
> -    let annos = [];
> -    if (aData.annos) {
> -      annos = aData.annos.filter(function(aAnno) {
> -        if (aAnno.name == PlacesUtils.LMANNO_FEEDURI) {
> -          feedURI = PlacesUtils._uri(aAnno.value);

Please file a mentored bug to remove PlacesUtils._uri

::: browser/components/places/content/controller.js:818
(Diff revision 1)
> -        var uri = NetUtil.newURI(node.uri);
> -        if (PlacesUIUtils.useAsyncTransactions) {
> -          let tag = node.parent.title;
> +        let tag = node.parent.title;
> -          if (!tag) {
> +        if (!tag) {
> +          let tagItemId = PlacesUtils.getConcreteItemId(node.parent);
> -            let tagGuid = await PlacesUtils.promiseItemGuid(tagItemId);
> +          let tagGuid = await PlacesUtils.promiseItemGuid(tagItemId);

can we getConcreteItemGuid here?

::: browser/components/places/content/controller.js:1473
(Diff revision 1)
>      }
>  
> -    if (PlacesUIUtils.useAsyncTransactions) {
> -      let nodes = [];
> +    let nodes = [];
> -      // TODO: When sync transactions are removed, merge the for loop here into the
> +    // TODO: When sync transactions are removed, merge the for loop here into the
> -      // one above.
> +    // one above.

File a bug to do this, if it's effectively feasible.

::: browser/components/places/tests/browser/browser.ini
(Diff revision 1)
>  [browser_click_bookmarks_on_toolbar.js]
>  [browser_controller_onDrop_tagFolder.js]
>  skip-if = (os == 'win' && ccov) # Bug 1423667
>  [browser_controller_onDrop.js]
>  skip-if = (os == 'win' && ccov) # Bug 1423667
> -[browser_copy_folder_tree.js]

please remember to file a bug to reimplement this test properly

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8944440 [details]
Bug 1131491 - Remove old synchronous Place's transactions.

https://reviewboard.mozilla.org/r/214642/#review220374

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8944439 [details]
Bug 1131491 - Remove browser.places.useAsyncTransactions preference - async transactions are now the only version.

https://reviewboard.mozilla.org/r/214640/#review220350

> Looks like this can be removed since it's now unused... That makes me guess how we handle non-copiable annotations in PlacesTransactions. For example, the smart bookmarks annotations or the leftpane query annotations should not be copied because they are specific to the original bookmark. I suspect there may be a bug lying around and should be verified.

We discussed this on irc, and found that the smart bookmark annotation is already being handled (https://searchfox.org/mozilla-central/rev/e9a067a401e41017766f4ab90bc3906603273d51/browser/components/places/PlacesUIUtils.jsm#545), but the leftpane query ones are not.

Since we're working on virtualisation of the left pane (bug 1310295), and whatever damage here is already done, we decided to continue focus on the virtualisation which will automatically resolve that part of the issue.

> Please file a mentored bug to remove PlacesUtils._uri

Filed bug 1432403

> can we getConcreteItemGuid here?

I tried, but I believe it broke tests. For now I've filed bug 1432405 as a follow-up to investigate.

> File a bug to do this, if it's effectively feasible.

Filed bug 1432407

> please remember to file a bug to reimplement this test properly

Filed bug 1432412

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8944439 [details]
Bug 1131491 - Remove browser.places.useAsyncTransactions preference - async transactions are now the only version.

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

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8944440 [details]
Bug 1131491 - Remove old synchronous Place's transactions.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/772b0d449e35
Remove browser.places.useAsyncTransactions preference - async transactions are now the only version. r=mak
https://hg.mozilla.org/integration/autoland/rev/9e0080c2930f
Remove old synchronous Place's transactions. r=mak

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/772b0d449e35
https://hg.mozilla.org/mozilla-central/rev/9e0080c2930f