"ctrl + double click" acts on the highlighted/selected bookmark list items irrespective of on which…

Handles selection on dblclick on any bookmark list item

"ctrl + double click" acts on the highlighted/selected bookmark list items
irrespective of on which list item the action is performed. To extend this to
clicked, unhighlighted list item, invoking mouseup event handling functionality
under doubleclick event handling before command execution. mouseup event takes
care of highlighting and unhighlighting list items.

BUG=243221
Committed: https://crrev.com/1d406f488b1f66f6779816459254a007bb652382
Cr-Commit-Position: refs/heads/master@{#295021}

[b.rout, 2014-08-28 08:50:36]

b.rout@samsung.com changed reviewers:
+ pam@chromium.org, yosin@chromium.org

[yosin_UTC9, 2014-09-01 01:04:54]

yosin@chromium.org changed reviewers:
+ arv@chromium.org

[yosin_UTC9, 2014-09-01 01:04:55]

LGTM

+arv@ for OWNERS review

https://codereview.chromium.org/515853002/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/515853002/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:244: var index = -1;
nit: "?" makes shorter:
var index = target ? list.getIndexOfListItem(target) : -1

[b.rout, 2014-09-01 06:40:34]

On 2014/09/01 01:04:55, Yosi_UTC9 wrote:
> LGTM
> 
> +arv@ for OWNERS review
> 
>
https://codereview.chromium.org/515853002/diff/1/chrome/browser/resources/boo...
> File chrome/browser/resources/bookmark_manager/js/main.js (right):
> 
>
https://codereview.chromium.org/515853002/diff/1/chrome/browser/resources/boo...
> chrome/browser/resources/bookmark_manager/js/main.js:244: var index = -1;
> nit: "?" makes shorter:
> var index = target ? list.getIndexOfListItem(target) : -1

Thanks Yosi.
I have implemented the review comment and uploaded the 2nd patch.

@arv,
Please take a look.

[b.rout, 2014-09-02 05:51:06]

The CQ bit was checked by b.rout@samsung.com

[commit-bot: I haz the power, 2014-09-02 05:51:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.rout@samsung.com/515853002/40001

[b.rout, 2014-09-02 06:10:52]

The CQ bit was unchecked by b.rout@samsung.com

[b.rout, 2014-09-02 06:31:23]

My bad. I checked the commit bit without OWNERS review.

@pam and @arv,
Could you please take a look at it.

[arv (Not doing code reviews), 2014-09-02 17:17:33]

Thanks for looking into this.

Can you clean up the CL description a bit. The first line should be a short
summary what the change does:

"
cr.ui.List: Handle selection on dblclick

Longer description goes here, limited to 80 columns
"

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:248: if (!indexSelected)
index 0 is also a valid index.

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:249:
list.handlePointerDownUp_(e);
This does not seem like the correct fix.

The list itself handles selection and we want the selection to be updated on
dblclick. My suggestion is to add a dblclick listener in cr/ui/list.js instead.

[b.rout, 2014-09-03 09:01:13]

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:248: if (!indexSelected)
On 2014/09/02 17:17:32, arv wrote:
> index 0 is also a valid index.

right, index 0 is a valid index. But, getIndexSelected will tell if the index is
selected, which is captured in indexSelected variable. Sorry, I am just
wondering what is wrong here.

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:249:
list.handlePointerDownUp_(e);
On 2014/09/02 17:17:32, arv wrote:
> This does not seem like the correct fix.
> 
> The list itself handles selection and we want the selection to be updated on
> dblclick. My suggestion is to add a dblclick listener in cr/ui/list.js
instead.

There is already a dblclick listener in the cr/ui/list.js. But, I think list.js
covers common event handling functionalities. And the list in bookmark manager
is specific to bookmark. I think there might be other users of list. So, I am
doubtful if this change could be applied to common event handling fucntionality
of the list. Need a bit of insight and your advice.

[arv (Not doing code reviews), 2014-09-04 15:19:21]

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:248: if (!indexSelected)
On 2014/09/03 09:01:13, b.rout wrote:
> On 2014/09/02 17:17:32, arv wrote:
> > index 0 is also a valid index.
> 
> right, index 0 is a valid index. But, getIndexSelected will tell if the index
is
> selected, which is captured in indexSelected variable. Sorry, I am just
> wondering what is wrong here.

You are right. I was not reading this thoroughly.

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:249:
list.handlePointerDownUp_(e);
On 2014/09/03 09:01:13, b.rout wrote:
> On 2014/09/02 17:17:32, arv wrote:
> > This does not seem like the correct fix.
> > 
> > The list itself handles selection and we want the selection to be updated on
> > dblclick. My suggestion is to add a dblclick listener in cr/ui/list.js
> instead.
> 
> There is already a dblclick listener in the cr/ui/list.js. But, I think
list.js
> covers common event handling functionalities. And the list in bookmark manager
> is specific to bookmark. I think there might be other users of list. So, I am
> doubtful if this change could be applied to common event handling
fucntionality
> of the list. Need a bit of insight and your advice.

I would prefer that this is handled in list. A Ctrl+dblclick should update the
selection in list, which happens before this dblclick handler is called.

[b.rout, 2014-09-05 09:16:54]

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/515853002/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:249:
list.handlePointerDownUp_(e);
On 2014/09/04 15:19:20, arv wrote:
> On 2014/09/03 09:01:13, b.rout wrote:
> > On 2014/09/02 17:17:32, arv wrote:
> > > This does not seem like the correct fix.
> > > 
> > > The list itself handles selection and we want the selection to be updated
on
> > > dblclick. My suggestion is to add a dblclick listener in cr/ui/list.js
> > instead.
> > 
> > There is already a dblclick listener in the cr/ui/list.js. But, I think
> list.js
> > covers common event handling functionalities. And the list in bookmark
manager
> > is specific to bookmark. I think there might be other users of list. So, I
am
> > doubtful if this change could be applied to common event handling
> fucntionality
> > of the list. Need a bit of insight and your advice.
> 
> I would prefer that this is handled in list. A Ctrl+dblclick should update the
> selection in list, which happens before this dblclick handler is called.

Done.

[b.rout, 2014-09-05 09:19:04]

@arv,

I have incorporated the review comment and uploaded the 3rd patch.
Please have a look.

Thanks.

[arv (Not doing code reviews), 2014-09-15 21:35:37]

LGTM

[b.rout, 2014-09-16 05:29:27]

The CQ bit was checked by b.rout@samsung.com

[commit-bot: I haz the power, 2014-09-16 05:30:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/515853002/60001

[commit-bot: I haz the power, 2014-09-16 06:34:08]

Committed patchset #3 (id:60001) as 338adc69ba365a1806eece935277d7fec483f5fc

[commit-bot: I haz the power, 2014-09-16 06:35:29]

Patchset 3 (id:??) landed as
https://crrev.com/1d406f488b1f66f6779816459254a007bb652382
Cr-Commit-Position: refs/heads/master@{#295021}