Summary: | [EFL] Merge ewk_view_single into ewk_view | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||||||
Component: | WebKit EFL | Assignee: | Ryuan Choi <ryuan.choi> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rakuco | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Ryuan Choi
2014-01-05 17:03:01 PST
Created attachment 220408 [details]
Patch
Created attachment 220409 [details]
Patch
Created attachment 220410 [details]
Patch
Comment on attachment 220410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220410&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:1014 > + Evas_Coord x, y, cw, ch; How about using more meaningful name for cw, ch ? centerWidth, centerHeight ? > Source/WebKit/efl/ewk/ewk_view.cpp:1049 > + startHeight = startHeight + moveLineUpDown; I wonder why we need this line. > Source/WebKit/efl/ewk/ewk_view.cpp:1135 > + return true; I doubt if "return true" is correct logic. > Source/WebKit/efl/ewk/ewk_view.cpp:1147 > + if (smartData->animated_zoom.zoom.current < 0.00001) { How about use a constant variable for 0.00001 ? > Source/WebKit/efl/ewk/ewk_view.cpp:1404 > + // TODO: review Need to re-write with more detail comment. > Source/WebKit/efl/ewk/ewk_view.cpp:1408 > + Evas_Coord dx, dy, cw, ch; ditto. > Source/WebKit/efl/ewk/ewk_view.cpp:1442 > +} Add a new line. Created attachment 220595 [details]
Patch
Created attachment 220607 [details]
Patch
Comment on attachment 220410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220410&action=review >> Source/WebKit/efl/ewk/ewk_view.cpp:1014 >> + Evas_Coord x, y, cw, ch; > > How about using more meaningful name for cw, ch ? centerWidth, centerHeight ? Ok, I changed to contentWidth, contentHeight. >> Source/WebKit/efl/ewk/ewk_view.cpp:1049 >> + startHeight = startHeight + moveLineUpDown; > > I wonder why we need this line. It's rquired. But I refactored little bit. s/startHeight/row and moved into for loop. >> Source/WebKit/efl/ewk/ewk_view.cpp:1135 >> + return true; > > I doubt if "return true" is correct logic. You're right. scrolls_process doesn't need to return anything. I fixed it. >> Source/WebKit/efl/ewk/ewk_view.cpp:1147 >> + if (smartData->animated_zoom.zoom.current < 0.00001) { > > How about use a constant variable for 0.00001 ? It's for epsilon. I changed to std::numeric_limits<float>::epsilon() >> Source/WebKit/efl/ewk/ewk_view.cpp:1404 >> + // TODO: review > > Need to re-write with more detail comment. It looks wrong comment. removed >> Source/WebKit/efl/ewk/ewk_view.cpp:1408 >> + Evas_Coord dx, dy, cw, ch; > > ditto. fixed >> Source/WebKit/efl/ewk/ewk_view.cpp:1442 >> +} > > Add a new line. Ooops. done Comment on attachment 220607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220607&action=review Looks fine. > Source/WebKit/efl/ewk/ewk_view.cpp:1415 > + Evas_Coord dx, dy, contentWidth, contentHeight; It would be nicer if we use more readable name for dx, dy. Committed r161526: <http://trac.webkit.org/changeset/161526> Comment on attachment 220607 [details]
Patch
Clearing flags.
Landed manually after applied last comment.
|