Bug 26265 – add Android platform-specific files to WebCore/platform (part 8)

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

[Feng Qian]

This is a spin-off of bug 23296, patch part 8:
+        * platform/android/SystemTimeAndroid.cpp: Added.
+        * platform/android/TextBoundaries.cpp: Added.
+        * platform/android/TextBreakIteratorInternalICU.cpp: Added.
+        * platform/android/WidgetAndroid.cpp: Added.

Original comments:

(From update of attachment 30004 [details] [review])
I'm surprised we have any time code in WebCore?  Isn't that all down in WTF?

Why does andriod need its own different ICU code?  TextBoundaries.cpp?

IntRect() is the same as  54         return IntRect(0, 0, 0, 0);

Isn't there an easier way to get the FrameView than your manual crawl?

r- for the nits and questions.

[Feng Qian]

Created attachment 31106 [details]
patch set 1

[Feng Qian]

(In reply to comment #0)
> This is a spin-off of bug 23296, patch part 8:
> +        * platform/android/SystemTimeAndroid.cpp: Added.
> +        * platform/android/TextBoundaries.cpp: Added.
> +        * platform/android/TextBreakIteratorInternalICU.cpp: Added.
> +        * platform/android/WidgetAndroid.cpp: Added.
> 
> Original comments:
> 
> (From update of attachment 30004 [details] [review] [review])
> I'm surprised we have any time code in WebCore?  Isn't that all down in WTF?
> 

Time code seems necessary to link WebCore/history/PageCache.cpp, there is a similar file, WebCore/platform/win/SystemTimeWin.cpp.

> Why does andriod need its own different ICU code?  TextBoundaries.cpp?
> 

You are right, these files should under platform/text, removed. Will add them in a separate CL.

> IntRect() is the same as  54         return IntRect(0, 0, 0, 0);
> 
> Isn't there an easier way to get the FrameView than your manual crawl?
> 

Only function similar is Widget::root(), but not the exact same.
ScrollView* Widget::root() const
{
    const Widget* top = this;
    while (top->parent())
        top = top->parent();
    if (top->isFrameView())
        return const_cast<ScrollView*>(static_cast<const ScrollView*>(top));
    return 0;
}

root() directly goes to the root Widget, but in screenWidth(), it searches the first FrameView ancestor. I don't know if it is possible to have a non-FrameView ancestor that's not a root, but both code are different. (it will be great if that's true).

> r- for the nits and questions.
> 

[Eric Seidel (no email)]

Comment on attachment 31106 [details]
patch set 1

This probably should have a notImplemented() call:
+    // Needed for PageCache, which we currently have disabled.
+    return 0.0F;

I'm also surprised to see an F there instead of an 'f'.  I didn't even know F was valid.


+    // FIXME: use m_frame instead?
+    if (!platformWidget())
+        return IntRect(0, 0, 0, 0);
IntRect() is the same

Looks fine.  Please fix those nits when landing.

[Brent Fulgham]

Landed in @r44586.