Bug 35870 – Upstream iPhone KeyEvent platform code and share with Mac platform

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

The iPhone and Mac platforms both share a large amount of code, yet still have their differences. It would be nice to have a place to put shared code. Something like platform/cocoa, and a PLATFORM(COCOA) flag. Also, we should move up some of the platform/iphone that uses the shared code.

[Joseph Pecoraro]

Created attachment 50227 [details]
PLATFORM(COCOA) and upstreams platform/iphone

[Joseph Pecoraro]

Created attachment 50229 [details]
Updated to ToT for Bots

- bots showed an svn-apply issue with the XCode project, so I updated the patch to ToT.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
re/platform/cocoa/KeyEventCocoa.h:74:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:75:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:76:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:77:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:78:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:79:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:80:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:81:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:82:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:83:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:84:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:85:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:86:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:87:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:88:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:89:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:90:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:91:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:92:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:93:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:94:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:95:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:96:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:97:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:98:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:99:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:100:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:101:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:102:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:103:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/cocoa/KeyEventCocoa.h:104:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 73 in 8 files


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

[Joseph Pecoraro]

Created attachment 50449 [details]
[PATCH] Part 1 - Upstream iPhone Code (unused so far)

[Joseph Pecoraro]

Created attachment 50450 [details]
[PATCH] Part 2 - Move Mac Code into Shared Location (no change to code itself yet)

[Joseph Pecoraro]

Created attachment 50451 [details]
[PATCH] Part 3 - Share Code (make generic for both platforms)

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
orm/iphone/KeyEventCodesIPhone.h:77:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:78:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:79:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:80:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:81:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:82:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:83:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:84:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:85:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:86:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:87:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:88:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:89:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:90:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:91:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:92:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:93:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:94:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:95:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:96:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:97:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:98:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:99:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:100:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:101:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:102:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:103:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:104:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/iphone/KeyEventCodesIPhone.h:105:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 72 in 3 files


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

[Joseph Pecoraro]

I was asked to break this up a little, so I broke this apart into 3 patches.

I also removed the PLATFORM(COCOA) flag concept.  I don't see a need for it if only the Mac / iPhone platforms decide to include / use the new files. Are there build configs that are greedy enough to grab these files unless I protect them with a guard?

[Joseph Pecoraro]

I have an obvious mistake in Part 2. I call windowsKeyCodeForKeyCode when I should call windowsKeyCodeForCharCode. Tests caught these pretty quick.

[Joseph Pecoraro]

Created attachment 50645 [details]
[PATCH] Part 3 - Share Code (make generic for both platforms) + FIX

Looks like it was in Part 3. So I've attached an updated version. Passes the tests locally now.

[David Kilzer (:ddkilzer)]

Comment on attachment 50449 [details]
[PATCH] Part 1 - Upstream iPhone Code (unused so far)

> diff --git a/WebCore/platform/iphone/KeyEventCodesIPhone.h b/WebCore/platform/iphone/KeyEventCodesIPhone.h
> +	NSUpArrowFunctionKey		= 0xF700,

Please change the tabs to spaces in the header file to correct the style issues.

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> (From update of attachment 50449 [details])
> > diff --git a/WebCore/platform/iphone/KeyEventCodesIPhone.h b/WebCore/platform/iphone/KeyEventCodesIPhone.h
> > +	NSUpArrowFunctionKey		= 0xF700,
> 
> Please change the tabs to spaces in the header file to correct the style
> issues.
> 
> r=me

Actually, two more things:

- You need to add these files to the Xcode project file.
- The *.mm file needs #if PLATFORM(IPHONE)/#endif macros around its source so that it doesn't compile for Mac.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 50449 [details] [details])
> > > diff --git a/WebCore/platform/iphone/KeyEventCodesIPhone.h b/WebCore/platform/iphone/KeyEventCodesIPhone.h
> > > +	NSUpArrowFunctionKey		= 0xF700,
> > 
> > Please change the tabs to spaces in the header file to correct the style
> > issues.
> > 
> > r=me
> 
> Actually, two more things:
> 
> - You need to add these files to the Xcode project file.
> - The *.mm file needs #if PLATFORM(IPHONE)/#endif macros around its source so
> that it doesn't compile for Mac.

