Bug 126508 – [EFL] Merge ewk_view_single into ewk_view

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

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)

[Ryuan Choi]

Created attachment 220408 [details]
Patch

[Ryuan Choi]

Created attachment 220409 [details]
Patch

[Ryuan Choi]

Created attachment 220410 [details]
Patch

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 220595 [details]
Patch

[Ryuan Choi]

Created attachment 220607 [details]
Patch

[Ryuan Choi]

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

[Gyuyoung Kim]

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.

[Ryuan Choi]

Committed r161526: <http://trac.webkit.org/changeset/161526>

[Ryuan Choi]

Comment on attachment 220607 [details]
Patch

Clearing flags.

Landed manually after applied last comment.