[webkit-reviews] review denied: [Bug 31893] Simplify Chromium embedding APIs for zoom : [Attachment 43878] patch v1

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Peter Kasting
<pkasting at google.com>'s request for review:
Bug 31893: Simplify Chromium embedding APIs for zoom
https://bugs.webkit.org/show_bug.cgi?id=31893

Attachment 43878: patch v1
https://bugs.webkit.org/attachment.cgi?id=43878&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.