Bug 35870 - Upstream iPhone KeyEvent platform code and share with Mac platform
Summary: Upstream iPhone KeyEvent platform code and share with Mac platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.6
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-08 08:49 PST by Joseph Pecoraro
Modified: 2010-03-23 12:40 PDT (History)
4 users (show)

See Also:


Attachments
PLATFORM(COCOA) and upstreams platform/iphone (116.82 KB, patch)
2010-03-08 08:58 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Updated to ToT for Bots (117.12 KB, patch)
2010-03-08 09:40 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Part 1 - Upstream iPhone Code (unused so far) (17.14 KB, patch)
2010-03-10 16:02 PST, Joseph Pecoraro
ddkilzer: review+
Details | Formatted Diff | Diff
[PATCH] Part 2 - Move Mac Code into Shared Location (no change to code itself yet) (55.26 KB, patch)
2010-03-10 16:03 PST, Joseph Pecoraro
ddkilzer: review+
Details | Formatted Diff | Diff
[PATCH] Part 3 - Share Code (make generic for both platforms) (14.87 KB, patch)
2010-03-10 16:04 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Part 3 - Share Code (make generic for both platforms) + FIX (14.87 KB, patch)
2010-03-12 17:55 PST, Joseph Pecoraro
ddkilzer: review+
Details | Formatted Diff | Diff
[PATCH] Upstream Minor Fixes to MatchDownstream (1.68 KB, patch)
2010-03-22 18:39 PDT, Joseph Pecoraro
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2010-03-08 08:49:56 PST
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.
Comment 1 Joseph Pecoraro 2010-03-08 08:58:39 PST
Created attachment 50227 [details]
PLATFORM(COCOA) and upstreams platform/iphone
Comment 2 Joseph Pecoraro 2010-03-08 09:40:29 PST
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.
Comment 3 WebKit Review Bot 2010-03-08 09:48:15 PST
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.
Comment 4 Joseph Pecoraro 2010-03-10 16:02:17 PST
Created attachment 50449 [details]
[PATCH] Part 1 - Upstream iPhone Code (unused so far)
Comment 5 Joseph Pecoraro 2010-03-10 16:03:17 PST
Created attachment 50450 [details]
[PATCH] Part 2 - Move Mac Code into Shared Location (no change to code itself yet)
Comment 6 Joseph Pecoraro 2010-03-10 16:04:29 PST
Created attachment 50451 [details]
[PATCH] Part 3 - Share Code (make generic for both platforms)
Comment 7 WebKit Review Bot 2010-03-10 16:06:00 PST
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.
Comment 8 Joseph Pecoraro 2010-03-10 16:08:17 PST
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?
Comment 9 Joseph Pecoraro 2010-03-12 17:37:35 PST
I have an obvious mistake in Part 2. I call windowsKeyCodeForKeyCode when I should call windowsKeyCodeForCharCode. Tests caught these pretty quick.
Comment 10 Joseph Pecoraro 2010-03-12 17:55:34 PST
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.
Comment 11 David Kilzer (:ddkilzer) 2010-03-22 12:13:29 PDT
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
Comment 12 David Kilzer (:ddkilzer) 2010-03-22 12:16:23 PDT
(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.
Comment 13 David Kilzer (:ddkilzer) 2010-03-22 12:17:10 PDT
(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).
Comment 14 Joseph Pecoraro 2010-03-22 12:28:52 PDT
> > - 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?
Comment 15 David Kilzer (:ddkilzer) 2010-03-22 12:40:53 PDT
(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.
Comment 16 David Kilzer (:ddkilzer) 2010-03-22 14:15:13 PDT
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
Comment 17 Joseph Pecoraro 2010-03-22 14:29:03 PDT
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.
Comment 18 David Kilzer (:ddkilzer) 2010-03-22 14:37:17 PDT
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
Comment 19 Joseph Pecoraro 2010-03-22 15:00:31 PDT
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.
Comment 20 Joseph Pecoraro 2010-03-22 15:35:01 PDT
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
Comment 21 Joseph Pecoraro 2010-03-22 18:39:15 PDT
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.
Comment 22 David Kilzer (:ddkilzer) 2010-03-22 20:29:44 PDT
Comment on attachment 51380 [details]
[PATCH] Upstream Minor Fixes to MatchDownstream

r=me
Comment 23 Joseph Pecoraro 2010-03-23 12:40:58 PDT
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!