Fixed to return original context element.

Fixed to return original context element.

BUG=423248
TEST=out/Release/browser_tests --gtest_filter=WebUIResourceBrowserTest.ContextMenuHandlerTest

Committed: https://crrev.com/4830c50b38bcb3b5c2144b7d85fb34ab7f985270
Cr-Commit-Position: refs/heads/master@{#327004}

[yawano, 2014-10-14 08:01:02]

yawano@chromium.org changed reviewers:
+ dbeam@chromium.org

[yawano, 2014-10-14 08:01:03]

PTAL.

[Dan Beam, 2014-10-14 18:06:21]

lgtm, but you'd get more points if you wrote a test in the style of
chrome/test/data/webui/*_test.html (you'll have to add your test to
webui_resource_browsertest.cc as well)

[yawano, 2014-10-15 03:07:28]

Thank you for the review. I added a test case for this patch. PTAL.

[Dan Beam, 2014-10-15 03:09:41]

lgtm, yawano++

https://codereview.chromium.org/647483006/diff/60001/chrome/test/data/webui/c...
File chrome/test/data/webui/context_menu_handler_test.html (right):

https://codereview.chromium.org/647483006/diff/60001/chrome/test/data/webui/c...
chrome/test/data/webui/context_menu_handler_test.html:7: var cm =
cr.ui.contextMenuHandler;
nit: cmh

https://codereview.chromium.org/647483006/diff/60001/chrome/test/data/webui/c...
chrome/test/data/webui/context_menu_handler_test.html:9: // Create context menu
nit: end comments with .

[yawano, 2014-10-15 04:16:27]

Thanks for the review.

https://codereview.chromium.org/647483006/diff/60001/chrome/test/data/webui/c...
File chrome/test/data/webui/context_menu_handler_test.html (right):

https://codereview.chromium.org/647483006/diff/60001/chrome/test/data/webui/c...
chrome/test/data/webui/context_menu_handler_test.html:7: var cm =
cr.ui.contextMenuHandler;
On 2014/10/15 03:09:40, Dan Beam wrote:
> nit: cmh

Done.

https://codereview.chromium.org/647483006/diff/60001/chrome/test/data/webui/c...
chrome/test/data/webui/context_menu_handler_test.html:9: // Create context menu
On 2014/10/15 03:09:41, Dan Beam wrote:
> nit: end comments with .

Done.

[yawano, 2014-10-15 04:16:32]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-15 04:17:06]

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

[commit-bot: I haz the power, 2014-10-15 04:20:00]

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

[commit-bot: I haz the power, 2014-10-15 04:20:01]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yawano, 2014-10-15 06:03:16]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-15 06:03:52]

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

[commit-bot: I haz the power, 2014-10-15 06:06:29]

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

[commit-bot: I haz the power, 2014-10-15 06:06:30]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yawano, 2014-10-16 03:11:59]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-16 03:14:01]

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

[commit-bot: I haz the power, 2014-10-16 03:18:13]

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

[commit-bot: I haz the power, 2014-10-16 03:18:15]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/69247)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yawano, 2014-10-16 03:53:57]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-16 03:54:25]

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

[commit-bot: I haz the power, 2014-10-16 04:56:36]

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

[commit-bot: I haz the power, 2014-10-16 04:56:37]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[yawano, 2014-10-16 05:29:37]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-16 05:30:37]

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

[commit-bot: I haz the power, 2014-10-16 07:40:50]

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

[commit-bot: I haz the power, 2014-10-16 07:40:51]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yawano, 2014-10-17 01:34:46]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-17 01:36:11]

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

[commit-bot: I haz the power, 2014-10-17 03:32:51]

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

[commit-bot: I haz the power, 2014-10-17 03:32:52]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yawano, 2014-10-17 04:58:56]

The CQ bit was checked by yawano@chromium.org

[yawano, 2014-10-17 04:59:18]

The CQ bit was unchecked by yawano@chromium.org

[yawano, 2014-10-17 05:03:32]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-17 05:03:45]

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

[yawano, 2014-10-17 07:59:54]

The CQ bit was unchecked by yawano@chromium.org

[yawano, 2014-10-20 01:14:07]

On 2014/10/17 05:03:45, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/647483006/120001

Note: Test case fails on win_chromium_(x64_)rel_swarming. I created another
patch set to run tests on trybot in order to debug and fix the failed test
cases. https://codereview.chromium.org/645033007

[yawano, 2014-10-27 01:11:36]

The test case has failed on Windows since we had to wait at least 50ms after we
hide the context menu. I fixed the test case to pass the test. PTAL. Thanks.

Reference: context_menu_handler.js line:93
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[yawano, 2014-10-29 01:14:38]

@dbeam: Could you take a look at this CL since I changed the code of the test
case? Thanks!

On 2014/10/27 01:11:36, yawano wrote:
> The test case has failed on Windows since we had to wait at least 50ms after
we
> hide the context menu. I fixed the test case to pass the test. PTAL. Thanks.
> 
> Reference: context_menu_handler.js line:93
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[Dan Beam, 2014-10-29 01:17:17]

https://codereview.chromium.org/647483006/diff/180001/chrome/test/data/webui/...
File chrome/test/data/webui/context_menu_handler_test.html (right):

https://codereview.chromium.org/647483006/diff/180001/chrome/test/data/webui/...
chrome/test/data/webui/context_menu_handler_test.html:42: setTimeout(function()
{
this is almost never a good idea.  please make this callback-based (i.e. either
find a way to hook into existing code or write new code to ensure your
environment is set up correctly rather than waiting a hopefully correct amount
of time)

[yawano, 2014-10-30 09:21:10]

On 2014/10/29 01:17:17, Dan Beam wrote:
>
https://codereview.chromium.org/647483006/diff/180001/chrome/test/data/webui/...
> File chrome/test/data/webui/context_menu_handler_test.html (right):
> 
>
https://codereview.chromium.org/647483006/diff/180001/chrome/test/data/webui/...
> chrome/test/data/webui/context_menu_handler_test.html:42:
setTimeout(function()
> {
> this is almost never a good idea.  please make this callback-based (i.e.
either
> find a way to hook into existing code or write new code to ensure your
> environment is set up correctly rather than waiting a hopefully correct amount
> of time)

@dbeam: Thank you for the review! I think it's better not to use time-based
approach also in the implementation of context_menu_handler.js. I'm going to
investigate it and find a way to fix it.

[yawano, 2015-04-22 02:25:50]

Sorry for asking review of very old CL. I changed not to use setTimeout in test
code. PTAL. Thank you!

[Dan Beam, 2015-04-22 18:15:59]

lgtm

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
File chrome/test/data/webui/context_menu_handler_test.html (right):

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
chrome/test/data/webui/context_menu_handler_test.html:6: function
testShowAndHideEvents() {
var originalDateNow = Date.now;

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
chrome/test/data/webui/context_menu_handler_test.html:30: var callbacks = [];
nit: callbacks -> events

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
chrome/test/data/webui/context_menu_handler_test.html:55: assertEquals(menu,
callbacks[2].menu);
Date.now = originalDateNow;

[yawano, 2015-04-27 05:25:32]

Thank you!

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
File chrome/test/data/webui/context_menu_handler_test.html (right):

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
chrome/test/data/webui/context_menu_handler_test.html:6: function
testShowAndHideEvents() {
On 2015/04/22 18:15:59, Dan Beam wrote:
> var originalDateNow = Date.now;

Done.

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
chrome/test/data/webui/context_menu_handler_test.html:30: var callbacks = [];
On 2015/04/22 18:15:59, Dan Beam wrote:
> nit: callbacks -> events

Done.

https://codereview.chromium.org/647483006/diff/220001/chrome/test/data/webui/...
chrome/test/data/webui/context_menu_handler_test.html:55: assertEquals(menu,
callbacks[2].menu);
On 2015/04/22 18:15:59, Dan Beam wrote:
> Date.now = originalDateNow;

Done.

[yawano, 2015-04-27 05:25:48]

The CQ bit was checked by yawano@chromium.org

[yawano, 2015-04-27 05:25:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/647483006/#ps240001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-04-27 05:26:03]

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

[commit-bot: I haz the power, 2015-04-27 06:34:33]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-04-27 06:35:26]

Patchset 13 (id:??) landed as
https://crrev.com/4830c50b38bcb3b5c2144b7d85fb34ab7f985270
Cr-Commit-Position: refs/heads/master@{#327004}