Summary: | add Android platform-specific files to WebCore/platform (part 9) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Feng Qian <feng> | ||||
Component: | WebCore Misc. | Assignee: | Feng Qian <feng> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | Linux | ||||||
Attachments: |
|
Created attachment 31115 [details]
patch set
Eric, The new patch addressed most of your comments: 1. functions commented out are removed 2. ASSERT(0) -> ASSERT_NOT_REACHED(); 3. String localization functions are moved to LocalizedStringsAndroid.cpp I didn't change 1-line functions, it has too much editing. This file is mostly empty stubs to make webcore link. Thanks Feng (In reply to comment #0) > + * platform/android/TemporaryLinkStubs.cpp: Added. > > Comments on the old patch: > > (From update of attachment 30005 [details] [review] [review]) > We don't generally commit commented out code: > 97 //void FrameView::updateBorder() { verifiedOk(); } > > I'm surprised there are all these localized strings here when I just reviewed > another patch with unimplemented localized strings. Seems these shoudl be > mvoed, no? > > ASSERT(0); > we use ASSERT_NOT_REACHED() > > In general this is fine. Again if you were a commiter you could just fix it as > you landed, but as is, you should re-post this for someone else to land. > > Please remove all the commetned out ones: > 718 //void Frame::clearPlatformScriptObjects() { notImplemented(); } > > I think you could make all these 1-line definitions to make them more readable. > Up to you. But we generally found them more readable that way. WK does not > have an 80 col limit like google does. > > r- for the nit above. > Comment on attachment 31115 [details]
patch set
I'm surprised some of these are stubs. Like focusRingColor().
But you're really welcome to add whatever you like to this file. ;) rs=me.
+ return IntSize(0, 0);
is the same as IntSize();
You can also disable DEBUGGER and PROFILER instead of adding all these stubs for the debugger.
|
+ * platform/android/TemporaryLinkStubs.cpp: Added. Comments on the old patch: (From update of attachment 30005 [details] [review]) We don't generally commit commented out code: 97 //void FrameView::updateBorder() { verifiedOk(); } I'm surprised there are all these localized strings here when I just reviewed another patch with unimplemented localized strings. Seems these shoudl be mvoed, no? ASSERT(0); we use ASSERT_NOT_REACHED() In general this is fine. Again if you were a commiter you could just fix it as you landed, but as is, you should re-post this for someone else to land. Please remove all the commetned out ones: 718 //void Frame::clearPlatformScriptObjects() { notImplemented(); } I think you could make all these 1-line definitions to make them more readable. Up to you. But we generally found them more readable that way. WK does not have an 80 col limit like google does. r- for the nit above.