Bug 118548 changed the way contentScaleFactor and contentPosition are manipulated in WebView. After that, we've been dealing with some unexpected behaviors in contentPosition-related stuff. Some Nix API tests are failing and we got some regressions in panning and zoom features of Nix's MiniBrowser. It seems like whenever we set WebView's contentPosition we have to multiply/scale it by the current scale factor explictly, that "solved" most of those issues mentioned before, but I suppose that is not intended. We should make sure we really need to store contentPosition as a scaled value, as it's being done in https://github.com/WebKitNix/webkitnix/blob/master/Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp#L515. Besides that, WebView::pageDidRequestScroll is passing the non-scaled value to client callback, that is at least weird, maybe a separate issue. Macelo Lira and Thiago Lacerda are currently working on some API/Layout tests to make sure this kind of regression won't be missed in future changes. It would be good to have some feedback from Noam and some EFL guys using CoordinatedGraphics and contentScaleFactor related stuff.
Created attachment 219584 [details] Proposed Patch This patch modifies WebView to store contentPosition as a non-scaled value and refactor EFL functions functions that use contentPosition API to do not assume it is stored scaled anymore.
Comment on attachment 219584 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=219584&action=review This patch is LGTM. I added patch in Bug 118548 to make usage of contentPosition consistent by regarding it as scaled value. But as you mentioned, we have to multiply it by the current scale factor whenever set WebView's contentPosition. Codes are more simple after changing contentPosition as a non-scaled value :) > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1403 > FloatPoint newPosition(oldPosition.x + offset.width(), oldPosition.y + offset.height()); If we divide offset with contentScale, we don't have to multiply oldPosition and divide newPosition.
(In reply to comment #2) > (From update of attachment 219584 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219584&action=review > > This patch is LGTM. > I added patch in Bug 118548 to make usage of contentPosition consistent by regarding it as scaled value. > But as you mentioned, we have to multiply it by the current scale factor whenever set WebView's contentPosition. > Codes are more simple after changing contentPosition as a non-scaled value :) > > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1403 > > FloatPoint newPosition(oldPosition.x + offset.width(), oldPosition.y + offset.height()); > > If we divide offset with contentScale, we don't have to multiply oldPosition and divide newPosition. Thanks for the suggestion, indeed that will remove some redundancy from that calculation, I'll send an updated patch applying this change.
Created attachment 219649 [details] Proposed patch Simplifies calculations of newPosition when scrolling in EwkView, as suggested by Eummi.
Comment on attachment 219649 [details] Proposed patch Rejecting attachment 219649 [details] from commit-queue. nick.yamane@openbossa.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 219649 [details] Proposed patch Clearing flags on attachment: 219649 Committed r160833: <http://trac.webkit.org/changeset/160833>
All reviewed patches have been landed. Closing bug.