Remove a deprecated RemovePages call from WindowsJumpLists.jsm

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: tiago, Mentored)

Tracking

({good-first-bug})

55 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
http://searchfox.org/mozilla-central/source/browser/modules/WindowsJumpLists.jsm#464

Should be replaced with the new PlacesUtils.history.remove API that is promise based, in this case we should add Cu to the top and .catch(Cu.reportError).
Hi

I want to dive into it.Any documentation for the PlacesUtils.history.remove API, to implement it in WindowsJumpLists.jsm#464
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #1)
> I want to dive into it.Any documentation for the PlacesUtils.history.remove
> API, to implement it in WindowsJumpLists.jsm#464

PlacesUtils.history is in History,jsm, specifically:

https://dxr.mozilla.org/mozilla-central/rev/96b36c5f527dd42e680a230839519eee1fc2c9f3/toolkit/components/places/History.jsm#255
(Reporter)

Updated

2 years ago
Priority: -- → P3
(Assignee)

Comment 3

2 years ago
Hi!

Hemant, are you still interested in this bug? I'm happy to take it if you are busy.
Flags: needinfo?(hemantsingh1612)
Sure, due to exams I couldn't look into it.Happy you are taking it.
Flags: needinfo?(hemantsingh1612)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8873941 [details]
Bug 1364337 - Remove a deprecated RemovePages call from WindowsJumpLists.jsm.

https://reviewboard.mozilla.org/r/145318/#review149294

::: browser/modules/WindowsJumpLists.jsm:464
(Diff revision 1)
>            let uriSpec = oldItem.app.getParameter(0);
>            URIsToRemove.push(NetUtil.newURI(uriSpec));
>          } catch (err) { }
>        }
>      }
>      if (URIsToRemove.length > 0) {

This check is also made inside the remove(), but there, if length == 0, it throws an error. Should I remove this if ?
(Reporter)

Updated

2 years ago
Assignee: nobody → tiago.paez11
Status: NEW → ASSIGNED
(Reporter)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8873941 [details]
Bug 1364337 - Remove a deprecated RemovePages call from WindowsJumpLists.jsm.

https://reviewboard.mozilla.org/r/145320/#review149690

::: browser/modules/WindowsJumpLists.jsm:6
(Diff revision 1)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +Components.utils.import("resource://gre/modules/PlacesUtils.jsm");

Please use XPCOMUtils.defineLazyModuleGetter, we don't want to pay the price until it's needed.

::: browser/modules/WindowsJumpLists.jsm:464
(Diff revision 1)
>            let uriSpec = oldItem.app.getParameter(0);
>            URIsToRemove.push(NetUtil.newURI(uriSpec));
>          } catch (err) { }
>        }
>      }
>      if (URIsToRemove.length > 0) {

keeping the if is fine, remove() throws if the array is empty because it assumes the consumer did some mistake trying to remove an empty array.

::: browser/modules/WindowsJumpLists.jsm:465
(Diff revision 1)
>            URIsToRemove.push(NetUtil.newURI(uriSpec));
>          } catch (err) { }
>        }
>      }
>      if (URIsToRemove.length > 0) {
> -      PlacesUtils.bhistory.removePages(URIsToRemove, URIsToRemove.length, true);
> +      PlacesUtils.history.remove(URIsToRemove).catch(Cu.reportError);

Cu.reportError won't work because you didn't define Cu at the top of the file.
While there you could probably replace the current code with:

const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
Attachment #8873941 - Flags: review?(mak77) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8873941 [details]
Bug 1364337 - Remove a deprecated RemovePages call from WindowsJumpLists.jsm.

https://reviewboard.mozilla.org/r/145320/#review150048

r=me with a couple fixes. once you'll attach an updated patch I'll push it to Try to quickly check tests are fine (I'm not sure whether this is tested honestly, but it won't hurt)

::: browser/modules/WindowsJumpLists.jsm:53
(Diff revision 2)
> -});
> -
>  XPCOMUtils.defineLazyGetter(this, "NetUtil", function() {
> -  Components.utils.import("resource://gre/modules/NetUtil.jsm");
> +  Cu.import("resource://gre/modules/NetUtil.jsm");
>    return NetUtil;
>  });

please, while here remove this "NetUtil" getter completely, we'll fix the only consumer down here...

::: browser/modules/WindowsJumpLists.jsm:452
(Diff revision 2)
>      while (e.hasMoreElements()) {
>        let oldItem = e.getNext().QueryInterface(Ci.nsIJumpListShortcut);
>        if (oldItem) {
>          try { // in case we get a bad uri
>            let uriSpec = oldItem.app.getParameter(0);
>            URIsToRemove.push(NetUtil.newURI(uriSpec));

Just use Services.io.newURI here instead of NetUtil.newURI
Attachment #8873941 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 11

2 years ago
there's a small whitespace nit, I'll fix it before pushing.
Comment hidden (mozreview-request)
(Reporter)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8875401 [details]
Bug 1364337 - Remove a deprecated RemovePages call from WindowsJumpLists.jsm.

https://reviewboard.mozilla.org/r/146844/#review150914
Attachment #8875401 - Flags: review?(mak77) → review+
(Reporter)

Updated

2 years ago
Attachment #8873941 - Attachment is obsolete: true

Comment 14

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/9721edbfbba3
Remove a deprecated RemovePages call from WindowsJumpLists.jsm. r=mak

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9721edbfbba3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.