Summary: | [EFL] [WK2] Replacing zoom functionality of MiniBrowser from scale_set to page_zoom_set | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrzej Badowski <a.badowski> | ||||||||||||||
Component: | UI Events | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gyuyoung.kim, hw1008.kim, sergio | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Attachments: |
|
Description
Andrzej Badowski
2014-03-18 05:27:31 PDT
3)In contrast to chromium browser and mozilla browser window dimension change in the above circumstances will, that there is no left margin Created attachment 227762 [details]
proposed patch
Fixing described bugs.
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 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. (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. (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. 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 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). Created attachment 228294 [details]
proposed patch 3
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. Created attachment 228401 [details]
proposed patch 4
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 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. Created attachment 228487 [details]
proposed patch 5
Thank you for review.
There's a new patch.
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]. Created attachment 228505 [details]
proposed patch 6
Thank you for review.
This is a patch to commit.
Comment on attachment 228505 [details] proposed patch 6 Clearing flags on attachment: 228505 Committed r166718: <http://trac.webkit.org/changeset/166718> All reviewed patches have been landed. Closing bug. 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). (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. |