Bug 26266 - add Android platform-specific files to WebCore/platform (part 9)
Summary: add Android platform-specific files to WebCore/platform (part 9)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Feng Qian
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-08 17:50 PDT by Feng Qian
Modified: 2009-06-10 12:21 PDT (History)
0 users

See Also:


Attachments
patch set (15.67 KB, patch)
2009-06-09 17:27 PDT, Feng Qian
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Feng Qian 2009-06-08 17:50:38 PDT
+        * 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.
Comment 1 Feng Qian 2009-06-09 17:27:35 PDT
Created attachment 31115 [details]
patch set
Comment 2 Feng Qian 2009-06-09 17:30:40 PDT
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 3 Eric Seidel (no email) 2009-06-10 00:28:18 PDT
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.
Comment 4 Brent Fulgham 2009-06-10 12:21:30 PDT
Landed in @r44587.