Bug 31893 – Simplify Chromium embedding APIs for zoom

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Peter Kasting]

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.

[Peter Kasting]

Created attachment 43878 [details]
patch v1

[Adam Barth]

style-queue ran check-webkit-style on attachment 43878 [details] without any errors.

[Darin Fisher (:fishd, Google)]

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.

[Peter Kasting]

(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.

[Peter Kasting]

(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."

[Peter Kasting]

Created attachment 44098 [details]
patch v2

Renames getZoomLevel() -> zoomLevel() and restores old APIs, marking them as deprecated instead.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44098 [details] without any errors.

[Darin Fisher (:fishd, Google)]

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

[Peter Kasting]

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.

[Peter Kasting]

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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44120 [details] without any errors.

[Peter Kasting]

Fixed in r51719.