Bug 31893 - Simplify Chromium embedding APIs for zoom
Summary: Simplify Chromium embedding APIs for zoom
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-25 16:26 PST by Peter Kasting
Modified: 2009-12-04 15:56 PST (History)
2 users (show)

See Also:


Attachments
patch v1 (5.46 KB, patch)
2009-11-25 16:40 PST, Peter Kasting
fishd: review-
Details | Formatted Diff | Diff
patch v2 (5.47 KB, patch)
2009-12-01 13:51 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Remove deprecated APIs (3.61 KB, patch)
2009-12-01 18:23 PST, Peter Kasting
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-11-25 16:26:03 PST
Needed by http://crbug.com/567 .  In particular we need the ability to set a zoom level directly, not just make relative changes; replacing the existing API with a single "get" and "set" pair will shorten the code and allow this.
Comment 1 Peter Kasting 2009-11-25 16:40:29 PST
Created attachment 43878 [details]
patch v1
Comment 2 Adam Barth 2009-11-30 12:41:38 PST
style-queue ran check-webkit-style on attachment 43878 [details] without any errors.
Comment 3 Darin Fisher (:fishd, Google) 2009-12-01 12:41:44 PST
Comment on attachment 43878 [details]
patch v1

> Index: WebKit/chromium/public/WebView.h
...
> -    virtual void zoomIn(bool textOnly) = 0;
> -    virtual void zoomOut(bool textOnly) = 0;
> -    virtual void zoomDefault() = 0;

please preserve these old methods but mark them deprecated with a
comment.  this way there is no need to juggle a two-sided commit.
the webkit portion can land well in advance of the chromium bits
without disruption.

getZoomLevel should probably just be zoomLevel to match webkit
attribute naming conventions.  although setZoomLevel is not a
normal attribute setter... hmm... see my naming suggestion below:


> +    // If |textOnly| is set, only the text will be zoomed; otherwise the entire
> +    // page will be zoomed. You can only have either text zoom or full page zoom
> +    // at one time.  Changing the mode while the page is zoomed will have odd
> +    // effects.
> +    virtual int setZoomLevel(bool textOnly, int zoomLevel) = 0;

please document what this function returns.

perhaps the API should be setTextZoomLevel and setPageZoomLevel with
associated textZoomLevel and pageZoomLevel accessors?  we can then
say that setting the text zoom while the page is zoomed, resets the
page zoom and vice versa.
Comment 4 Peter Kasting 2009-12-01 13:41:45 PST
(In reply to comment #3)
> perhaps the API should be setTextZoomLevel and setPageZoomLevel with
> associated textZoomLevel and pageZoomLevel accessors?  we can then
> say that setting the text zoom while the page is zoomed, resets the
> page zoom and vice versa.

This doesn't make sense, because the underlying code does not have a concept of two zoom levels (one for text, and one for everything).  Both accessors would always return the same value.

If I could simply get rid of the concept of text zoom from this API I would--Chrome never uses it--but it is needed by test_shell for some unittests, apparently.

I'd also consider splitting the setter into two pieces, a level and a type, but then all callers would have to call both for safety's sake.

So I think in the end it makes the most sense how it is now.
Comment 5 Peter Kasting 2009-12-01 13:44:00 PST
(In reply to comment #3)
> > +    virtual int setZoomLevel(bool textOnly, int zoomLevel) = 0;
> 
> please document what this function returns.

I did: "...returns the current zoom level after applying the change."
Comment 6 Peter Kasting 2009-12-01 13:51:01 PST
Created attachment 44098 [details]
patch v2

Renames getZoomLevel() -> zoomLevel() and restores old APIs, marking them as deprecated instead.
Comment 7 WebKit Review Bot 2009-12-01 13:55:05 PST
style-queue ran check-webkit-style on attachment 44098 [details] without any errors.
Comment 8 Darin Fisher (:fishd, Google) 2009-12-01 15:31:27 PST
Comment on attachment 44098 [details]
patch v2

> Index: WebKit/chromium/ChangeLog
...
> +        (WebKit::WebViewImpl::getZoomLevel):
> +        (WebKit::WebViewImpl::setZoomLevel):

nit: please update the ChangeLog ... zoomLevel


R=me otherwise
Comment 9 Peter Kasting 2009-12-01 15:35:12 PST
Comment on attachment 44098 [details]
patch v2

Landed in r51560, clearing flags.  Leaving this bug open to provide a home for the followup change to remove the deprecated APIs.
Comment 10 Peter Kasting 2009-12-01 18:23:16 PST
Created attachment 44120 [details]
Remove deprecated APIs

Followup to the previous patch: Remove the deprecated APIs.  Can land once http://codereview.chromium.org/437077 lands on the Chromium side.
Comment 11 WebKit Review Bot 2009-12-01 18:24:01 PST
style-queue ran check-webkit-style on attachment 44120 [details] without any errors.
Comment 12 Peter Kasting 2009-12-04 15:56:41 PST
Fixed in r51719.