Bug 130391 – [EFL] [WK2] Replacing zoom functionality of MiniBrowser from scale_set to page_zoom_set

• Component: UI Events
• Status: RESOLVED FIXED

[Andrzej Badowski]

After loading stackoverflow.com and several times zoom out (Ctrl - MiniBrowser, such as zoom 0.67), the subsequent increase or decrease of the width of the entire window shows the following errors: 
1) dark-gray rectangles in the header and footer decrease / increase disproportionately in relation to changes in the width of the window. 
2) a significant reduction of the width of the window, but the window is large enough for it to fit the contents of the page, a large margin arises on the right side and the content of the web site is truncated.

None of these errors do not arise in the chromium browser or the mozilla browser. These dark-gray rectangles occupy the entire width of the browser window.

[Andrzej Badowski]

3)In contrast to chromium browser and mozilla browser window dimension change in the above circumstances will, that there is no left margin

[Andrzej Badowski]

Created attachment 227762 [details]
proposed patch

Fixing described bugs.

[Jinwoo Song]

Comment on attachment 227762 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:240
> +Eina_Bool ewk_view_zoom_set(Evas_Object* ewkView, double zoomFactor)

It would be clear to name as *ewk_view_page_zoom_factor_set*.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:248
> +double ewk_view_zoom_get(const Evas_Object* ewkView)

It would be clear to name as *ewk_view_page_zoom_factor_get*.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:576
> + * Sets zoom of the current page.

s/zoom/zoom factor/

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:583
> +EAPI Eina_Bool ewk_view_zoom_set(Evas_Object *o, double zoomFactor);

s/zoomFactor/zoom_factor

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:586
> + * Queries the current zom factor of the page.

typo: s/zom/zoom

[Ryuan Choi]

Comment on attachment 227762 [details]
proposed patch

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

> Source/WebKit2/ChangeLog:9
> +        Fixing an improper zooming in EFL WK2 by using WKPageSetPageZoomFactor via ewk_view_zoom_set
> +        instead WKPageSetScaleFactor via ewk_view_scale_set.

page_zoom_set and scale_set is based on different requirement.

I don't object to introduce page_zoom_set but I don't want to avoid a bug of scale_set if it has.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:240
>> +Eina_Bool ewk_view_zoom_set(Evas_Object* ewkView, double zoomFactor)
> 
> It would be clear to name as *ewk_view_page_zoom_factor_set*.

We already use ewk_view_page_zoom_set for ewebkit.

If it is not important, I'd like to keep the consistency.

[Andrzej Badowski]

