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.
Created attachment 50227 [details] PLATFORM(COCOA) and upstreams platform/iphone
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.
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.
Created attachment 50449 [details] [PATCH] Part 1 - Upstream iPhone Code (unused so far)
Created attachment 50450 [details] [PATCH] Part 2 - Move Mac Code into Shared Location (no change to code itself yet)
Created attachment 50451 [details] [PATCH] Part 3 - Share Code (make generic for both platforms)
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.
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?
I have an obvious mistake in Part 2. I call windowsKeyCodeForKeyCode when I should call windowsKeyCodeForCharCode. Tests caught these pretty quick.
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 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
(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.
(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).
> > - 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?
(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 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
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 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
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.
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
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 on attachment 51380 [details] [PATCH] Upstream Minor Fixes to MatchDownstream r=me
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!