Bug 70004 – [EFL] Change the behavior of smart_zoom.

• Status: RESOLVED
• Resolution: LATER
• Component: WebKit EFL

[Ryuan Choi]

IMO, smart_zoom of ewk_view is for ewk_tiled_backing_store to support proportional scaling.
So, Using ewk_view_scale_set is better than ewk_frame_page_zoom_set.

[Ryuan Choi]

Created attachment 110803 [details]
Patch

[Ryuan Choi]

Created attachment 110807 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 110807 [details]
Patch

LGTM if zooming (the animations, etc) still works fine after this change.

[Raphael Kubo da Costa (:rakuco)]

Oh, and please adjust Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp as well.

[Ryuan Choi]

Created attachment 110863 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 110863 [details]
Patch

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.

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 110863 [details])
> 
> 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.

I agree.
If then, I'll create new bug to remove centering code from ewk_view_scale_set.

> c) Change the tiled backing store to call the scaling code if it is indeed better for it in terms of performance.

For the performance, Page::setPageScaleFactor and page-zoom are not comparable.
It probably depends on contents.

But, I can say;
1) Performance between setPageScaleFactor and cairo scaled patch is almost same.
(IIRC, cairo scaled patch is not contributed, because of may webcore changes.)
2) page zoom can't support proportional scaling.

[Raphael Kubo da Costa (:rakuco)]

OK, so there's no need for this patch anymore?

[Ryuan Choi]

(In reply to comment #8)
> OK, so there's no need for this patch anymore?

I still want this changes, but I need to investigate little bit more.
So I will close and reopen if needed.