Closed Bug 1364337 Opened 7 years ago Closed 7 years ago

Remove a deprecated RemovePages call from WindowsJumpLists.jsm

Categories

(Toolkit :: Places, enhancement, P3)

Product:
Toolkit
Component:
Places
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: tiago, Mentored)

References

Details

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

Attachments

(1 file, 1 obsolete file)

Bug 1364337 - Remove a deprecated RemovePages call from WindowsJumpLists.jsm.
59 bytes, text/x-review-board-request
mak
: review+
Details 
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
Priority: -- → P3
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 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 ?
Assignee: nobody → tiago.paez11
Status: NEW → ASSIGNED
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 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+
there's a small whitespace nit, I'll fix it before pushing.
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+
Attachment #8873941 - Attachment is obsolete: true
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/9721edbfbba3
Remove a deprecated RemovePages call from WindowsJumpLists.jsm. r=mak
https://hg.mozilla.org/mozilla-central/rev/9721edbfbba3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: