Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12217015:
History: Fix selecting multiple visits. (Closed)
|
Created:
7 years, 10 months ago by Sergiu Modified:
7 years, 10 months ago Reviewers:
Patrick Dubroy CC:
chromium-reviews, arv (Not doing code reviews), pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHistory: Fix selecting multiple visits.
Initially the visist IDs for the checkboxes were set as the visits were
retrieved but currently the visits can be displayed in a different order.
Fix this by setting the ID when the visit is displayed, fixing multiple
selection. Add a test to check that in the future.
BUG=170690
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182470
Patch Set 1 #
Total comments: 2
Patch Set 2 : A test and minor fix. #
Total comments: 10
Patch Set 3 : Refactor #
Total comments: 6
Patch Set 4 : Rebase #Patch Set 5 : More updates #Patch Set 6 : Rebase #
Messages
Total messages: 14 (0 generated)
Even if we decide to just iterate through the visits I'd like to have this fixed first so please take a look.
Looks fine, but can you please add a test, at least for the regular history page
(i.e., without the grouped flag)?
You can simulate a shift click like this:
var e = new MouseEvent('click', { shiftKey:true });
$('checkbox-4').dispatchEvent(e);
https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist...
File chrome/browser/resources/history/history.js (right):
https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist...
chrome/browser/resources/history/history.js:49: this.id_ = 0;
Should this maybe be -1? 0 is a valid id -- it will be the id of the first visit
displayed. I think everything will still work correctly but it could be a bit
confusing.
Also, can you change the first line of the description to begin with "History:"?
On 2013/02/05 18:01:00, dubroy wrote:
> Looks fine, but can you please add a test, at least for the regular history
page
> (i.e., without the grouped flag)?
>
> You can simulate a shift click like this:
>
> var e = new MouseEvent('click', { shiftKey:true });
> $('checkbox-4').dispatchEvent(e);
>
>
https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist...
> File chrome/browser/resources/history/history.js (right):
>
>
https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist...
> chrome/browser/resources/history/history.js:49: this.id_ = 0;
> Should this maybe be -1? 0 is a valid id -- it will be the id of the first
visit
> displayed. I think everything will still work correctly but it could be a bit
> confusing.
Added a test as well, take a look. https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist... chrome/browser/resources/history/history.js:49: this.id_ = 0; On 2013/02/05 18:01:00, dubroy wrote: > Should this maybe be -1? 0 is a valid id -- it will be the id of the first visit > displayed. I think everything will still work correctly but it could be a bit > confusing. It makes sense to set it to -1 to show that it has not been set, maybe in the future it will help debugging.
lgtm, but please change the CL description to begin with "History:", and consider adding the extra test that I mentioned. On 2013/02/05 20:16:26, Sergiu wrote: > Added a test as well, take a look. > > https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist... > File chrome/browser/resources/history/history.js (right): > > https://codereview.chromium.org/12217015/diff/1/chrome/browser/resources/hist... > chrome/browser/resources/history/history.js:49: this.id_ = 0; > On 2013/02/05 18:01:00, dubroy wrote: > > Should this maybe be -1? 0 is a valid id -- it will be the id of the first > visit > > displayed. I think everything will still work correctly but it could be a bit > > confusing. > > It makes sense to set it to -1 to show that it has not been set, maybe in the > future it will help debugging.
https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... File chrome/test/data/webui/history_browsertest.js (right): https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:329: expectFalse(checkboxes[i].checked); You could also do this with: expectEqual(document.querySelectorAll('input[type=checkbox]:checked'), 0) https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:331: var getNewEvent = function() { Nit: maybe name this getClickEvent? https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:342: if (i >= 4 && i <= 9) Again, you could use querySelectorAll('input[type=checkbox]:checked') and just check that the first element is #checkbox-4 and the last one is #checkbox-9. I think it would be cleaner than looping, but I'll leave it up to you. https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:351: for (var i = 0; i < checkboxes.length; i++) { Same here. https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:369: One more test I would add: $('checkbox-26').click(); $('checkbox-20').dispatchEvent(getNewEvent()); and test that 19 is still checked, and 20-26 are unchecked.
I've refactored stuff around, see what you think. https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... File chrome/test/data/webui/history_browsertest.js (right): https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:329: expectFalse(checkboxes[i].checked); On 2013/02/05 20:43:14, dubroy wrote: > You could also do this with: > > expectEqual(document.querySelectorAll('input[type=checkbox]:checked'), 0) Done, expectFalse doesn't actually work as it seems to do some type checking as well. https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:331: var getNewEvent = function() { On 2013/02/05 20:43:14, dubroy wrote: > Nit: maybe name this getClickEvent? Made it getShiftClickEvent to make it clearer. https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:342: if (i >= 4 && i <= 9) On 2013/02/05 20:43:14, dubroy wrote: > Again, you could use querySelectorAll('input[type=checkbox]:checked') and just > check that the first element is #checkbox-4 and the last one is #checkbox-9. I > think it would be cleaner than looping, but I'll leave it up to you. Just for thoroughness I would like to check the id of every checked item, maybe it will catch some subtle stuff later on. But the idea of looking only at the checked boxes is good, so I've done that. https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:351: for (var i = 0; i < checkboxes.length; i++) { On 2013/02/05 20:43:14, dubroy wrote: > Same here. Done. https://codereview.chromium.org/12217015/diff/4004/chrome/test/data/webui/his... chrome/test/data/webui/history_browsertest.js:369: On 2013/02/05 20:43:14, dubroy wrote: > One more test I would add: > > $('checkbox-26').click(); > $('checkbox-20').dispatchEvent(getNewEvent()); > > and test that 19 is still checked, and 20-26 are unchecked. Done.
https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... File chrome/test/data/webui/history_browsertest.js (right): https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:346: '#results-display input[type=checkbox]:checked').length); All of the duplicated querySelectorAll stuff clutters things up. I'd pull it out into a separate function like getCheckedCheckboxes() or something. It would make it a lot easier to see the actual test logic. https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:353: $('checkbox-4').dispatchEvent(getShiftClickEvent()); Maybe factor the dispatchEvent out too? Something like: function shiftClick(el) { el.dispatchEvent(new MouseEvent('click', { shiftKey: true }); } https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:382: checkInterval(checked, 19, 24, 11); It might be more clear if you eliminated the last parameter from all these calls, and instead used Array.slice(), e.g.: checkInterval(checked.slice(11), 19, 24)
https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... File chrome/test/data/webui/history_browsertest.js (right): https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:346: '#results-display input[type=checkbox]:checked').length); On 2013/02/06 18:53:07, dubroy wrote: > All of the duplicated querySelectorAll stuff clutters things up. I'd pull it out > into a separate function like getCheckedCheckboxes() or something. It would make > it a lot easier to see the actual test logic. Done. https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:353: $('checkbox-4').dispatchEvent(getShiftClickEvent()); On 2013/02/06 18:53:07, dubroy wrote: > Maybe factor the dispatchEvent out too? Something like: > > function shiftClick(el) { > el.dispatchEvent(new MouseEvent('click', { shiftKey: true }); > } Done. https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:382: checkInterval(checked, 19, 24, 11); On 2013/02/06 18:53:07, dubroy wrote: > It might be more clear if you eliminated the last parameter from all these > calls, and instead used Array.slice(), e.g.: > > checkInterval(checked.slice(11), 19, 24) Heh, actually it seems like querySelectorAll returns a NodeList and not an Array so slice doesn't work there. But Array.prototype.slice.call(list) transforms it into one so I did that.
https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... File chrome/test/data/webui/history_browsertest.js (right): https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:346: '#results-display input[type=checkbox]:checked').length); On 2013/02/06 18:53:07, dubroy wrote: > All of the duplicated querySelectorAll stuff clutters things up. I'd pull it out > into a separate function like getCheckedCheckboxes() or something. It would make > it a lot easier to see the actual test logic. Done. https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:353: $('checkbox-4').dispatchEvent(getShiftClickEvent()); On 2013/02/06 18:53:07, dubroy wrote: > Maybe factor the dispatchEvent out too? Something like: > > function shiftClick(el) { > el.dispatchEvent(new MouseEvent('click', { shiftKey: true }); > } Done. https://codereview.chromium.org/12217015/diff/13001/chrome/test/data/webui/hi... chrome/test/data/webui/history_browsertest.js:382: checkInterval(checked, 19, 24, 11); On 2013/02/06 18:53:07, dubroy wrote: > It might be more clear if you eliminated the last parameter from all these > calls, and instead used Array.slice(), e.g.: > > checkInterval(checked.slice(11), 19, 24) Heh, actually it seems like querySelectorAll returns a NodeList and not an Array so slice doesn't work there. But Array.prototype.slice.call(list) transforms it into one so I did that.
Ping on the latest changes on this one, I'll land if it's ok.
LGTM. BTW, if I give an LGTM with comments, it means that I'm fine with you landing after you've made the suggested changes. If you'd like me to comment on new patch sets, it's best to say so, otherwise I'll assume you're not waiting for me.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/12217015/17001
Message was sent while issue was closed.
Change committed as 182470 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