(In reply to comment #3)
> (From update of attachment 227762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227762&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:240
> > +Eina_Bool ewk_view_zoom_set(Evas_Object* ewkView, double zoomFactor)
> 
> It would be clear to name as *ewk_view_page_zoom_factor_set*.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:248
> > +double ewk_view_zoom_get(const Evas_Object* ewkView)
> 
> It would be clear to name as *ewk_view_page_zoom_factor_get*.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:576
> > + * Sets zoom of the current page.
> 
> s/zoom/zoom factor/
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:583
> > +EAPI Eina_Bool ewk_view_zoom_set(Evas_Object *o, double zoomFactor);
> 
> s/zoomFactor/zoom_factor
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:586
> > + * Queries the current zom factor of the page.
> 
> typo: s/zom/zoom

There is no problem other names for the proposed functions, especially if it will help keep consistency.

[Andrzej Badowski]

(In reply to comment #4)
> (From update of attachment 227762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227762&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        Fixing an improper zooming in EFL WK2 by using WKPageSetPageZoomFactor via ewk_view_zoom_set
> > +        instead WKPageSetScaleFactor via ewk_view_scale_set.
> 
> page_zoom_set and scale_set is based on different requirement.
> 
> I don't object to introduce page_zoom_set but I don't want to avoid a bug of scale_set if it has.
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:240
> >> +Eina_Bool ewk_view_zoom_set(Evas_Object* ewkView, double zoomFactor)
> > 
> > It would be clear to name as *ewk_view_page_zoom_factor_set*.
> 
> We already use ewk_view_page_zoom_set for ewebkit.
> 
> If it is not important, I'd like to keep the consistency.

There is no problem other names for the proposed functions, especially if it will help keep consistency.

(In reply to comment #4)
> (From update of attachment 227762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227762&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        Fixing an improper zooming in EFL WK2 by using WKPageSetPageZoomFactor via ewk_view_zoom_set
> > +        instead WKPageSetScaleFactor via ewk_view_scale_set.
> 
> page_zoom_set and scale_set is based on different requirement.
> 
> I don't object to introduce page_zoom_set but I don't want to avoid a bug of scale_set if it has.
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:240
> >> +Eina_Bool ewk_view_zoom_set(Evas_Object* ewkView, double zoomFactor)
> > 
> > It would be clear to name as *ewk_view_page_zoom_factor_set*.
> 
> We already use ewk_view_page_zoom_set for ewebkit.
> 
> If it is not important, I'd like to keep the consistency.

There is no problem other names for the proposed functions, especially if it will help keep consistency.
As for the error function ewk_view_scale_set, it is like a slightly different problem than the solution to the reported error. I suggest turn it off as another reported error on webkit.org. I'll try to define this bug.

[Andrzej Badowski]

Created attachment 227949 [details]
proposed patch 2

Changing of the function names according to opinions of Ryuan Choi and Jinwoo Song.
The problem of a defective function ewk_view_scale_set I enrolled as a new bug 130838.

[Ryuan Choi]

Comment on attachment 227949 [details]
proposed patch 2

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

> Source/WebKit2/ChangeLog:3
> +        [EFL] Page rendering is not proper when zooming and resizing.

I am still not sure whether this patch is to fix "Page rendering is not proper when zooming and resizing".

Let's change the bug title properly.
And [EFL][WK2] is correct prefix, IMO.

> Source/WebKit2/ChangeLog:13
> +        (ewk_view_page_zoom_factor_set):
> +        (ewk_view_page_zoom_factor_get):

Sorry, after discussed with jinwoo.
We'd better to use ewk_view_page_zoom_set/get.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:576
> + * Sets zoom of the current page.

s/zoom/zoom factor/

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:583
> +EAPI Eina_Bool ewk_view_page_zoom_factor_set(Evas_Object *o, double zoomFactor);

s/zoomFactor/zoom_factor/

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:586
> + * Queries the current zom factor of the page.

s/zom/zoom/

> Tools/ChangeLog:9
> +        Fixing an improper zooming in EFL WK2 by using WKPageSetPageZoomFactor via ewk_view_page_zoom_factor_set
> +        instead WKPageSetScaleFactor via ewk_view_scale_set.

This is not fixing improper zooming in EFL WK2.
It's replacing zoom functionality of MiniBrowser from scale_set to page_zoom_set (maybe: because it looks more desired behaviour of zoom_level_set).

[Andrzej Badowski]

Created attachment 228294 [details]
proposed patch 3

[Gyuyoung Kim]

Comment on attachment 228294 [details]
proposed patch 3

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

> Source/WebKit2/ChangeLog:7
> +

Missing patch description.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:583
> +EAPI Eina_Bool ewk_view_page_zoom_set(Evas_Object *o, double zoomFactor);

Wrong EFL parameter style. Look at https://trac.webkit.org/wiki/EFLWebKitCodingStyle

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:1102
> ++

Remove +

> Tools/ChangeLog:7
> +

ditto.

[Andrzej Badowski]

Created attachment 228401 [details]
proposed patch 4

[Jinwoo Song]

Comment on attachment 228401 [details]
proposed patch 4

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

> Source/WebKit2/ChangeLog:9
> +        to ewk_view_page_zoom_set. Adding to the API functions: ewk_view_page_set and

s/ewk_view_page_set/ewk_view_page_zoom_set

> Source/WebKit2/ChangeLog:10
> +        ewk_view_page_get to call appropriate WK functions.

s/ewk_view_page_get/ewk_view_page_zoom_get

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:586
> + * Queries the current zom factor of the page.

typo; s/zom/zoom

[Gyuyoung Kim]

Comment on attachment 228401 [details]
proposed patch 4

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

r- due to some nits.

> Source/WebKit2/ChangeLog:3
> +        [EFL] [WK2] Replacing zoom functionality of MiniBrowser from scale_set to page_zoom_set.

Unnecessary a space between [EFL] and [WK2]

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:245
> +    WKPageSetPageZoomFactor(impl->wkPage(), zoomFactor);

Add a new line.

[Andrzej Badowski]

Created attachment 228487 [details]
proposed patch 5

Thank you for review. 
There's a new patch.

[Gyuyoung Kim]

Comment on attachment 228487 [details]
proposed patch 5

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

LGTM.

> Tools/ChangeLog:3
> +        [EFL] [WK2] Replacing zoom functionality of MiniBrowser from scale_set to page_zoom_set.

Unneeded space between [EFL] [WK2].

[Andrzej Badowski]

Created attachment 228505 [details]
proposed patch 6

Thank you for review.
This is a patch to commit.

[WebKit Commit Bot]

Comment on attachment 228505 [details]
proposed patch 6

Clearing flags on attachment: 228505

Committed r166718: <http://trac.webkit.org/changeset/166718>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 227949 [details]
proposed patch 2

Cleared review? from obsolete attachment 227949 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Andrzej Badowski]

(In reply to comment #19)
> (From update of attachment 227949 [details])
> Cleared review? from obsolete attachment 227949 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

I'm sorry for the trouble I had apparently left the patch with the ? sign.