Bug 126508

Summary: [EFL] Merge ewk_view_single into ewk_view
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2014-01-05 17:03:01 PST
Since ewk_view_tiled is removed, we don't need to keep ewk_view_single.cpp out of ewk_view_single.cpp

It just makes extra hierarchy(ewk_base and ewk_single)
Comment 1 Ryuan Choi 2014-01-05 17:09:24 PST
Created attachment 220408 [details]
Patch
Comment 2 Ryuan Choi 2014-01-05 17:16:05 PST
Created attachment 220409 [details]
Patch
Comment 3 Ryuan Choi 2014-01-05 17:21:48 PST
Created attachment 220410 [details]
Patch
Comment 4 Gyuyoung Kim 2014-01-07 20:19:35 PST
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.
Comment 5 Ryuan Choi 2014-01-07 22:06:44 PST
Created attachment 220595 [details]
Patch
Comment 6 Ryuan Choi 2014-01-08 01:08:50 PST
Created attachment 220607 [details]
Patch
Comment 7 Ryuan Choi 2014-01-08 01:12:15 PST
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 8 Gyuyoung Kim 2014-01-08 15:52:30 PST
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.
Comment 9 Ryuan Choi 2014-01-08 16:55:34 PST
Committed r161526: <http://trac.webkit.org/changeset/161526>
Comment 10 Ryuan Choi 2014-01-08 16:56:05 PST
Comment on attachment 220607 [details]
Patch

Clearing flags.

Landed manually after applied last comment.