Bug 26265

Summary: add Android platform-specific files to WebCore/platform (part 8)
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:
Description Flags
patch set 1 eric: review+

Feng Qian
Reported 2009-06-08 17:48:52 PDT
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.
Attachments
patch set 1 (5.96 KB, patch)
2009-06-09 14:15 PDT, Feng Qian
eric: review+
Feng Qian
Comment 1 2009-06-09 14:15:03 PDT
Created attachment 31106 [details] patch set 1
Feng Qian
Comment 2 2009-06-09 14:21:48 PDT
(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 3 2009-06-10 00:25:14 PDT
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
Comment 4 2009-06-10 12:20:02 PDT
Landed in @r44586.
Note You need to log in before you can comment on or make changes to this bug.