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.
Created attachment 43878 [details] patch v1
style-queue ran check-webkit-style on attachment 43878 [details] without any errors.
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.
(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.
(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."
Created attachment 44098 [details] patch v2 Renames getZoomLevel() -> zoomLevel() and restores old APIs, marking them as deprecated instead.
style-queue ran check-webkit-style on attachment 44098 [details] without any errors.
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 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.
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.
style-queue ran check-webkit-style on attachment 44120 [details] without any errors.
Fixed in r51719.