|
|
DescriptionDownloads: Add an alt + c key to clear all downloads on chrome://downloads.
Committed: https://crrev.com/cd82b473e9485d401a6c6651c51ece3c795ab188
Cr-Commit-Position: refs/heads/master@{#295382}
Patch Set 1 #Patch Set 2 : with comment #
Total comments: 13
Patch Set 3 : address dbeam comments #
Total comments: 4
Patch Set 4 : nit #Patch Set 5 : keyEvt #Patch Set 6 : moar nits #Messages
Total messages: 19 (4 generated)
thestig@chromium.org changed reviewers: + asanka@chromium.org, dbeam@chromium.org
Code lgtm, but I'm wondering if the problem being address could be solved by adding something along the lines of "Don't keep history." Have you looked at the UMA for how often 'Clear all' is invoked? Also does Alt+C conflict with a common shortcut on our platforms? Do we need a nanny prompt to intervene before destroying all of history based on a hotkey press?
> Do we need a nanny prompt to intervene before destroying all of history based on > a hotkey press? history has this before mass delete of history entries. i don't really think it matters much, but might be worth it... https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:112: window.addEventListener('keydown', this.onKeyDown, true); why are you doing this while capturing (why true as last arg)? https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:274: * @param {KeyboardEvent} evt The keyboard event. callbacks given to addEventListener() only take Event rather than {Mouse,Key,...}Event @param {Event} evt The keyboard event. @private https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:276: Downloads.prototype.onKeyDown = function(evt) { onKeyDown_ https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:276: Downloads.prototype.onKeyDown = function(evt) { var keyEvt = /** @type {KeyboardEvent} */(evt); https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:277: if (evt.keyCode == 67 && evt.altKey) { // alt + c nit: end comment with . https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:280: return; why return?
On 2014/09/17 19:20:33, asanka wrote: > Code lgtm, but I'm wondering if the problem being address could be solved by > adding something along the lines of "Don't keep history." Have you looked at the > UMA for how often 'Clear all' is invoked? I'm not so much concerned about the privacy/history policy. > Also does Alt+C conflict with a common shortcut on our platforms? Nope. On 2014/09/17 20:48:59, Dan Beam wrote: > > Do we need a nanny prompt to intervene before destroying all of history based > on > > a hotkey press? > > history has this before mass delete of history entries. i don't really think it > matters much, but might be worth it... The clear downloads button in FF never prompts either. It used to have the alt+c shortcut, which I liked, but FF removed it in recent builds. As a keyboard user, I really like to be able to do it without touching the mouse.
https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:112: window.addEventListener('keydown', this.onKeyDown, true); On 2014/09/17 20:48:58, Dan Beam wrote: > why are you doing this while capturing (why true as last arg)? *shrug* I copy+pasted from chrome/browser/resources/chromeos/chromevox/chromevox/injected/ui/widget.js https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:274: * @param {KeyboardEvent} evt The keyboard event. On 2014/09/17 20:48:58, Dan Beam wrote: > callbacks given to addEventListener() only take Event rather than > {Mouse,Key,...}Event > > @param {Event} evt The keyboard event. > @private Done. (Also copy+pasted FYI) https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:276: Downloads.prototype.onKeyDown = function(evt) { On 2014/09/17 20:48:58, Dan Beam wrote: > var keyEvt = /** @type {KeyboardEvent} */(evt); Done. https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:276: Downloads.prototype.onKeyDown = function(evt) { On 2014/09/17 20:48:58, Dan Beam wrote: > onKeyDown_ Done. https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:277: if (evt.keyCode == 67 && evt.altKey) { // alt + c On 2014/09/17 20:48:58, Dan Beam wrote: > nit: end comment with . Done. https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:280: return; On 2014/09/17 20:48:58, Dan Beam wrote: > why return? In case we add more keys later?
https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:280: return; On 2014/09/17 21:31:15, Lei Zhang wrote: > On 2014/09/17 20:48:58, Dan Beam wrote: > > why return? > > In case we add more keys later? meh. if you *really* want to put a return in this function for some reason, I'd do: if (!evt.altKey || evt.keyCode != 67) return; clearAll(); evt.preventDefault();
On 2014/09/17 21:39:29, Dan Beam wrote: > https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... > File chrome/browser/resources/downloads/downloads.js (right): > > https://codereview.chromium.org/579583005/diff/20001/chrome/browser/resources... > chrome/browser/resources/downloads/downloads.js:280: return; > On 2014/09/17 21:31:15, Lei Zhang wrote: > > On 2014/09/17 20:48:58, Dan Beam wrote: > > > why return? > > > > In case we add more keys later? > > meh. if you *really* want to put a return in this function for some reason, I'd > do: > > if (!evt.altKey || evt.keyCode != 67) > return; > > clearAll(); > evt.preventDefault(); Patch set 4 removes the return; "No and then"
https://codereview.chromium.org/579583005/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/579583005/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:112: window.addEventListener('keydown', this.onKeyDown_); nit: onKeyDown_.bind(this) is a better "in case" because it's likely that somebody tries to access a this.* member in onKeyDown_ but |this| is changed to the target in event listeners. https://codereview.chromium.org/579583005/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:279: if (evt.keyCode == 67 && evt.altKey) { // alt + c. s/evt/keyEvt/
lgtm w/nits
https://codereview.chromium.org/579583005/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/579583005/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:112: window.addEventListener('keydown', this.onKeyDown_); On 2014/09/17 21:41:41, Dan Beam wrote: > nit: onKeyDown_.bind(this) is a better "in case" because it's likely that > somebody tries to access a this.* member in onKeyDown_ but |this| is changed to > the target in event listeners. Done. https://codereview.chromium.org/579583005/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:279: if (evt.keyCode == 67 && evt.altKey) { // alt + c. On 2014/09/17 21:41:41, Dan Beam wrote: > s/evt/keyEvt/ Done.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/579583005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/579583005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 6d34441eb850d01f7291f63ed143b4788038ef3f
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cd82b473e9485d401a6c6651c51ece3c795ab188 Cr-Commit-Position: refs/heads/master@{#295382} |