Bug 130391 - [EFL] [WK2] Replacing zoom functionality of MiniBrowser from scale_set to page_zoom_set
Summary: [EFL] [WK2] Replacing zoom functionality of MiniBrowser from scale_set to pag...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-18 05:27 PDT by Andrzej Badowski
Modified: 2014-06-04 01:35 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (5.44 KB, patch)
2014-03-25 09:49 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 2 (5.64 KB, patch)
2014-03-27 09:04 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff
proposed patch 3 (5.19 KB, patch)
2014-04-01 09:30 PDT, Andrzej Badowski
gyuyoung.kim: review-
Details | Formatted Diff | Diff
proposed patch 4 (5.54 KB, patch)
2014-04-02 09:24 PDT, Andrzej Badowski
gyuyoung.kim: review-
Details | Formatted Diff | Diff
proposed patch 5 (5.55 KB, patch)
2014-04-03 01:19 PDT, Andrzej Badowski
gyuyoung.kim: review+
Details | Formatted Diff | Diff
proposed patch 6 (5.55 KB, patch)
2014-04-03 06:57 PDT, Andrzej Badowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrzej Badowski 2014-03-18 05:27:31 PDT
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.
Comment 1 Andrzej Badowski 2014-03-19 03:32:55 PDT
3)In contrast to chromium browser and mozilla browser window dimension change in the above circumstances will, that there is no left margin
Comment 2 Andrzej Badowski 2014-03-25 09:49:22 PDT
Created attachment 227762 [details]
proposed patch

Fixing described bugs.
Comment 3 Jinwoo Song 2014-03-25 16:33:23 PDT
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
Comment 4 Ryuan Choi 2014-03-25 17:35:04 PDT
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.
Comment 5 Andrzej Badowski 2014-03-26 02:53:32 PDT
(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.
Comment 6 Andrzej Badowski 2014-03-26 03:21:51 PDT
(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.
Comment 7 Andrzej Badowski 2014-03-27 09:04:21 PDT
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.
Comment 8 Ryuan Choi 2014-03-27 17:43:00 PDT
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).
Comment 9 Andrzej Badowski 2014-04-01 09:30:37 PDT
Created attachment 228294 [details]
proposed patch 3
Comment 10 Gyuyoung Kim 2014-04-01 17:58:50 PDT
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.
Comment 11 Andrzej Badowski 2014-04-02 09:24:58 PDT
Created attachment 228401 [details]
proposed patch 4
Comment 12 Jinwoo Song 2014-04-02 17:01:31 PDT
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
Comment 13 Gyuyoung Kim 2014-04-02 18:46:25 PDT
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.
Comment 14 Andrzej Badowski 2014-04-03 01:19:09 PDT
Created attachment 228487 [details]
proposed patch 5

Thank you for review. 
There's a new patch.
Comment 15 Gyuyoung Kim 2014-04-03 06:11:45 PDT
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].
Comment 16 Andrzej Badowski 2014-04-03 06:57:56 PDT
Created attachment 228505 [details]
proposed patch 6

Thank you for review.
This is a patch to commit.
Comment 17 WebKit Commit Bot 2014-04-03 08:33:37 PDT
Comment on attachment 228505 [details]
proposed patch 6

Clearing flags on attachment: 228505

Committed r166718: <http://trac.webkit.org/changeset/166718>
Comment 18 WebKit Commit Bot 2014-04-03 08:33:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Csaba Osztrogonác 2014-06-04 00:52:47 PDT
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).
Comment 20 Andrzej Badowski 2014-06-04 01:35:28 PDT
(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.