Summary: | [Chromium] Add Android keycodes | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bolin Hsu <bhsu> | ||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Minor | CC: | abarth, peter, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Android | ||||||||||||
OS: | Android | ||||||||||||
Attachments: |
|
Description
Bolin Hsu
2012-03-22 12:47:14 PDT
Created attachment 133387 [details]
Patch that adds Android keycodes
Created attachment 133396 [details]
Patch that adds Android keycodes
Did you mean to mark this patch for review? Comment on attachment 133396 [details] Patch that adds Android keycodes View in context: https://bugs.webkit.org/attachment.cgi?id=133396&action=review Thanks! I've got some questions/comments on the patch (following up on the patch failing to apply in the e-mail thread). > WebKit/Source/WebCore/ChangeLog:3 > + Add Android keycodes nit: Since this is Chromium-specific code, we usually prepend the title with [Chromium]. > WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:36 > +namespace { The usage of anonymous namespaces within WebKit is not really preferred (though not completely disallowed either). What is the benefit of putting these values in one? It's going to break during a future NDK update either way. > WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:238 > +#if defined(GTV) What is the reason that we want to keep this specific to GTV? (In reply to comment #4) > (From update of attachment 133396 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133396&action=review > > Thanks! I've got some questions/comments on the patch (following up on the patch failing to apply in the e-mail thread). > > > WebKit/Source/WebCore/ChangeLog:3 > > + Add Android keycodes > > nit: Since this is Chromium-specific code, we usually prepend the title with [Chromium]. I will change the title, if this is possible. > > > WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:36 > > +namespace { > > The usage of anonymous namespaces within WebKit is not really preferred (though not completely disallowed either). What is the benefit of putting these values in one? It's going to break during a future NDK update either way. The intention of the anonymous namespace is to limit the visibility of the enum to this file. I will remove it. > > > WebKit/Source/WebCore/platform/chromium/KeyCodeConversionAndroid.cpp:238 > > +#if defined(GTV) > > What is the reason that we want to keep this specific to GTV? The guarded part uses GTV specific VKEY_OEM_103 and VKEY_OEM_104 defined elsewhere. I will take this chance to add the definition of these two codes, then this is not GTV specific anymore. Created attachment 133850 [details]
Patch that adds Android keycodes
New patch that includes recommendation from Peter's review.
Created attachment 133906 [details]
Patch
Comment on attachment 133906 [details]
Patch
Thanks for the patch!
Comment on attachment 133906 [details] Patch Clearing flags on attachment: 133906 Committed r112174: <http://trac.webkit.org/changeset/112174> All reviewed patches have been landed. Closing bug. |