Bug 92446 - [WK2][EFL] Add ewk_view_scale_{get|set} to EwkView.
Summary: [WK2][EFL] Add ewk_view_scale_{get|set} to EwkView.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks: 61838
  Show dependency treegraph
 
Reported: 2012-07-26 18:50 PDT by Ryuan Choi
Modified: 2012-07-28 16:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.80 KB, patch)
2012-07-26 19:05 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (3.36 KB, patch)
2012-07-27 23:29 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (3.35 KB, patch)
2012-07-28 15:18 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2012-07-26 18:50:31 PDT
We need a functionality to scale up/down page by user such as pinch zoom.
Comment 1 Ryuan Choi 2012-07-26 19:05:35 PDT
Created attachment 154809 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-07-27 03:31:40 PDT
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?
Comment 3 Ryuan Choi 2012-07-27 05:02:47 PDT
(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.
Comment 4 Ryuan Choi 2012-07-27 23:29:19 PDT
Created attachment 155114 [details]
Patch
Comment 5 Gyuyoung Kim 2012-07-27 23:33:07 PDT
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.
Comment 6 Gyuyoung Kim 2012-07-27 23:34:20 PDT
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.
Comment 7 Chris Dumez 2012-07-27 23:38:57 PDT
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.
Comment 8 Gyuyoung Kim 2012-07-27 23:41:23 PDT
(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.
Comment 9 Chris Dumez 2012-07-27 23:42:16 PDT
(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"
Comment 10 Gyuyoung Kim 2012-07-27 23:44:39 PDT
(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.
Comment 11 Gyuyoung Kim 2012-07-28 00:10:21 PDT
(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
Comment 12 Chris Dumez 2012-07-28 00:23:30 PDT
(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.
Comment 13 Chris Dumez 2012-07-28 00:27:40 PDT
(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).
Comment 14 Gyuyoung Kim 2012-07-28 00:37:53 PDT
(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.
Comment 15 Ryuan Choi 2012-07-28 15:18:24 PDT
Created attachment 155144 [details]
Patch
Comment 16 WebKit Review Bot 2012-07-28 16:28:33 PDT
Comment on attachment 155144 [details]
Patch

Clearing flags on attachment: 155144

Committed r123974: <http://trac.webkit.org/changeset/123974>
Comment 17 WebKit Review Bot 2012-07-28 16:28:39 PDT
All reviewed patches have been landed.  Closing bug.