Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296153008:
Testing contextmenu attribute which enables webpage to add custom menu items to the platform contex… (Closed)
DescriptionTesting contextmenu attribute which enables webpage to add custom menu items to the platform context menu
BUG=87553
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated test #
Total comments: 4
Patch Set 3 : Fixed comments #Patch Set 4 : Fixed comments #
Total comments: 10
Patch Set 5 : Done #
Messages
Total messages: 43 (15 generated)
Browser tests for custom context menu CL https://codereview.chromium.org/243403006/. PTAL.
https://chromiumcodereview.appspot.com/296153008/diff/1/chrome/browser/render... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://chromiumcodereview.appspot.com/296153008/diff/1/chrome/browser/render... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:181: menu_observer.setCommandId(47002); Probably use IDC_CONTENT_CONTEXT_CUSTOM_FIRST + 2? https://chromiumcodereview.appspot.com/296153008/diff/1/chrome/browser/render... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h (right): https://chromiumcodereview.appspot.com/296153008/diff/1/chrome/browser/render... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h:19: void setCommandId(int id) { command_to_execute_ = id; } SetCommandId, or OverrideCommandId https://chromiumcodereview.appspot.com/296153008/diff/1/chrome/test/data/cont... File chrome/test/data/context_menu/custom_context_menu.html (right): https://chromiumcodereview.appspot.com/296153008/diff/1/chrome/test/data/cont... chrome/test/data/context_menu/custom_context_menu.html:2: <div contextmenu="menu_id" style="height: 100px; width: 100px; background-color: lightgreen"> Add <html><head><title></title><body>...
I can't review things out in chrome/browser, you need to find someone in their OWNERS. https://codereview.chromium.org/296153008/diff/1/chrome/test/data/context_men... File chrome/test/data/context_menu/custom_context_menu.html (right): https://codereview.chromium.org/296153008/diff/1/chrome/test/data/context_men... chrome/test/data/context_menu/custom_context_menu.html:2: <div contextmenu="menu_id" style="height: 100px; width: 100px; background-color: lightgreen"> Those elements are not required, you don't need them.
sanjoy.pal@samsung.com changed reviewers: + tkent@chromium.org
As blink side changes have been committed, reviving this CL. Please review.
sanjoy.pal@samsung.com changed reviewers: + brettw@chromium.org - lazyboy@chromium.org, tkent@chromium.org
Please review.
https://codereview.chromium.org/296153008/diff/40001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/296153008/diff/40001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:57: void executeCustomMenuCommandAndVerifyTabTitle(unsigned command_id, The formatting of this is all messed up. Also: function names should be uppercase. https://codereview.chromium.org/296153008/diff/40001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:61: // Open context menu and execute |command_id| This context menu code is almost an exact copy of some code below. Is it reasonable to share this code somehow?
Updated the patch. Please take another look.
Done. PTAL. https://codereview.chromium.org/296153008/diff/40001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/296153008/diff/40001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:57: void executeCustomMenuCommandAndVerifyTabTitle(unsigned command_id, On 2014/09/02 23:50:29, brettw wrote: > The formatting of this is all messed up. Also: function names should be > uppercase. Done. https://codereview.chromium.org/296153008/diff/40001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:61: // Open context menu and execute |command_id| On 2014/09/02 23:50:29, brettw wrote: > This context menu code is almost an exact copy of some code below. Is it > reasonable to share this code somehow? Done.
brettw, I have made the changes as suggested. Please take another look.
On 2014/09/09 18:18:22, sanjoy_pal wrote: > brettw, I have made the changes as suggested. Please take another look. Friendly ping. Please take a look.
sanjoy.pal@samsung.com changed reviewers: + lazyboy@chromium.org
Hi Istiaque, brettw, Could you please review the CL? Thank you.
I'm not an owner of this dir any more, so I'm removing myself.
https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:75: void ExecuteCustomMenuCommandAndVerifyTabTitle(unsigned command_id, Fix indentation of params. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:79: OpenContextMenuAtPoint(50, 50); Add a comment noting the significance of point (50, 50). https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:156: IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, nit: this can fit in one line. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:162: ui_test_utils::NavigateToURL( nit: can fit in one line. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h (right): https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h:21: void SetCommandId(int id) { command_to_execute_ = id; } Revert this change.
sanjoy.pal@samsung.com changed reviewers: - brettw@chromium.org
Fixed the comments. PTAL. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:75: void ExecuteCustomMenuCommandAndVerifyTabTitle(unsigned command_id, On 2014/09/19 21:38:18, lazyboy wrote: > Fix indentation of params. Done. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:79: OpenContextMenuAtPoint(50, 50); On 2014/09/19 21:38:18, lazyboy wrote: > Add a comment noting the significance of point (50, 50). Done. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:156: IN_PROC_BROWSER_TEST_F(ContextMenuBrowserTest, On 2014/09/19 21:38:18, lazyboy wrote: > nit: this can fit in one line. Done. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:162: ui_test_utils::NavigateToURL( On 2014/09/19 21:38:18, lazyboy wrote: > nit: can fit in one line. Done. https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h (right): https://codereview.chromium.org/296153008/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h:21: void SetCommandId(int id) { command_to_execute_ = id; } On 2014/09/19 21:38:18, lazyboy wrote: > Revert this change. Done.
lgtm
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/296153008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2014/09/23 06:01:03, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hi Istiaque, Blink side implementation of this feature is under a RuntimeEnabledFeatures flag, is that the cause of all *_swarming bots are failing this tests. any idea how do we handle this case? Thank you,
On 2014/09/23 14:06:54, sanjoy_pal wrote: > On 2014/09/23 06:01:03, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > Hi Istiaque, > > Blink side implementation of this feature is under a RuntimeEnabledFeatures > flag, is that the cause of all *_swarming bots are failing this tests. any idea > how do we handle this case? > > Thank you, One way could be to expose setting the flag through RuntimeEnabledFeatures e.g. like this CL: https://codereview.chromium.org/386943009
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/296153008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/296153008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/296153008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/296153008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
