Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
83163002:
[Aura] Adding Writing direction to the context menu for Aura. (Closed)
|
Created:
7 years, 1 month ago by pals Modified:
7 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, tfarina, jam, darin-cc_chromium.org, eseidel, aharon Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Aura] Adding Writing direction to the context menu for Aura.
BUG=91178
Blink side changes are committed here https://codereview.chromium.org/66313007/
TBR=cpu@chromium.org,cdn@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239457
Patch Set 1 #
Total comments: 13
Patch Set 2 : Patch 2 #Patch Set 3 : Aura-CrOS-Win #
Total comments: 21
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Fixed compilation on Win #
Messages
Total messages: 23 (0 generated)
+pkasting for chrome/browser/ui/views/* +cpu for chrome/app/chrome_command_ids.h +piman for content/* PTAL. Thanks
LGTM for content/
https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:123: int command_id, int event_flags) { Nit: One arg per line; try to put the first arg on the line above if possible https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:136: view_host->UpdateTextDirection(dir); Nit: Shorter and simpler: view_host->UpdateTextDirection((command_id == IDC_WRITING_DIRECTION_RTL) ? blink::WebTextDirectionRightToLeft : blink::WebTextDirectionLeftToRight); https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:186: // This functionality is exposed as a keyboard shortcut on Windows. We need to have a keyboard shortcut on all platforms, context menu or not. If you are only adding this menu because you don't have a shortcut, add a shortcut instead of adding this menu. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h (right): https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:36: #if !defined(OS_WIN) Don't add so many #ifs. Use as few #ifs as possible, e.g. prototype and implement everything for all platforms except for in some minimal number of places in the implementation switch things off for the platforms you don't care about. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:37: // SimpleMenuModel::Delegate implementation. You don't inherit from SimpleMenuModel::Delegate. Put these in the list with the other RenderViewContextMenuDelegate overrides, in the appropriate place. Make these private if they are not called directly by external classes. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:52: // RenderViewContextMenu implementation. Nit: While here: blank line above this
don't see anything where I can add value. Ping me if you need my review, else tbr me. Removing myself.
Fixed the comments. Please review. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:123: int command_id, int event_flags) { On 2013/11/22 21:53:03, Peter Kasting wrote: > Nit: One arg per line; try to put the first arg on the line above if possible Done. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:136: view_host->UpdateTextDirection(dir); On 2013/11/22 21:53:03, Peter Kasting wrote: > Nit: Shorter and simpler: > > view_host->UpdateTextDirection((command_id == IDC_WRITING_DIRECTION_RTL) ? > blink::WebTextDirectionRightToLeft : > blink::WebTextDirectionLeftToRight); Done. looks better. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:186: // This functionality is exposed as a keyboard shortcut on Windows. As per the comment #31 http://code.google.com/p/chromium/issues/detail?id=91178#c31, I thought of exposing this as context menu similar to MAC and Linux. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h (right): https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:36: #if !defined(OS_WIN) On 2013/11/22 21:53:03, Peter Kasting wrote: > Don't add so many #ifs. Use as few #ifs as possible, e.g. prototype and > implement everything for all platforms except for in some minimal number of > places in the implementation switch things off for the platforms you don't care > about. Done. Moved all the changes to a single #if. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:37: // SimpleMenuModel::Delegate implementation. On 2013/11/22 21:53:03, Peter Kasting wrote: > You don't inherit from SimpleMenuModel::Delegate. > Put these in the list with the other RenderViewContextMenuDelegate overrides, in > the appropriate place. > > Make these private if they are not called directly by external classes. Done. Moved to private. https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:52: // RenderViewContextMenu implementation. On 2013/11/22 21:53:03, Peter Kasting wrote: > Nit: While here: blank line above this Done.
https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/1/chrome/browser/ui/views/tab_c... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:186: // This functionality is exposed as a keyboard shortcut on Windows. On 2013/11/25 06:28:13, sanjoy.pal wrote: > As per the comment #31 > http://code.google.com/p/chromium/issues/detail?id=91178#c31, I thought of > exposing this as context menu similar to MAC and Linux. I'm not necessarily opposed to having a context menu at all, but context menus should always be a secondary way of triggering some functionality. It seems like adding a keyboard shortcut should happen either first or at least alongside this patch. Also why wouldn't we be consistent and expose the context menu here on Windows as well? It doesn't make sense to me to implement this only for some platforms. Or is such an option already exposed in the Windows context menu some other way?
Windows has a ctrl-shift (horrible) hotkey for this. Aharon (bidi-team) suggested that we start with a context menu to have *something* on Linux/ChromeOS. There are other alternatives discussed on the bug as well if you'd like. I don't think we can change the windows hotkey (since it matches Word), but we could use something better for ChromeOS/Linux. I don't think linux has a standard for "change writing direction" however. http://superuser.com/questions/543559/how-to-change-the-text-direction-in-lib... says ctrl-shift-d ctrl-shift-a for libre office. FF has ctrl-shift-x on all platforms.
On 2013/11/26 07:14:19, eseidel wrote: > I don't think we can change the windows hotkey (since it matches Word), but we > could use something better for ChromeOS/Linux. I don't think linux has a > standard for "change writing direction" however. > > http://superuser.com/questions/543559/how-to-change-the-text-direction-in-lib... > says ctrl-shift-d ctrl-shift-a for libre office. FF has ctrl-shift-x on all > platforms. Normally CrOS tries to match Windows in these sorts of subtle cases, so I'd suggest ctrl-shift there, even if it's crappy; it's what users would be most likely to try. Ctrl-shift-x is a possible alternative if for some reason we really can't use ctrl-shift. Is there any reason not to add the context menu on Windows as well?
No reason I can think of. On Tuesday, November 26, 2013, wrote: On 2013/11/26 07:14:19, eseidel wrote: I don't think we can change the windows hotkey (since it matches Word), but we could use something better for ChromeOS/Linux. I don't think linux has a standard for "change writing direction" however. http://<http://superuser.com/questions/543559/how-to-change-the-text-direction-in-libreoffice> superuser.com<http://superuser.com/questions/543559/how-to-change-the-text-direction-in-libreoffice> /questions/543559/how-to-change-the-text-direction-in-<http://superuser.com/questions/543559/how-to-change-the-text-direction-in-libreoffice> libreoffice<http://superuser.com/questions/543559/how-to-change-the-text-direction-in-libreoffice> says ctrl-shift-d ctrl-shift-a for libre office. FF has ctrl-shift-x on all platforms. Normally CrOS tries to match Windows in these sorts of subtle cases, so I'd suggest ctrl-shift there, even if it's crappy; it's what users would be most likely to try. Ctrl-shift-x is a possible alternative if for some reason we really can't use ctrl-shift. Is there any reason not to add the context menu on Windows as well? https <https://codereview.chromium.org/83163002/>://<https://codereview.chromium.org...> codereview.chromium.org <https://codereview.chromium.org/83163002/> /83163002/ <https://codereview.chromium.org/83163002/> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I have updated the CL to have context menu on aura-CrOS and Windows as well. I shall attempt to add Ctrl-Shift shortcut support on CrOS and Linux to match the existing Windows behavior. Please review.
https://codereview.chromium.org/83163002/diff/160001/chrome/app/chrome_comman... File chrome/app/chrome_command_ids.h (right): https://codereview.chromium.org/83163002/diff/160001/chrome/app/chrome_comman... chrome/app/chrome_command_ids.h:231: #define IDC_WRITING_DIRECTION_MENU 41120 // OSX, Linux Gtk and Aura These cover most platforms, let's just remove these comments from the 4 lines here. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:20: #if !defined(OS_WIN) These #includes should not be ifdefed. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:119: int event_flags) { Nit: Indent even https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:122: // WebKit's current behavior is for this menu item to always be disabled. Then why are we displaying it in the menu? Or will this be changing imminently? https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:181: // This functionality is exposed as a keyboard shortcut on Windows. This comment seems out of date. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:182: AppendBidiSubMenu(); Why a separate function for this instead of just doing the relevant work here? https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:186: bidi_submenu_model_.AddCheckItem(IDC_WRITING_DIRECTION_DEFAULT, Nit: Wrap after '(' so all lines of args are aligned (3 places) https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h (right): https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:55: void AppendBidiSubMenu(); Nit: Blank line below this https://codereview.chromium.org/83163002/diff/160001/content/public/common/co... File content/public/common/context_menu_params.h (right): https://codereview.chromium.org/83163002/diff/160001/content/public/common/co... content/public/common/context_menu_params.h:127: #if defined(OS_MACOSX) || defined(TOOLKIT_GTK) || defined(USE_AURA) Why don't we just nuke these ifdefs? These cover most platforms and it doesn't seem problematic to send unused details for Android/iOS. Similar comment applies in the .cc file, the builder file, and the IPC header.
Fixed the comments. Please review. https://codereview.chromium.org/83163002/diff/160001/chrome/app/chrome_comman... File chrome/app/chrome_command_ids.h (right): https://codereview.chromium.org/83163002/diff/160001/chrome/app/chrome_comman... chrome/app/chrome_command_ids.h:231: #define IDC_WRITING_DIRECTION_MENU 41120 // OSX, Linux Gtk and Aura On 2013/12/03 22:52:35, Peter Kasting wrote: > These cover most platforms, let's just remove these comments from the 4 lines > here. Done. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:20: #if !defined(OS_WIN) On 2013/12/03 22:52:35, Peter Kasting wrote: > These #includes should not be ifdefed. Done. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:119: int event_flags) { On 2013/12/03 22:52:35, Peter Kasting wrote: > Nit: Indent even Done. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:122: // WebKit's current behavior is for this menu item to always be disabled. On 2013/12/03 22:52:35, Peter Kasting wrote: > Then why are we displaying it in the menu? Or will this be changing imminently? I think default is for dir="auto". see crbug.com/317739. Not sure why it is always be disabled. Mac chrome has same implementation. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:181: // This functionality is exposed as a keyboard shortcut on Windows. On 2013/12/03 22:52:35, Peter Kasting wrote: > This comment seems out of date. Done. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:182: AppendBidiSubMenu(); On 2013/12/03 22:52:35, Peter Kasting wrote: > Why a separate function for this instead of just doing the relevant work here? Done. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:186: bidi_submenu_model_.AddCheckItem(IDC_WRITING_DIRECTION_DEFAULT, On 2013/12/03 22:52:35, Peter Kasting wrote: > Nit: Wrap after '(' so all lines of args are aligned (3 places) Done. https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h (right): https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:55: void AppendBidiSubMenu(); On 2013/12/03 22:52:35, Peter Kasting wrote: > Nit: Blank line below this Done. https://codereview.chromium.org/83163002/diff/160001/content/public/common/co... File content/public/common/context_menu_params.h (right): https://codereview.chromium.org/83163002/diff/160001/content/public/common/co... content/public/common/context_menu_params.h:127: #if defined(OS_MACOSX) || defined(TOOLKIT_GTK) || defined(USE_AURA) On 2013/12/03 22:52:35, Peter Kasting wrote: > Why don't we just nuke these ifdefs? These cover most platforms and it doesn't > seem problematic to send unused details for Android/iOS. > > Similar comment applies in the .cc file, the builder file, and the IPC header. Done.
https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:122: // WebKit's current behavior is for this menu item to always be disabled. On 2013/12/04 12:54:50, sanjoy.pal wrote: > On 2013/12/03 22:52:35, Peter Kasting wrote: > > Then why are we displaying it in the menu? Or will this be changing > imminently? > > I think default is for dir="auto". see crbug.com/317739. Not sure why it is > always be disabled. Mac chrome has same implementation. Right, I'd also think it corresponds to dir=auto. But there's no obvious reason why that shouldn't be available in the menu. Can you investigate why this is always disabled so we can understand whether to remove it from the menu for now or just make it enabled at the appropriate times? https://codereview.chromium.org/83163002/diff/200001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h (right): https://codereview.chromium.org/83163002/diff/200001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:54: // Adds writing direction submenu. Nit: This comment is vestigial. https://codereview.chromium.org/83163002/diff/200001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:56: Nit: This blank line is inappropriate. (The previous "add blank line" comment referred to AppendBidiSubMenu().)
https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:122: // WebKit's current behavior is for this menu item to always be disabled. I found original implementaion on OSX (webkit side https://bugs.webkit.org/show_bug.cgi?id=34524 and chromium side https://codereview.chromium.org/566031) which does not explain much about this behavior. (I have also sent a mail to jeremy, waiting for him to get back on this) and I came across this https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... We can enable "default" for Natural writing direction ("inherit"). https://codereview.chromium.org/83163002/diff/200001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h (right): https://codereview.chromium.org/83163002/diff/200001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:54: // Adds writing direction submenu. On 2013/12/04 19:46:31, Peter Kasting wrote: > Nit: This comment is vestigial. Done. https://codereview.chromium.org/83163002/diff/200001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:56: On 2013/12/04 19:46:31, Peter Kasting wrote: > Nit: This blank line is inappropriate. (The previous "add blank line" comment > referred to AppendBidiSubMenu().) Done.
https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc (right): https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:122: // WebKit's current behavior is for this menu item to always be disabled. If I had to guess, I'd think this is a "match-the-mac-platform-ism" leftover from WebKit, you can find documentation on it here: http://support.apple.com/kb/PH11211 . Aharon would be able to weigh in on whether this item makes sense on other platforms.
On 2013/12/05 14:33:17, jeremy wrote: > https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... > File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc > (right): > > https://codereview.chromium.org/83163002/diff/160001/chrome/browser/ui/views/... > chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc:122: // > WebKit's current behavior is for this menu item to always be disabled. > If I had to guess, I'd think this is a "match-the-mac-platform-ism" leftover > from WebKit, you can find documentation on it here: > http://support.apple.com/kb/PH11211 . It sounds from that link like "default" is a valid direction, and it also is sounding like maybe this can be enabled sometimes in WebKit? If that's the case, it seems good to have it in the context menu. If it's not the case, it seems like WebKit should allow this as a valid choice. LGTM on this CL, I will leave it to Aharon/Jeremy et al. to decide whether there are followup changes needed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/83163002/220001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/83163002/220001
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/83163002/260001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/83163002/280001
Message was sent while issue was closed.
Change committed as 239457 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
