Bug 70004 - [EFL] Change the behavior of smart_zoom.
Summary: [EFL] Change the behavior of smart_zoom.
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on: 70078
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-12 23:59 PDT by Ryuan Choi
Modified: 2011-10-16 17:34 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.53 KB, patch)
2011-10-13 00:02 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (9.51 KB, patch)
2011-10-13 00:31 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (10.37 KB, patch)
2011-10-13 10:09 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2011-10-12 23:59:37 PDT
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.
Comment 1 Ryuan Choi 2011-10-13 00:02:41 PDT
Created attachment 110803 [details]
Patch
Comment 2 Ryuan Choi 2011-10-13 00:31:32 PDT
Created attachment 110807 [details]
Patch
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-10-13 05:49:29 PDT
Comment on attachment 110807 [details]
Patch

LGTM if zooming (the animations, etc) still works fine after this change.
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-10-13 06:56:10 PDT
Oh, and please adjust Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp as well.
Comment 5 Ryuan Choi 2011-10-13 10:09:47 PDT
Created attachment 110863 [details]
Patch
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-10-13 11:01:59 PDT
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.
Comment 7 Ryuan Choi 2011-10-13 16:43:41 PDT
(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.
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-10-14 04:36:15 PDT
OK, so there's no need for this patch anymore?
Comment 9 Ryuan Choi 2011-10-16 17:34:19 PDT
(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.