Swallow double-click events on menu buttons in Web UI in chrome://bookmarks.

Swallow double-click events on menu buttons in Web UI in chrome://bookmarks.

'dblclick' event is getting propagated to the 'bmm.list' and selected bookmark is getting opened in the newtab.

Now Swallowing double-click events on menu buttons in Web-UI and stopping its propagation to bookmark manager list.

BUG=437695
Committed: https://crrev.com/a84191ca8ce935421eba6c13cca3cd45d01e8bd4
Cr-Commit-Position: refs/heads/master@{#326265}

[Deepak, 2015-04-21 13:47:46]

deepak.m1@samsung.com changed reviewers:
+ bauerb@chromium.org

[Deepak, 2015-04-21 13:47:46]

PTAL
Thanks

[Bernhard Bauer, 2015-04-21 15:54:47]

BTW, in general it would be very helpful if you changed your CL description to
state what you are changing instead of the bug (which is linked anyway).

https://codereview.chromium.org/1092933004/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1092933004/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:158: case 'dblclick':
Can you add a comment like "Don't allow double click events to propagate"?

[Deepak, 2015-04-22 04:14:25]

Thanks for review.
Comment added and issue description updated as suggested.
PTAL

https://codereview.chromium.org/1092933004/diff/20001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1092933004/diff/20001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:158: case 'dblclick':
On 2015/04/21 15:54:47, Bernhard Bauer wrote:
> Can you add a comment like "Don't allow double click events to propagate"?

Done.

[Bernhard Bauer, 2015-04-22 07:54:52]

On 2015/04/22 04:14:25, Deepak wrote:
> Thanks for review.
> Comment added and issue description updated as suggested.

The issue title / first paragraph of the issue description is still the same,
and that is what is going to be most important: for example, when looking
through a long list of changes, you want to immediately see what the changes
are. The bug that is fixed is not as interesting there. In addition, stating the
bug could be interpreted as the behavior _after_ your change, which is exactly
backwards.

> PTAL
> 
>
https://codereview.chromium.org/1092933004/diff/20001/ui/webui/resources/js/c...
> File ui/webui/resources/js/cr/ui/menu_button.js (right):
> 
>
https://codereview.chromium.org/1092933004/diff/20001/ui/webui/resources/js/c...
> ui/webui/resources/js/cr/ui/menu_button.js:158: case 'dblclick':
> On 2015/04/21 15:54:47, Bernhard Bauer wrote:
> > Can you add a comment like "Don't allow double click events to propagate"?
> 
> Done.

[Deepak, 2015-04-22 08:04:55]

I understand, Description should tell about fixed issue.
I have changed description as:
"Selected Item should not open in newTab on double clicking dropdown menu button
in bookmark manager."

Please suggest if still it is not proper.
Thanks

[Bernhard Bauer, 2015-04-22 08:11:36]

On 2015/04/22 08:04:55, Deepak wrote:
> I understand, Description should tell about fixed issue.
> I have changed description as:
> "Selected Item should not open in newTab on double clicking dropdown menu
button
> in bookmark manager."
> 
> Please suggest if still it is not proper.
> Thanks

Not quite, you're just adding "should not" :) Just very briefly describe the
changes that you make: "Swallow double-click events on menu buttons in Web UI",
for example.

[Deepak, 2015-04-22 08:21:23]

Thanks for suggestion.
Tried to improve title as suggested. Is this fine :)

[Bernhard Bauer, 2015-04-22 08:36:31]

Yes :) LGTM

[Deepak, 2015-04-22 08:37:10]

On 2015/04/22 08:36:31, Bernhard Bauer wrote:
> Yes :) LGTM

Thanks.

[Deepak, 2015-04-22 08:37:15]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-04-22 08:37:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1092933004/40001

[commit-bot: I haz the power, 2015-04-22 08:42:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-22 08:42:18]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Deepak, 2015-04-22 08:49:27]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-04-22 08:50:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1092933004/40001

[commit-bot: I haz the power, 2015-04-22 08:53:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-22 08:53:49]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Deepak, 2015-04-22 09:32:24]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-04-22 09:32:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1092933004/#ps60001
(title: "Resolving merge conflict.")

[commit-bot: I haz the power, 2015-04-22 09:32:43]

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

[commit-bot: I haz the power, 2015-04-22 10:54:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-22 10:55:47]

Patchset 4 (id:??) landed as
https://crrev.com/a84191ca8ce935421eba6c13cca3cd45d01e8bd4
Cr-Commit-Position: refs/heads/master@{#326265}