Bug 35870 - Upstream iPhone KeyEvent platform code and share with Mac platform
: Upstream iPhone KeyEvent platform code and share with Mac platform
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.6
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-03-08 08:49 PST by
Modified: 2010-03-23 12:40 PST (History)


Attachments
PLATFORM(COCOA) and upstreams platform/iphone (116.82 KB, patch)
2010-03-08 08:58 PST, Joseph Pecoraro
no flags Review Patch | Details | Formatted Diff | Diff
Updated to ToT for Bots (117.12 KB, patch)
2010-03-08 09:40 PST, Joseph Pecoraro
no flags Review Patch | 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+
Review Patch | 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+
Review Patch | 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 Review Patch | 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+
Review Patch | Details | Formatted Diff | Diff
[PATCH] Upstream Minor Fixes to MatchDownstream (1.68 KB, patch)
2010-03-22 18:39 PST, Joseph Pecoraro
ddkilzer: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-03-08 08:58:39 PST -------
Created an attachment (id=50227) [details]
PLATFORM(COCOA) and upstreams platform/iphone
------- Comment #2 From 2010-03-08 09:40:29 PST -------
Created an attachment (id=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 From 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 From 2010-03-10 16:02:17 PST -------
Created an attachment (id=50449) [details]
[PATCH] Part 1 - Upstream iPhone Code (unused so far)
------- Comment #5 From 2010-03-10 16:03:17 PST -------
Created an attachment (id=50450) [details]
[PATCH] Part 2 - Move Mac Code into Shared Location (no change to code itself yet)
------- Comment #6 From 2010-03-10 16:04:29 PST -------
Created an attachment (id=50451) [details]
[PATCH] Part 3 - Share Code (make generic for both platforms)
------- Comment #7 From 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 From 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 From 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 From 2010-03-12 17:55:34 PST -------
Created an attachment (id=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 From 2010-03-22 12:13:29 PST -------
(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
------- Comment #12 From 2010-03-22 12:16:23 PST -------
(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.
------- Comment #13 From 2010-03-22 12:17:10 PST -------
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 50449 [details] [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 From 2010-03-22 12:28:52 PST -------
> > - 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 From 2010-03-22 12:40:53 PST -------
(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 From 2010-03-22 14:15:13 PST -------
(From update of attachment 50450 [details])
> 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 From 2010-03-22 14:29:03 PST -------
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 From 2010-03-22 14:37:17 PST -------
(From update of attachment 50645 [details])
> 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 From 2010-03-22 15:00:31 PST -------
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 From 2010-03-22 15:35:01 PST -------
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 From 2010-03-22 18:39:15 PST -------
Created an attachment (id=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 From 2010-03-22 20:29:44 PST -------
(From update of attachment 51380 [details])
r=me
------- Comment #23 From 2010-03-23 12:40:58 PST -------
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!