Bug 92446 – [WK2][EFL] Add ewk_view_scale_{get|set} to EwkView.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

We need a functionality to scale up/down page by user such as pinch zoom.

[Ryuan Choi]

Created attachment 154809 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 154809 [details]
Patch

First of all, why are you storing a local copy of the variable? Are you sure they can never come out of sync?

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 154809 [details])
> First of all, why are you storing a local copy of the variable? Are you sure they can never come out of sync?

I want not to send IPC when same scale factor is requested.
But, I think that it looks unnecessary.

I will remove it.

[Ryuan Choi]

Created attachment 155114 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 155114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:887
> +Eina_Bool ewk_view_scale_set(const Evas_Object* ewkView, double scaleFactor, int x, int y)

EFL port has not used const keyword in _set function so far.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);

-1 => -1.0 ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:391
> +Eina_Bool ewk_view_scale_set(const Evas_Object* ewkView, double scaleFactor, int x, int y);

ditto.

[Gyuyoung Kim]

Comment on attachment 155114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review

BTW, don't you need to support API test as well ?

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:391
>> +Eina_Bool ewk_view_scale_set(const Evas_Object* ewkView, double scaleFactor, int x, int y);
> 
> ditto.

Nit : Move '*' tovariable side.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:403
> +double ewk_view_scale_get(const Evas_Object* ewkView);

ditto.

[Chris Dumez]

Comment on attachment 155114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
>> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);
> 
> -1 => -1.0 ?

Gyuyoung, this would be against coding style.

[Gyuyoung Kim]

(In reply to comment #7)
> (From update of attachment 155114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
> >> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);
> > 
> > -1 => -1.0 ?
> 
> Gyuyoung, this would be against coding style.

Where is the coding style ? please let me know.

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 155114 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
> > >> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);
> > > 
> > > -1 => -1.0 ?
> > 
> > Gyuyoung, this would be against coding style.
> 
> Where is the coding style ? please let me know.

 http://www.webkit.org/coding/coding-style.html
-> earch for "Floating point literals"

[Gyuyoung Kim]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 155114 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review
> > > 
> > > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
> > > >> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);
> > > > 
> > > > -1 => -1.0 ?
> > > 
> > > Gyuyoung, this would be against coding style.
> > 
> > Where is the coding style ? please let me know.
> 
>  http://www.webkit.org/coding/coding-style.html
> -> earch for "Floating point literals"

yes, I just check it. ok, let's use this. Ryuan, please use -1.

[Gyuyoung Kim]

(In reply to comment #7)
> (From update of attachment 155114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
> >> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);
> > 
> > -1 => -1.0 ?
> 
> Gyuyoung, this would be against coding style.

I check this a little further. Are you sure we shouldn't use x.0 in return value case ? It seems to me we need to use casting in return value case. It looks there is no mention in return value case.

See also, 

- http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebView/WebView.mm#L4220
 - http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitwebview.cpp#L4768
 - http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/qt/qwebkittest.cpp#L144

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #7)
> > (From update of attachment 155114 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
> > >> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);
> > > 
> > > -1 => -1.0 ?
> > 
> > Gyuyoung, this would be against coding style.
> 
> I check this a little further. Are you sure we shouldn't use x.0 in return value case ? It seems to me we need to use casting in return value case. It looks there is no mention in return value case.
> 
> See also, 
> 
> - http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebView/WebView.mm#L4220
>  - http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/webkit/webkitwebview.cpp#L4768
>  - http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/qt/qwebkittest.cpp#L144

Return value case is not any different than argument case in my opinion. The coding style gives this example:

void setDiameter(float diameter)
{
    // ...
}
setDiameter(10);

If you look at WebKit source code, of course, you will find a lot of case where they don't follow coding style: nobody is perfect.

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #7)
> > (From update of attachment 155114 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155114&action=review
> > 
> > >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:898
> > >> +    EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, -1);
> > > 
> > > -1 => -1.0 ?
> > 
> > Gyuyoung, this would be against coding style.
> 
> I check this a little further. Are you sure we shouldn't use x.0 in return value case ? It seems to me we need to use casting in return value case. It looks there is no mention in return value case.

The compiler will do an implicit conversion from int to float. The generated binary code will be the same (no performance impact).

[Gyuyoung Kim]

(In reply to comment #13)
 
> The compiler will do an implicit conversion from int to float. The generated binary code will be the same (no performance impact).

Even if there is no performance impact, this is related to coding style. I would like to ask this to reviewers. Reviewer may decide this. This is not major issue in this bug. So, I don't wanna litter this bug's review thread anymore.

[Ryuan Choi]

Created attachment 155144 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155144 [details]
Patch

Clearing flags on attachment: 155144

Committed r123974: <http://trac.webkit.org/changeset/123974>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.