Bug 34524 - [Chromium] Writing Direction context menu for OS X
Summary: [Chromium] Writing Direction context menu for OS X
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-03 05:53 PST by Jeremy Moskovich
Modified: 2010-02-04 12:11 PST (History)
5 users (show)

See Also:


Attachments
patch (3.29 KB, patch)
2010-02-03 06:15 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
patch (3.29 KB, patch)
2010-02-03 06:23 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Fix style issues (3.27 KB, patch)
2010-02-03 12:45 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Better document possible values for writing direction variables. (1.33 KB, patch)
2010-02-04 04:16 PST, Jeremy Moskovich
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2010-02-03 05:53:53 PST
[Chromium] Writing Direction context menu for OS X
Comment 1 Jeremy Moskovich 2010-02-03 06:00:09 PST
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.
Comment 2 Jeremy Moskovich 2010-02-03 06:15:08 PST
Created attachment 48020 [details]
patch
Comment 3 WebKit Review Bot 2010-02-03 06:17:46 PST
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.
Comment 4 Jeremy Moskovich 2010-02-03 06:23:37 PST
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.
Comment 5 WebKit Review Bot 2010-02-03 06:28:57 PST
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.
Comment 6 David Levin 2010-02-03 07:13:47 PST
(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."
Comment 7 Jeremy Moskovich 2010-02-03 12:45:12 PST
Created attachment 48061 [details]
Fix style issues

David: could you take another look please
Comment 8 David Levin 2010-02-03 14:45:52 PST
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;
Comment 9 Jeremy Moskovich 2010-02-03 14:51:04 PST
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...
Comment 10 WebKit Commit Bot 2010-02-04 00:41:57 PST
Comment on attachment 48061 [details]
Fix style issues

Clearing flags on attachment: 48061

Committed r54331: <http://trac.webkit.org/changeset/54331>
Comment 11 WebKit Commit Bot 2010-02-04 00:42:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Fisher (:fishd, Google) 2010-02-04 02:46:15 PST
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.
Comment 13 Jeremy Moskovich 2010-02-04 04:16:17 PST
Created attachment 48132 [details]
Better document possible values for writing direction variables.
Comment 14 Jeremy Moskovich 2010-02-04 04:17:12 PST
Reopening to fix Darin's comment.
Comment 15 Darin Fisher (:fishd, Google) 2010-02-04 11:37:49 PST
Comment on attachment 48132 [details]
Better document possible values for writing direction variables.

R=me, thanks!
Comment 16 WebKit Commit Bot 2010-02-04 12:10:51 PST
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>
Comment 17 WebKit Commit Bot 2010-02-04 12:11:00 PST
All reviewed patches have been landed.  Closing bug.