Remove legacy places transactions code

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mano, Assigned: standard8)

Tracking

(Blocks 2 bugs, {perf})

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

Assignee: asaf → nobody
Priority: -- → P3
No longer blocks: PlacesAsyncTransact
Priority: P3 → P2
Whiteboard: [fxsearch]
Keywords: perf
Blocks: 1083465
Assignee: nobody → standard8
Priority: P2 → P1
Status: NEW → ASSIGNED
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
Attachment #8944439 - Flags: review?(mak77) → review+
Comment on attachment 8944440 [details]
Bug 1131491 - Remove old synchronous Place's transactions.

https://reviewboard.mozilla.org/r/214642/#review220374
Attachment #8944440 - Flags: review?(mak77) → review+
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
Pushed by mbanner@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/772b0d449e35
https://hg.mozilla.org/mozilla-central/rev/9e0080c2930f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.