Created attachment 106354 [details] Patch

Comment on attachment 106354 [details] Patch Can you say more about how this will be used? It is really hard to review this without having the bigger picture in mind.

Sure. Here are some snippets from the Android to-be-upstreamed patch. It looks like plumbing for the viewport meta tag. --- a/Source/WebKit/chromium/public/WebViewClient.h +++ b/Source/WebKit/chromium/public/WebViewClient.h + // Did receive the viewport meta tag + virtual void didReceiveViewport(const WebViewport&) { } --- a/Source/WebKit/chromium/src/ChromeClientImpl.cpp +++ b/Source/WebKit/chromium/src/ChromeClientImpl.cpp +void ChromeClientImpl::dispatchViewportDataDidChange(const ViewportArguments& arguments) const +{ +#if OS(ANDROID) + if (m_webView->client()) { + ViewportArguments args; + if (arguments == args) { + // Default viewport arguments passed in. This is a signal to reset the viewport. + args.width = ViewportArguments::ValueDesktopWidth; + } else { + args = arguments; + if (arguments.minimumScale == 1.0f && + arguments.initialScale == ViewportArguments::ValueAuto && + arguments.width == ViewportArguments::ValueAuto && + arguments.height == ViewportArguments::ValueAuto) { + // Fix for mobile google.com, since right now only minimum-scale + // 1.0 is passed in the viewport argument. + args.width = ViewportArguments::ValueDeviceWidth; + } + } + WebViewClient* client = m_webView->client(); + WebRect deviceRect = client->getDeviceRect(); + // Call the common viewport computing logic in ViewportArguments.cpp. + ViewportAttributes computed = computeViewportAttributes( + args, client->getFixedLayoutWidth(), deviceRect.width, deviceRect.height, + client->getDeviceDpi(), IntSize(deviceRect.width, deviceRect.height)); + int computedWidth = computed.layoutSize.width(); + int computedHeight = computed.layoutSize.height(); + + WebViewport viewport; + viewport.initialScale = computed.initialScale; + viewport.initialScaleExplicitlySet = + (arguments.initialScale != ViewportArguments::ValueAuto); + viewport.minimumScale = computed.minimumScale; + viewport.maximumScale = computed.maximumScale; + viewport.width = computedWidth; + viewport.height = computedHeight; + viewport.devicePixelRatio = computed.devicePixelRatio; + viewport.userScalable = (computed.userScalable == 1 || + computed.userScalable == ViewportArguments::ValueAuto); + m_webView->client()->didReceiveViewport(viewport); + + // FIXME: + // "width=320" is a magic number as it was used in the early days to + // indicate the site wants the device width; while it's not handled in + // computeViewportAttributes. May want to upstream later. + IntSize size(computedWidth, computedHeight); + if (computedWidth == 320 || !viewport.userScalable) { + FloatRect rect = const_cast<ChromeClientImpl*>(this)->windowRect(); + size.setWidth(rect.width()); + size.setHeight(rect.height()); + } + m_webView->page()->mainFrame()->view()->setFixedLayoutSize(size); + } +#endif +} Here's some documentation about the viewport meta tag: https://developer.mozilla.org/en/mobile/viewport_meta_tag http://dev.opera.com/articles/view/an-introduction-to-meta-viewport-and-viewport/ It is a widely used feature on the mobile web.

Comment on attachment 106354 [details] Patch Why a struct? That's very much not webkit style. :(

grep tells me there are 49 struct declarations in the Chromium WebKit API. It's an API pattern we use when exposing pure data either to or from the embedder. For example, here is a declaration for WebRect: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebRect.h

Comment on attachment 106354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106354&action=review > Source/WebKit/chromium/public/WebViewport.h:39 > + DeviceValue = 0, nit: there's a convention in webkit API to name enum values like so: enum Foo { FooBar, FooBaz }; Obviously, it is OK to drop the enum name since you don't need it, but perhaps it would be good to change DeviceValue to ValueSomething. That said, it looks like DeviceValue is unused. If a different patch will use it, then maybe it could be called ValueDefault? > Source/WebKit/chromium/public/WebViewport.h:66 > +} nit: } // namespace WebKit

Comment on attachment 106354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106354&action=review >> Source/WebKit/chromium/public/WebViewport.h:39 >> + DeviceValue = 0, > > nit: there's a convention in webkit API to name enum values like so: > > enum Foo { > FooBar, > FooBaz > }; > > Obviously, it is OK to drop the enum name since you don't need it, but perhaps > it would be good to change DeviceValue to ValueSomething. That said, it looks > like DeviceValue is unused. If a different patch will use it, then maybe it > could be called ValueDefault? ValueDevice doesn't read that well... Maybe: enum SpecialValue { SpecialValueUndefined, SpecialValueDevice, }; Although, perhaps a better solution is to avoid in-band signaling altogether using a struct for each of these values: struct Value { bool isDefined; bool isDevice; float value; }; Although then you have the problem of what the semantics are if both isDefined and isDevice are true...

Created attachment 106710 [details] Patch

Comment on attachment 106710 [details] Patch I talked this over with fishd. It's hard to understand whether this data structure is correct in isolation. I'm going to look at the bigger picture and revise.

I took a look at the renderer side of things to see where its used. This seems to be where (content/renderer/render_view.cc - now render_view_impl.cc): #if defined(OS_ANDROID) void RenderView::didReceiveViewport(const WebKit::WebViewport& viewport) { dpi_scale_ = viewport.devicePixelRatio; user_scalable_ = viewport.userScalable; minimum_scale_ = std::max(viewport.minimumScale, content::kMinMagnifyZoom); maximum_scale_ = std::min(viewport.maximumScale, content::kMaxMagnifyZoom); UpdateHostViewportScale(); SetMagnifyZoomAndScroll(viewport.initialScale * dpi_scale_, gfx::Point(0, 0)); magnify_scale_set_ = viewport.initialScaleExplicitlySet; } This suggests to me width, height and targetDensityDpi are unnecessary. I'm not certain if we absolutely need two variables, one for userScalable, and initialScaleExplicitlySet. I'll see if we can avoid that. I suspect devicePixelRatio might be unnecessary as well if we use and expose deviceScaleFactor that was recently upstreamed.

(In reply to comment #10) > I took a look at the renderer side of things to see where its used. This seems to be where (content/renderer/render_view.cc - now render_view_impl.cc): > > #if defined(OS_ANDROID) > void RenderView::didReceiveViewport(const WebKit::WebViewport& viewport) { > dpi_scale_ = viewport.devicePixelRatio; > user_scalable_ = viewport.userScalable; > minimum_scale_ = std::max(viewport.minimumScale, content::kMinMagnifyZoom); > maximum_scale_ = std::min(viewport.maximumScale, content::kMaxMagnifyZoom); > UpdateHostViewportScale(); > > SetMagnifyZoomAndScroll(viewport.initialScale * dpi_scale_, gfx::Point(0, 0)); > magnify_scale_set_ = viewport.initialScaleExplicitlySet; > } > > This suggests to me width, height and targetDensityDpi are unnecessary. > > I'm not certain if we absolutely need two variables, one for userScalable, and initialScaleExplicitlySet. I'll see if we can avoid that. I suspect devicePixelRatio might be unnecessary as well if we use and expose deviceScaleFactor that was recently upstreamed. Given that we can probably get away with just 4 parameters, I suggest avoiding this struct all together, and instead passing them all explicitly to didReceiveViewport.

Adam, do you mind if I close this bug? I don't think I'll be using WebViewport when I upstream my changes. Thanks.

You should feel free to close any of my bugs that you think should be closed. I'll re-open them if I disagree. :)