Bug 90778

Created attachment 151236 [details] Zoom for wheel event with pressed control key.

Attachment 151236 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:140: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 151239 [details] Zoom for wheel event with pressed control key. Fix the coding style error.

Created attachment 152516 [details] Patch

Comment on attachment 152516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152516&action=review LGTM with minor nit. > Source/WebKit2/ChangeLog:8 > + Zoom in the page when the vertial mouse wheel scrolls up and zoom out "vertical"

Comment on attachment 152516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152516&action=review To my quick search, it looks other ports don't support this functionality in port layer. So, I'm not sure if EFL port should support this. I wonder whether application can do this via existing APIs. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:152 > + if (priv->isWheelZoomEnabled && evas_key_modifier_is_set(wheelEvent->modifiers, "Control") && !wheelEvent->direction) { In my humble opinion, it is good to be compliant with functionality of other mouse event functions. In other words, I don't think we can add additional functionality to only this wheel event function. How do you think about my opinion ?

(In reply to comment #5) > (From update of attachment 152516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152516&action=review > > LGTM with minor nit. > > > Source/WebKit2/ChangeLog:8 > > + Zoom in the page when the vertial mouse wheel scrolls up and zoom out > > "vertical" my mistake, I will fix it.

(In reply to comment #6) > (From update of attachment 152516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152516&action=review > > To my quick search, it looks other ports don't support this functionality in port layer. So, I'm not sure if EFL port should support this. I wonder whether application can do this via existing APIs. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:152 > > + if (priv->isWheelZoomEnabled && evas_key_modifier_is_set(wheelEvent->modifiers, "Control") && !wheelEvent->direction) { > > In my humble opinion, it is good to be compliant with functionality of other mouse event functions. In other words, I don't think we can add additional functionality to only this wheel event function. How do you think about my opinion ? Gyuyoung, The WK1's win and wx port support wheel zoom in their port and other ports do not support. I've added wheel zoom in the EFL port because it is easier to implement than application side. If application implements the wheel zoom, they have to override ewk_view's mouse wheel functions. As you know, EFL is the smart class and many codes are needed to override ewk_view's smart APIs. So, I want to support wheel zoom in the EFL port and also offer the API to enable/disable the wheel zoom in order to reduce the application's work.

Created attachment 152679 [details] Patch

(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 152516 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=152516&action=review > > > > To my quick search, it looks other ports don't support this functionality in port layer. So, I'm not sure if EFL port should support this. I wonder whether application can do this via existing APIs. > > > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:152 > > > + if (priv->isWheelZoomEnabled && evas_key_modifier_is_set(wheelEvent->modifiers, "Control") && !wheelEvent->direction) { > > > > In my humble opinion, it is good to be compliant with functionality of other mouse event functions. In other words, I don't think we can add additional functionality to only this wheel event function. How do you think about my opinion ? > > Gyuyoung, > > The WK1's win and wx port support wheel zoom in their port and other ports do not support. > I've added wheel zoom in the EFL port because it is easier to implement than application side. > If application implements the wheel zoom, they have to override ewk_view's mouse wheel functions. > As you know, EFL is the smart class and many codes are needed to override ewk_view's smart APIs. > So, I want to support wheel zoom in the EFL port and also offer the API to enable/disable the wheel zoom in order to reduce the application's work. I can understand what do you want. However, I'm not sure if reviewer can accept this patch. If reviewer can accept to land this patch, I won't object to land this patch as well. 152 if (priv->isWheelZoomEnabled && evas_key_modifier_is_set(wheelEvent->modifiers, "Control") && !wheelEvent->direction) { 153 double currentPageZoomFactor = priv->pageClient->page()->pageZoomFactor(); 154 if (wheelEvent->z < 0 && currentPageZoomFactor < zoomMaximum) 155 priv->pageClient->page()->setPageZoomFactor(currentPageZoomFactor * zoomInRatio); 156 if (wheelEvent->z > 0 && currentPageZoomFactor > zoomMinimum) 157 priv->pageClient->page()->setPageZoomFactor(currentPageZoomFactor * zoomOutRatio); 158 return true; 159 } BTW, I think is it better to move below logic to new internal function from the perspective of code readability ?

Actually, Gyouyoung is right. We had a long discussion with Kenneth, Ryuan and Gyuyoung about zooming. We eventually agreed that zooming was deprecated and we should avoid it in the API. As a result, we did not add APIs for page or text zoom to Ewk_View in WK2. I think this patch should be dropped as well, unless we have a specific need for it (but I think we don't).

(In reply to comment #11) > Actually, Gyouyoung is right. We had a long discussion with Kenneth, Ryuan and Gyuyoung about zooming. We eventually agreed that zooming was deprecated and we should avoid it in the API. As a result, we did not add APIs for page or text zoom to Ewk_View in WK2. I think this patch should be dropped as well, unless we have a specific need for it (but I think we don't). I think wheel zoom is the different case. I can use "page scale" instead of "page zoom". With "page scale", there is no problem for zooming-in, but I have a problem for zooming-out. If we zooming-out the page with "page scale", the white area is shown in the right side of the contents because "page scale" does not re-layout the contents after scaling. So, I've used the "page zoom" for the wheel zoom. If we should not use the "page zoom" any more, how can I implement the zooming-out? Can I ignore the zooming-out and set the zoom factor after loading to the minimum zoom factor?? If it is the right approach, I can change the code to use "page scale". I want to listen Christophe and Gyuyoung's opinion.

(In reply to comment #12) > If we should not use the "page zoom" any more, how can I implement the zooming-out? > Can I ignore the zooming-out and set the zoom factor after loading to the minimum zoom factor?? > If it is the right approach, I can change the code to use "page scale". > > I want to listen Christophe and Gyuyoung's opinion. In my humble opinion, to use page scale is better than page zoom. But, did you check how did QT port support this functionality ? I think we need to refer to QT implementation first. CC'ing Kenneth, when you have time, could you take a look this patch ? I think we need your advice.

(In reply to comment #13) > (In reply to comment #12) > > > If we should not use the "page zoom" any more, how can I implement the zooming-out? > > Can I ignore the zooming-out and set the zoom factor after loading to the minimum zoom factor?? > > If it is the right approach, I can change the code to use "page scale". > > > > I want to listen Christophe and Gyuyoung's opinion. > > In my humble opinion, to use page scale is better than page zoom. But, did you check how did QT port support this functionality ? I think we need to refer to QT implementation first. > > CC'ing Kenneth, when you have time, could you take a look this patch ? I think we need your advice. I don't know how many people who actually use wheel zoom today, especially as it can easily conflict with web sites using the wheel already, thus you will need to first send the events to the view and if it didn't use then the browser view can. That is a very slow patch which is the reason for the ScrollingTree work. With that part of this information is on the UI side, so you can quickly decide whether you need to go the slow path or not. If you want to do full page zoom, it is possible to use it using the device pixel ratio which has the potential benefit of using higher resolution images.

Comment on attachment 152679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152679&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:160 > + if (priv->isWheelZoomEnabled && evas_key_modifier_is_set(wheelEvent->modifiers, "Control") && !wheelEvent->direction) { > + double currentPageZoomFactor = priv->pageClient->page()->pageZoomFactor(); > + if (wheelEvent->z < 0 && currentPageZoomFactor < zoomMaximum) > + priv->pageClient->page()->setPageZoomFactor(currentPageZoomFactor * zoomInRatio); > + if (wheelEvent->z > 0 && currentPageZoomFactor > zoomMinimum) > + priv->pageClient->page()->setPageZoomFactor(currentPageZoomFactor * zoomOutRatio); > + return true; > + } > + You are not even checking whether the web process is using the wheel events, so this can break pages :-( If you once do a webkit efl summit in korea you should invite me :-) Then I can explain must of this stuff at once.

(In reply to comment #15) > (From update of attachment 152679 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152679&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:160 > > + if (priv->isWheelZoomEnabled && evas_key_modifier_is_set(wheelEvent->modifiers, "Control") && !wheelEvent->direction) { > > + double currentPageZoomFactor = priv->pageClient->page()->pageZoomFactor(); > > + if (wheelEvent->z < 0 && currentPageZoomFactor < zoomMaximum) > > + priv->pageClient->page()->setPageZoomFactor(currentPageZoomFactor * zoomInRatio); > > + if (wheelEvent->z > 0 && currentPageZoomFactor > zoomMinimum) > > + priv->pageClient->page()->setPageZoomFactor(currentPageZoomFactor * zoomOutRatio); > > + return true; > > + } > > + > > You are not even checking whether the web process is using the wheel events, so this can break pages :-( > > If you once do a webkit efl summit in korea you should invite me :-) Then I can explain must of this stuff at once. I also want to have a change to invite you :) and, I've thought about this patch again. Actually, only the Tizen desktop browser requests the wheel zoom feature to me. I don't think I have to support wheel zoom by submitting to slow performance. So, I change my mind to pass the role of wheel zoom to application even though it is hard work for application. and, for that, I have to provide the way to inherit the ewk_view's SmartClass. That is in the Bug 90054 and I have to update that firstly.