Bug 26266 – add Android platform-specific files to WebCore/platform (part 9)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Feng Qian]

+        * 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.

[Feng Qian]

Created attachment 31115 [details]
patch set

[Feng Qian]

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.
> 

[Eric Seidel (no email)]

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.

[Brent Fulgham]

Landed in @r44587.