Bug 34524 – [Chromium] Writing Direction context menu for OS X

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Moskovich]

[Chromium] Writing Direction context menu for OS X

[Jeremy Moskovich]

Windows uses a keyboard shortcut to allow users to toggle RTL/LTR alignment of text input fields, on OS X the convention is to use a context menu.

This is the WebKit side of the change needed to add said menu in Chromium.

The reason for using an enabled/checked bitfield rather than passing the current writing direction and having the browser process figure out which menu items to enable/disable is because this is closer to the way the same task is handled in WebKit's core context menu code - WebKit/WebCore/platform/ContextMenu.cpp.

Since the ultimate intention is to rely on that code, it seems to make sense to match the way the code there works as closely as possible.

[Jeremy Moskovich]

Created attachment 48020 [details]
patch

[WebKit Review Bot]

Attachment 48020 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/ContextMenuClientImpl.cpp:220:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Moskovich]

Created attachment 48021 [details]
patch

Fix style - I think the error about header order is erroneous, if not I'd be grateful if someone could point out what to change.

[WebKit Review Bot]

Attachment 48021 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/ContextMenuClientImpl.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

(In reply to comment #4)
> I think the error about header order is erroneous, if not I'd be
> grateful if someone could point out what to change.

WebKit sorts headers in a case sensitive manner and ascii('S') < ascii('o'), so the headers should be ordered llke this:
   #include "CSSPropertyNames.h"
   #include "CSSStyleDeclaration.h"
   #include "ContextMenu.h"

Two other nits:

  You have a two space indent instead of four on the contents of the enum in WebContextMenuData.h

  I Am Not Sure Why The Comment Is Written Like This as opposed to a normal sentence capitalization. "// Writing Direction Context Menu."

[Jeremy Moskovich]

Created attachment 48061 [details]
Fix style issues

David: could you take another look please

[David Levin]

Comment on attachment 48061 [details]
Fix style issues

> diff --git a/WebKit/chromium/public/WebContextMenuData.h b/WebKit/chromium/public/WebContextMenuData.h

Should there be a "#if OS(DARWIN)" around this?

> +    enum CheckableMenuItemFlags {
> +        CheckableMenuItemDisabled = 0x0,
> +        CheckableMenuItemEnabled = 0x1,
> +        CheckableMenuItemChecked = 0x2,
> +    };
> +
> +    // Writing direction menu items.
> +    // Currently only used on OS X.
> +    int writingDirectionDefault;
> +    int writingDirectionLeftToRight;
> +    int writingDirectionRightToLeft;

[Jeremy Moskovich]

This is a public header so to the best of my knowledge I can't include config.h, the #ifdef doesn't really matter here.  If you grep WebKit/chromium/public , none of the headers in there use OS() or PLATFORM() macros - I'm guessing, for similar reasons...

[WebKit Commit Bot]

Comment on attachment 48061 [details]
Fix style issues

Clearing flags on attachment: 48061

Committed r54331: <http://trac.webkit.org/changeset/54331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48061 [details]
Fix style issues

> +++ b/WebKit/chromium/public/WebContextMenuData.h
...
> +    enum CheckableMenuItemFlags {
> +        CheckableMenuItemDisabled = 0x0,
> +        CheckableMenuItemEnabled = 0x1,
> +        CheckableMenuItemChecked = 0x2,
> +    };
> +
> +    // Writing direction menu items.
> +    // Currently only used on OS X.
> +    int writingDirectionDefault;
> +    int writingDirectionLeftToRight;
> +    int writingDirectionRightToLeft;

reading this header file, it is not obvious that writingDirection*
values are unions of the CheckableMenuItem* values.  A comment
seems at least warranted.

[Jeremy Moskovich]

Created attachment 48132 [details]
Better document possible values for writing direction variables.

[Jeremy Moskovich]

Reopening to fix Darin's comment.

[Darin Fisher (:fishd, Google)]

Comment on attachment 48132 [details]
Better document possible values for writing direction variables.

R=me, thanks!

[WebKit Commit Bot]

Comment on attachment 48132 [details]
Better document possible values for writing direction variables.

Clearing flags on attachment: 48132

Committed r54363: <http://trac.webkit.org/changeset/54363>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.