Change history2 to allow multi-deletion without entering a separate mode.

Change history2 to allow multi-deletion without entering a separate mode.

Eliminate "edit mode" altogether. When the mouse is over an entry, a
a checkbox appears to the left of the entry, and will remain visible as
long as it is checked. "Clear all" and "Remove selected" buttons are always
visible, and "Remove selected" is enabled whenever at least one checkbox
is checked.

This is only being done in history2 as it will soon replace chrome://history.

BUG=98549
TEST=Manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104277

[Patrick Dubroy, 2011-09-29 19:41:00]

arv: Can you please review this change to history2?
mkwst, jochen: FYI

Thanks,

Pat

[Patrick Dubroy, 2011-10-04 11:57:31]

arv: Ping.

Also +estade for review.

[arv (Not doing code reviews), 2011-10-04 23:26:30]

I like where this is going. Having different modes always felt ugly to me.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
File chrome/browser/resources/history2.html (right):

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.html:151: visibility: hidden;
also, :focused

Maybe use opacity instead and add a transition?

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.html:189: .fade-out-200 {
200?

class names should not describe the rendering

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
File chrome/browser/resources/history2.js (right):

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:121: checkbox.addEventListener("click",
checkboxClicked);
use single quotes

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:122: idToCheckbox[this.id_] = checkbox;
This doesn't seem necessary. Can you remove this map and use
document.querySelector instead?

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:140: entryBox["for"] = checkbox.id;
entryBox.htmlFor or entryBox.setAttribute

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:990: var callback = (i == 0) ?
reloadHistory : null;
useless parens

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:1012: var id =
parseInt(this.id.slice("checkbox-".length));
var id = Number(...);

or parseInt(..., 10)

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:1087: deleteQueue[0].callback.apply();
I assume this is so that 'this' is undefined?

[Patrick Dubroy, 2011-10-05 12:47:45]

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
File chrome/browser/resources/history2.html (right):

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.html:151: visibility: hidden;
On 2011/10/04 23:26:30, arv wrote:
> also, :focused

Good point. Done.

> Maybe use opacity instead and add a transition?

Good idea. I added a fade out transition, but didn't add a fade in transition,
because I think it feels to laggy that way.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.html:189: .fade-out-200 {
On 2011/10/04 23:26:30, arv wrote:
> 200?
> 
> class names should not describe the rendering

Done. I did that because the JS code explicitly relies on the transition being
<= 200ms, because it uses setTimeout after adding the class.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
File chrome/browser/resources/history2.js (right):

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:121: checkbox.addEventListener("click",
checkboxClicked);
On 2011/10/04 23:26:30, arv wrote:
> use single quotes

Done.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:122: idToCheckbox[this.id_] = checkbox;
On 2011/10/04 23:26:30, arv wrote:
> This doesn't seem necessary. Can you remove this map and use
> document.querySelector instead?

Done.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:140: entryBox["for"] = checkbox.id;
On 2011/10/04 23:26:30, arv wrote:
> entryBox.htmlFor or entryBox.setAttribute

Done.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:990: var callback = (i == 0) ?
reloadHistory : null;
On 2011/10/04 23:26:30, arv wrote:
> useless parens

Done.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:1012: var id =
parseInt(this.id.slice("checkbox-".length));
On 2011/10/04 23:26:30, arv wrote:
> var id = Number(...);
> 
> or parseInt(..., 10)

Done.

http://codereview.chromium.org/8079010/diff/1/chrome/browser/resources/histor...
chrome/browser/resources/history2.js:1087: deleteQueue[0].callback.apply();
On 2011/10/04 23:26:30, arv wrote:
> I assume this is so that 'this' is undefined?

Yes, that was the idea. Not sure that it's necessary, but I thought it would be
better for 'this' to be undefined, rather than some random object.

[arv (Not doing code reviews), 2011-10-05 21:30:08]

lgtm

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
File chrome/browser/resources/history2.html (right):

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.html:153: -webkit-transition: opacity 100ms;
the standard transition time in chrome is 150ms as far as I remember

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.html:155: .entry-box:hover
input[type=checkbox], .entry-box input[type=checkbox]:focus {
line break after comma

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.html:157: }
there should be an empty line between every rule

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
File chrome/browser/resources/history2.js (right):

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.js:1047: setTimeout(function() {
This can be done using the webkitTransitionEnd event so that the time does not
need to be hard coded.

[Patrick Dubroy, 2011-10-06 09:38:45]

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
File chrome/browser/resources/history2.html (right):

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.html:153: -webkit-transition: opacity 100ms;
On 2011/10/05 21:30:08, arv wrote:
> the standard transition time in chrome is 150ms as far as I remember

Done.

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.html:155: .entry-box:hover
input[type=checkbox], .entry-box input[type=checkbox]:focus {
On 2011/10/05 21:30:08, arv wrote:
> line break after comma

Done.

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.html:157: }
On 2011/10/05 21:30:08, arv wrote:
> there should be an empty line between every rule

Done.

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
File chrome/browser/resources/history2.js (right):

http://codereview.chromium.org/8079010/diff/6001/chrome/browser/resources/his...
chrome/browser/resources/history2.js:1047: setTimeout(function() {
On 2011/10/05 21:30:08, arv wrote:
> This can be done using the webkitTransitionEnd event so that the time does not
> need to be hard coded.

Excellent! Didn't know about that.

[commit-bot: I haz the power, 2011-10-06 09:39:25]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/8079010/11001

[commit-bot: I haz the power, 2011-10-06 11:00:32]

Change committed as 104277