[webkit-reviews] review denied: [Bug 70004] [EFL] Change the behavior of smart_zoom. : [Attachment 110863] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied Ryuan Choi
<ryuan.choi at samsung.com>'s request for review:
Bug 70004: [EFL] Change the behavior of smart_zoom.
https://bugs.webkit.org/show_bug.cgi?id=70004

Attachment 110863: Patch
https://bugs.webkit.org/attachment.cgi?id=110863&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
Some of my thoughts here may be a bit late now that r97043 is already in, but I
feel the discussion is still important.

Bug 62842 renamed the current existing zooming functions in ewk_view and added
another function to scale pages. So far, so good.

However, I still don't buy the argument for: 

a) Removing ewk_view_zoom_set
It is a much more predictable name for zooming, the smart object has a
smart_zoom_set member and in the end if one wants to zoom, ewk_view_zoom_set
makes much more sense.

b) Moving most of its behaviour to ewk_view_scale_set while making scaling the
default zooming algorithm.
https://bugs.webkit.org/show_bug.cgi?id=62842#c9 said: "Not everyone is mobile
and want to scale page on zoom. How about desktop users that want text-only
zoom? How about regular zoom that uses proper font size instead of scaled
vectors? Users of low-end hardware, like some TVs will be hurt by such
behavior". From what I see, this statement was not answered and this patch
makes scaling the default zooming algorithm. The reason given in the ChangeLog
was that "smart_zoom is for ewk_tiled_backing_store to support proportional
scaling"; if this is the sole reason for changing the default behaviour here, I
wonder if it does not make sense to fix it elsewhere and not change the current
expectations.

I suggest the following:

a) Keep ewk_view_zoom_{get,set} with their current behaviour untouched.
b) Verify whether the centering code currently in ewk_view_scale_set is really
needed. I suspect just calling Page::setPageScaleFactor without any adjustment
should work just fine.
c) Change the tiled backing store to call the scaling code if it is indeed
better for it in terms of performance.