Bug 62842 – [EFL] Refactor zoom related APIs.

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

[Ryuan Choi]

I want to use Frame::scalePage for zooming without layout changes.
It will help user experience in mobile domain.

However, WebKit/EFL already have some APIs for zooming.
ewk_frame_zoom_set is for page zoom and text zoom.
ewk_view_zoom_set is same as ewk_frame_zoom_set without moving center coordination.

So, I believe that changing behaviors of ewk_view_zoom_set is better than introducing new APIs.
This will also seperate ewk_view_zoom_set from ewk_frame_zoom_set.

[Ryuan Choi]

Created attachment 97542 [details]
Patch

[Ryuan Choi]

I hope that it replace cairo-scaling.

how do you think about it?

[Kenneth Rohde Christiansen]

You should talk the guy who did the tiling for EFL. Maybe it was Gustavo Barbieri or one of his coworkers.

Do you have any testing system to test if a change like this regress? I think that would be well worth working on.

[Ryuan Choi]

(In reply to comment #3)
> You should talk the guy who did the tiling for EFL. Maybe it was Gustavo Barbieri or one of his coworkers.

Yes, I believe that Rafael Antognolli did it.
I am waiting his or other profusion guy's comment.

> 
> Do you have any testing system to test if a change like this regress? I think that would be well worth working on.

In this case, it will change behavior of ewk_view_zoom_set.
It is almost similar but it will make different rendered results.
So, I think that it's not regression area.

Anyway, we have some regression tools for basic behaviors and APIs although it was not enough to open.
(and also I heard that profusion guys are preparing DRT on EFL port.)

Thanks.

[Raphael Kubo da Costa (:rakuco)]

Let's come back to this one at last.

Having API in ewk_view and ewk_frame with the same name but with different behavior does not sound very sensible to me -- either we make all the calls perform the same type of zoom/scaling, or we change the API to avoid confusions.

As as mentioned before, it'd be good to verify if the math in the patch is also right: all those multiplications were there so that the the page was kept pretty much at the same point after zooming. Does that still happen?

[Eric Seidel (no email)]

Comment on attachment 97542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97542&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:892
> +    Evas_Coord x, y;

No.  Please use C++.

[Kenneth Rohde Christiansen]

Comment on attachment 97542 [details]
Patch

I am agreeing with Eric here, this code is pretty hard to read and not so WebKit'ish. I understand that you want your public API to be in C, but isn't it possible to at least have all the private implementations written in C++? And I am also pretty sure that you should be able to replace EINA_FALSE with false without any issues.

[Ryuan Choi]

(In reply to comment #5)
> Let's come back to this one at last.
> 
> Having API in ewk_view and ewk_frame with the same name but with different behavior does not sound very sensible to me -- either we make all the calls perform the same type of zoom/scaling, or we change the API to avoid confusions.
> 

Sorry for late answer.

If then, how do you think about removing ewk_view_zoom_set/get and adding ewk_view_scale_set/get?

> As as mentioned before, it'd be good to verify if the math in the patch is also right: all those multiplications were there so that the the page was kept pretty much at the same point after zooming. Does that still happen?

Yes, at least, there are no issues related to position.

(In reply to comment #7)
> (From update of attachment 97542 [details])
> I am agreeing with Eric here, this code is pretty hard to read and not so WebKit'ish. I understand that you want your public API to be in C, but isn't it possible to at least have all the private implementations written in C++? And I am also pretty sure that you should be able to replace EINA_FALSE with false without any issues.

I tried to keep EFL style for EFL specific files although WebKit style is clear for me.
I'll prepare patch like you and eric mentioned.

Thanks.

[Gustavo Sverzut Barbieri]

r-

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.

Given the way we already do by having "ewk_view_zoom_text_only_set()", we could remove it and make an enum of zoom modes.

Or introduce another ewk_{view,frame}_zoom_scaled_set() and note that is just applied if text_only is False. Effectively ewk_frame_zoom_set() would become:


    if (sd->textZoom)
        sd->frame->setTextZoomFactor(zoom);
    else if (sd->scaledZoom)
        sd->frame->scalePage(zoom, WebCore::IntPoint(x, y));
    else
        sd->frame->setPageZoomFactor(zoom);

ewk_frame_zoom_set() is not widely used (its usage is discouraged for regular users) and thus can start to take x,y parameters if you think it's necessary.

[Ryuan Choi]

(In reply to comment #9)
> r-
> 
> 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.
> 

I don't want to remove text-only zoom and page zoom.
scale page is additional feature.

> Given the way we already do by having "ewk_view_zoom_text_only_set()", we could remove it and make an enum of zoom modes.
> 
> Or introduce another ewk_{view,frame}_zoom_scaled_set() and note that is just applied if text_only is False. Effectively ewk_frame_zoom_set() would become:
> 
> 
>     if (sd->textZoom)
>         sd->frame->setTextZoomFactor(zoom);
>     else if (sd->scaledZoom)
>         sd->frame->scalePage(zoom, WebCore::IntPoint(x, y));
>     else
>         sd->frame->setPageZoomFactor(zoom);
> 

Basically, It make sense for me. I'll do that.

However,
IMO, ewk_view_zoom_text_only_set() limit zoom feature.
For example, we can't get 2.0 page zoomed and 3.0 text zoomed contents.

I prefer to add enum parameter in ewk_{view, frame}_zoom_get() with removing ewk_view_zoom_text_only_{get,set}().

I know that API changes is not good.
I'll prepare new patch after getting opinion from webkit-efl mailing list.

> ewk_frame_zoom_set() is not widely used (its usage is discouraged for regular users) and thus can start to take x,y parameters if you think it's necessary.

[Gustavo Sverzut Barbieri]

Page scale is just another way of zooming. Actually it's the most logical way, just the most inefficient. :-)

Text-zoom is something that goes bit far from what people understand as zoom, but it is very useful for people with eye problems or different media. For instance, viewing a webpage on TV: you can spot icons and such, but hardly you can read a phrase with regular font size and want it doubled or so.

About your proposal: one more parameter to zoom_set() is bad. What do you get when you zoom_get()? Will be confusing for sure!

When I designed zoom_set() to operate on one of actual zoom or text I imagined the user would use just one or another. But as you mentioned the case of using both is reasonable.

If we're changing our API, why not change it for good and keep 1:1 with WebKit? If so then (with matching getters, of course):

   - ewk_{view,frame}_scale_set(o, factor, center_x, center_y);
   - ewk_{view,frame}_text_zoom_set(o, factor);
   - ewk_{view,frame}_zoom_set(o, factor).

I removed center point from regular zoom as it will not make much sense. Hardly the page contents will match (paragraphs will re-flow, etc) and thus not of much help.

Code in WebKit-EFL will be minimal, just C<->C++ bridge and the behavior will be consistent. Browsers will need to be changed, but that is very simple.

[Ryuan Choi]

Created attachment 109727 [details]
Patch

[Ryuan Choi]

(In reply to comment #12)
> Created an attachment (id=109727) [details]
> Patch

I changed patch like gustavo mentioned.

[Gyuyoung Kim]

Comment on attachment 109727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109727&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:1686
> +        priv->animated_zoom.zoom.start = ewk_view_scale_get(o);

In, Bug 69073, o can be changed with ewkView.

> Source/WebKit/efl/ewk/ewk_view.cpp:1724
> +    cur_zoom = ewk_view_scale_get(o);

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1763
> +    cur_zoom = ewk_view_scale_get(o);

ditto.

[Ryuan Choi]

(In reply to comment #14)
> (From update of attachment 109727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109727&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1686
> > +        priv->animated_zoom.zoom.start = ewk_view_scale_get(o);
> 
> In, Bug 69073, o can be changed with ewkView.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1724
> > +    cur_zoom = ewk_view_scale_get(o);
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1763
> > +    cur_zoom = ewk_view_scale_get(o);
> 
> ditto.

Right, but I think that this is fine until Bug 69073 merged.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 109727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109727&action=review

Informal r- here.

The ChangeLog is very incomplete -- this patch introduces a lot of changes, and if I were a reviewer who had not been closely paying attention to the whole discussion, I wouldn't really understand why these changes were made and what exactly is being changed by just looking at the ChangeLog.

Besides that, two separate things are being done at once here -- the refactoring of the APIs and the use of a different zoom method when zooming a view (this change is not mentioned in the ChangeLog at all). I'd rather do them separately.

> Source/WebKit/efl/ChangeLog:32
> +        * ewk/ewk_frame.cpp:
> +        (ewk_frame_text_zoom_get):
> +        (ewk_frame_text_zoom_set):
> +        (ewk_frame_zoom_get):
> +        (ewk_frame_zoom_set):
> +        * ewk/ewk_frame.h:
> +        * ewk/ewk_view.cpp:
> +        (_ewk_view_smart_scale_set):
> +        (_ewk_view_zoom_animator_cb):
> +        (ewk_view_base_smart_set):
> +        (ewk_view_scale_get):
> +        (ewk_view_scale_set):
> +        (ewk_view_text_zoom_get):
> +        (ewk_view_text_zoom_set):
> +        (ewk_view_zoom_get):
> +        (ewk_view_zoom_set):
> +        (ewk_view_zoom_weak_set):
> +        (ewk_view_zoom_animated_set):
> +        (ewk_view_pre_render_region):
> +        (ewk_view_pre_render_relative_radius):
> +        * ewk/ewk_view.h:

You've made a lot of changes, please describe them accordingly here.

> Source/WebKit/efl/ewk/ewk_view.cpp:931
> +    x = static_cast<double>(x + cx) / currentScaleFactor * scaleFactor - cx;
> +    y = static_cast<double>(y + cy) / currentScaleFactor * scaleFactor - cy;

Use parentheses in the expressions so we don't get confused trying to parse the precedence of the operators here.

> Source/WebKit/efl/ewk/ewk_view.cpp:933
> +    priv->page->setPageScaleFactor(scaleFactor, WebCore::LayoutPoint(x, y));

Please add the appropriate include for the LayoutPoint typedef.

> Source/WebKit/efl/ewk/ewk_view.cpp:1563
> +float ewk_view_text_zoom_get(const Evas_Object* o)

o -> ewkView?

> Source/WebKit/efl/ewk/ewk_view.cpp:1569
> +Eina_Bool ewk_view_text_zoom_set(Evas_Object* o, float textZoomFactor)

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1575
> +float ewk_view_zoom_get(const Evas_Object* o)

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:1581
> +Eina_Bool ewk_view_zoom_set(Evas_Object* o, float zoomFactor)

ditto.

[Ryuan Choi]

(In reply to comment #16)
> (From update of attachment 109727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109727&action=review
> 
> Informal r- here.
> 
> The ChangeLog is very incomplete -- this patch introduces a lot of changes, and if I were a reviewer who had not been closely paying attention to the whole discussion, I wouldn't really understand why these changes were made and what exactly is being changed by just looking at the ChangeLog.

OK, I'll try to fill more information.

> 
> Besides that, two separate things are being done at once here -- the refactoring of the APIs and the use of a different zoom method when zooming a view (this change is not mentioned in the ChangeLog at all). I'd rather do them separately.

Agree.
If then, I can split this -- 1) Refactoring current zoom related APIs. 2) Introduce ewk_view_scale_{get|set} and change zoom method when zooming a view.

I'll update first one.

> 
> > Source/WebKit/efl/ChangeLog:32
> > +        * ewk/ewk_frame.cpp:
> > +        (ewk_frame_text_zoom_get):
> > +        (ewk_frame_text_zoom_set):
> > +        (ewk_frame_zoom_get):
> > +        (ewk_frame_zoom_set):
> > +        * ewk/ewk_frame.h:
> > +        * ewk/ewk_view.cpp:
> > +        (_ewk_view_smart_scale_set):
> > +        (_ewk_view_zoom_animator_cb):
> > +        (ewk_view_base_smart_set):
> > +        (ewk_view_scale_get):
> > +        (ewk_view_scale_set):
> > +        (ewk_view_text_zoom_get):
> > +        (ewk_view_text_zoom_set):
> > +        (ewk_view_zoom_get):
> > +        (ewk_view_zoom_set):
> > +        (ewk_view_zoom_weak_set):
> > +        (ewk_view_zoom_animated_set):
> > +        (ewk_view_pre_render_region):
> > +        (ewk_view_pre_render_relative_radius):
> > +        * ewk/ewk_view.h:
> 
> You've made a lot of changes, please describe them accordingly here.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:931
> > +    x = static_cast<double>(x + cx) / currentScaleFactor * scaleFactor - cx;
> > +    y = static_cast<double>(y + cy) / currentScaleFactor * scaleFactor - cy;
> 
> Use parentheses in the expressions so we don't get confused trying to parse the precedence of the operators here.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:933
> > +    priv->page->setPageScaleFactor(scaleFactor, WebCore::LayoutPoint(x, y));
> 
> Please add the appropriate include for the LayoutPoint typedef.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1563
> > +float ewk_view_text_zoom_get(const Evas_Object* o)
> 
> o -> ewkView?
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1569
> > +Eina_Bool ewk_view_text_zoom_set(Evas_Object* o, float textZoomFactor)
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1575
> > +float ewk_view_zoom_get(const Evas_Object* o)
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1581
> > +Eina_Bool ewk_view_zoom_set(Evas_Object* o, float zoomFactor)
> 
> ditto.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #17)
> Agree.
> If then, I can split this -- 1) Refactoring current zoom related APIs. 2) Introduce ewk_view_scale_{get|set} and change zoom method when zooming a view.

I'm OK with introducing the scale functions in 1) (I even think it makes more sense), it's just the behaviour change in the smart zoom function that should be done separately.

BTW, you guys were discussing the naming some time ago. IMO, ewk_{frame,view}_{page_zoom,text_zoom,page_scale}_{get,set} look good to me.

[Ryuan Choi]

Created attachment 109981 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 109981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109981&action=review

> Source/WebKit/efl/ChangeLog:11
> +        In addition, introduce ewk_view_scale_{get|set} to support proportioanl scaling.

proportioanl -> proportional

> Source/WebKit/efl/ChangeLog:13
> +        ewk_view_zoom_{get|set} will be remained until the behavior of smart_zoom

will be remained -> will remain unchanged

> Source/WebKit/efl/ChangeLog:36
> +        (ewk_view_zoom_weak_set):
> +        (ewk_view_zoom_animated_set):
> +        (ewk_view_pre_render_region):
> +        (ewk_view_pre_render_relative_radius):

It'd be nice to indicate what's changed in these functions.

> Source/WebKit/efl/ewk/ewk_view.cpp:1605
> +    x = (static_cast<double>(x + cx) / currentScaleFactor * scaleFactor) - cx;
> +    y = (static_cast<double>(y + cy) / currentScaleFactor * scaleFactor) - cy;

Please also add another level of parentheses to indicate the precedence order between '/' and '*'.

[Ryuan Choi]

Created attachment 110067 [details]
Patch

[Ryuan Choi]

(In reply to comment #20)
> (From update of attachment 109981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109981&action=review
> 
> > Source/WebKit/efl/ChangeLog:11
> > +        In addition, introduce ewk_view_scale_{get|set} to support proportioanl scaling.
> 
> proportioanl -> proportional
Sorry for the typo. fixed.

> 
> > Source/WebKit/efl/ChangeLog:13
> > +        ewk_view_zoom_{get|set} will be remained until the behavior of smart_zoom
> 
> will be remained -> will remain unchanged
fixed.

> 
> > Source/WebKit/efl/ChangeLog:36
> > +        (ewk_view_zoom_weak_set):
> > +        (ewk_view_zoom_animated_set):
> > +        (ewk_view_pre_render_region):
> > +        (ewk_view_pre_render_relative_radius):
> 
> It'd be nice to indicate what's changed in these functions.
Done.

> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1605
> > +    x = (static_cast<double>(x + cx) / currentScaleFactor * scaleFactor) - cx;
> > +    y = (static_cast<double>(y + cy) / currentScaleFactor * scaleFactor) - cy;
> 
> Please also add another level of parentheses to indicate the precedence order between '/' and '*'.

I'm not sure that many parentheses help readability.
But fixed like you mentioned.

[Gyuyoung Kim]

Comment on attachment 110067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110067&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:1604
> +    x = ((static_cast<double>(x + cx) / currentScaleFactor) * scaleFactor) - cx;

Why do you cast data type as 'double' ? currentScaleFactor and scaleFactor are float. In addition, are you sure a cast isn't required here to force integer point arithmetic?

> Source/WebKit/efl/ewk/ewk_view.cpp:1605
> +    y = ((static_cast<double>(y + cy) / currentScaleFactor) * scaleFactor) - cy;

ditto.

[Ryuan Choi]

Created attachment 110071 [details]
Patch

[Ryuan Choi]

(In reply to comment #23)
> (From update of attachment 110067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110067&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1604
> > +    x = ((static_cast<double>(x + cx) / currentScaleFactor) * scaleFactor) - cx;
> 
> Why do you cast data type as 'double' ? currentScaleFactor and scaleFactor are float. In addition, are you sure a cast isn't required here to force integer point arithmetic?
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1605
> > +    y = ((static_cast<double>(y + cy) / currentScaleFactor) * scaleFactor) - cy;
> 
> ditto.

Right, I removed casting.
Thanks.

[Gyuyoung Kim]

Comment on attachment 110071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110071&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:1604
> +    x = (((x + cx) / currentScaleFactor) * scaleFactor) - cx;

AFAIK, Evas_Coord is int type. So, I think that (((x + cx) / currentScaleFactor) * scaleFactor) is floating type. Should we cast  (((x + cx) / currentScaleFactor) * scaleFactor) to int explicitly as below ?
static_cast<int>(((x + cx) / currentScaleFactor) * scaleFactor) - cx

> Source/WebKit/efl/ewk/ewk_view.cpp:1605
> +    y = (((y + cy) / currentScaleFactor) * scaleFactor) - cy;

ditto.

[Ryuan Choi]

Created attachment 110076 [details]
Patch

[Ryuan Choi]

(In reply to comment #26)
> (From update of attachment 110071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110071&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1604
> > +    x = (((x + cx) / currentScaleFactor) * scaleFactor) - cx;
> 
> AFAIK, Evas_Coord is int type. So, I think that (((x + cx) / currentScaleFactor) * scaleFactor) is floating type. Should we cast  (((x + cx) / currentScaleFactor) * scaleFactor) to int explicitly as below ?
> static_cast<int>(((x + cx) / currentScaleFactor) * scaleFactor) - cx
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:1605
> > +    y = (((y + cy) / currentScaleFactor) * scaleFactor) - cy;
> 
> ditto.

Thanks, done.

[Gyuyoung Kim]

Comment on attachment 110076 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110076&action=review

> Source/WebKit/efl/ewk/ewk_view.h:991
> +EAPI Eina_Bool    ewk_view_page_zoom_set(Evas_Object *o, float pageZoomFactor);

We are using efl coding style in public APIs despite some APIs don't adhere this rule. So, I think pageZoomFactor need to be changed with page_zoom_factor. Of course, variable name is gonna be discussed as one of coding style change.

> Source/WebKit/efl/ewk/ewk_view.h:1031
> +EAPI Eina_Bool    ewk_view_text_zoom_set(Evas_Object *o, float textZoomFactor);

ditto.

[Ryuan Choi]

Created attachment 110086 [details]
Patch

[Ryuan Choi]

(In reply to comment #29)
> (From update of attachment 110076 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110076&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:991
> > +EAPI Eina_Bool    ewk_view_page_zoom_set(Evas_Object *o, float pageZoomFactor);
> 
> We are using efl coding style in public APIs despite some APIs don't adhere this rule. So, I think pageZoomFactor need to be changed with page_zoom_factor. Of course, variable name is gonna be discussed as one of coding style change.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:1031
> > +EAPI Eina_Bool    ewk_view_text_zoom_set(Evas_Object *o, float textZoomFactor);
> 
> ditto.

OK, fixed.
For me, pageZoomFactor looks better.
But, as you mentioned, it can be covered as other issue.

[Gyuyoung Kim]

Comment on attachment 110086 [details]
Patch

LGTM.

[Raphael Kubo da Costa (:rakuco)]

Informal r+ from my side too.

[WebKit Review Bot]

Comment on attachment 110086 [details]
Patch

Clearing flags on attachment: 110086

Committed r97043: <http://trac.webkit.org/changeset/97043>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.