Bug 92446

Created attachment 154809 [details] Patch

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?

(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.

Created attachment 155114 [details] Patch

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 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 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.

(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.

(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"

(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.

(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

(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.

(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).

(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.

Created attachment 155144 [details] Patch

Comment on attachment 155144 [details] Patch Clearing flags on attachment: 155144 Committed r123974: <http://trac.webkit.org/changeset/123974>

All reviewed patches have been landed. Closing bug.