You can do the above in a follow-up patch as well (and add #if PLATFORM(MAC)/#endif macros around KeyEventMac.mm as well).

[Joseph Pecoraro]

> > - You need to add these files to the Xcode project file.
> > - The *.mm file needs #if PLATFORM(IPHONE)/#endif macros around its source so
> > that it doesn't compile for Mac.
> 
> You can do the above in a follow-up patch as well (and add #if
> PLATFORM(MAC)/#endif macros around KeyEventMac.mm as well).

So the MAC and IPHONE platforms share the same XCode project and the PLATFORM(X) macros pick and choose the files wanted to build for a particular platform?

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> > > - You need to add these files to the Xcode project file.
> > > - The *.mm file needs #if PLATFORM(IPHONE)/#endif macros around its source so
> > > that it doesn't compile for Mac.
> > 
> > You can do the above in a follow-up patch as well (and add #if
> > PLATFORM(MAC)/#endif macros around KeyEventMac.mm as well).
> 
> So the MAC and IPHONE platforms share the same XCode project and the
> PLATFORM(X) macros pick and choose the files wanted to build for a particular
> platform?

Correct.

[David Kilzer (:ddkilzer)]

Comment on attachment 50450 [details]
[PATCH] Part 2 - Move Mac Code into Shared Location (no change to code itself yet)

> diff --git a/WebCore/platform/cocoa/KeyEventCocoa.h b/WebCore/platform/cocoa/KeyEventCocoa.h
> new file mode 100644
> index 0000000..fe1d660
> --- /dev/null
> +++ b/WebCore/platform/cocoa/KeyEventCocoa.h
> +#ifndef KeyEventCocoa_h
> +#define KeyEventCocoa_h
> [...]
> +#endif // !KeyEventCocoa_h

Nit:  usually we don't use a "!" in this comment for headers.  Darin Adler mentioned on webkit-dev that this comment is unnecessary, so you may optionally remove it.

Thanks for using an "svn mv" to create the KeyEventCocoa.mm file!

r=me

[Joseph Pecoraro]

Committed r56360
	M	WebCore/ChangeLog
	M	WebCore/platform/mac/KeyEventMac.mm
	A	WebCore/platform/iphone/KeyEventCodesIPhone.h
	A	WebCore/platform/iphone/KeyEventIPhone.mm
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r56360 = abfebcccb264bc8466cdd6843ab2477e624e2515 (trunk)
http://trac.webkit.org/changeset/56360

(In reply to comment #16)
> Nit:  usually we don't use a "!" in this comment for headers.  Darin Adler
> mentioned on webkit-dev that this comment is unnecessary, so you may optionally
> remove it.

Sounds good.

[David Kilzer (:ddkilzer)]

Comment on attachment 50645 [details]
[PATCH] Part 3 - Share Code (make generic for both platforms) + FIX

> diff --git a/WebCore/platform/iphone/KeyEventIPhone.mm b/WebCore/platform/iphone/KeyEventIPhone.mm
> +    return keyIdentifierForKeyCharCode(c);

I think this should be:

    return keyIdentifierForCharCode(c);

as I don't see a keyIdentifierForKeyCharCode() method in the patch.

r=me

[Joseph Pecoraro]

Committed r56361
	M	WebCore/ChangeLog
	M	WebCore/platform/mac/KeyEventMac.mm
	A	WebCore/platform/cocoa/KeyEventCocoa.h
	A	WebCore/platform/cocoa/KeyEventCocoa.mm
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r56361 = d188be8b60d8e0b305f4ec52950b049ff2a0c0a0 (trunk)
http://trac.webkit.org/changeset/56361

Even though I created an svn checkout specifically for the svn copy, by putting "the move" in a separate patch from "the changes" git figured out it was a copy as well.


(In reply to comment #18)
> I think this should be:
> 
>     return keyIdentifierForCharCode(c);
> 
> as I don't see a keyIdentifierForKeyCharCode() method in the patch.

Good catch! I caught the Mac build through tests, but I didn't fix this typo.

[Joseph Pecoraro]

Committed r56363
	M	WebCore/ChangeLog
	M	WebCore/platform/mac/KeyEventMac.mm
	M	WebCore/platform/iphone/KeyEventIPhone.mm
	M	WebCore/platform/cocoa/KeyEventCocoa.h
	M	WebCore/platform/cocoa/KeyEventCocoa.mm
r56363 = cc578a5357718b5d48199ce34250bf476dc38ebb (trunk)
http://trac.webkit.org/changeset/56363

[Joseph Pecoraro]

Created attachment 51380 [details]
[PATCH] Upstream Minor Fixes to MatchDownstream

After merging this back downstream this required some minor changes. Bringing the minor fixes back upstream.

[David Kilzer (:ddkilzer)]

Comment on attachment 51380 [details]
[PATCH] Upstream Minor Fixes to MatchDownstream

r=me

[Joseph Pecoraro]

Committed r56408
	M	WebCore/ChangeLog
	M	WebCore/platform/iphone/KeyEventCodesIPhone.h
	M	WebCore/platform/iphone/KeyEventIPhone.mm
r56408 = 583c3f6701ac9ed948513ff0581ae533810aac1e (trunk)
http://trac.webkit.org/changeset/56408

All set, thanks!