Bug 73495 – [Chromium] Enable viewport metatag

• Component: New Bugs
• Status: RESOLVED FIXED

[Fady Samuel]

[Chromium] Enable viewport metatag

[Fady Samuel]

Created attachment 117289 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Fady Samuel]

Once this patch lands, it should be possible to begin upstreaming layout tests for viewport that are independent of chromium. As long as fixed layout mode is enabled, viewport is respected.

[Darin Fisher (:fishd, Google)]

Comment on attachment 117289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117289&action=review

I have to admit that I find it hard to understand what you are trying to accomplish,
which makes it hard to provide a good review.

You are doing a lot of work in ChromeClientImpl::dispatchViewportPropertiesDidChange,
which smells fishy.  Normally, the WebKit layer is just about providing API glue
around WebCore.  We try not to do a lot of substantive work there.  Are you sure
this couldn't / shouldn't live in WebCore?

> Source/WebKit/chromium/public/WebViewClient.h:115
> +    virtual void didReceiveViewport(float initialScale, bool initialScaleExplicitlySet, float minimumScale, float maximumScale, float devicePixelRatio) { }

hmm, have you considered creating a WebViewport structure for these values?

> Source/WebKit/chromium/public/WebViewClient.h:120
> +    virtual int getDefaultFixedLayoutWidth() const { return 980; }

Why isn't this value pushed down to WebKit?  Why does it need to be a callback?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1053
> +            m_page->chrome()->client()->dispatchViewportPropertiesDidChange(viewport);

It is kind of strange to be calling the ChromeClient from WebViewImpl.  ChromeClient is
meant to be called by WebCore.  Could it be that some more of this fixed layout processing
code should actually move down into WebCore?  Why isn't the FrameView handling this (i.e.,
why not do this work from FrameView::resize)?

[Fady Samuel]

Comment on attachment 117289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117289&action=review

>> Source/WebKit/chromium/public/WebViewClient.h:115
>> +    virtual void didReceiveViewport(float initialScale, bool initialScaleExplicitlySet, float minimumScale, float maximumScale, float devicePixelRatio) { }
> 
> hmm, have you considered creating a WebViewport structure for these values?

It was initially when we had a larger set of parameters, I'm actually not convinced we need to pass all these parameters now (because WebViewImpl keeps track of them for us now), thanks in part to aelias@) :)

>> Source/WebKit/chromium/public/WebViewClient.h:120
>> +    virtual int getDefaultFixedLayoutWidth() const { return 980; }
> 
> Why isn't this value pushed down to WebKit?  Why does it need to be a callback?

The value is very platform and device dependent. This doesn't belong in WebKit in my humble opinion. Some devices have 800px default layout width (if there is no viewport tag), some have 980. This is highly device dependent.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1053
>> +            m_page->chrome()->client()->dispatchViewportPropertiesDidChange(viewport);
> 
> It is kind of strange to be calling the ChromeClient from WebViewImpl.  ChromeClient is
> meant to be called by WebCore.  Could it be that some more of this fixed layout processing
> code should actually move down into WebCore?  Why isn't the FrameView handling this (i.e.,
> why not do this work from FrameView::resize)?

I guess we can call dispatchViewportPropertiesDidChange from FrameView::resize? The actual viewport computations are highly platform and device dependent as I mentioned. Not all the details have been standardized for how viewport should work.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> (From update of attachment 117289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117289&action=review
> 
> >> Source/WebKit/chromium/public/WebViewClient.h:115
> >> +    virtual void didReceiveViewport(float initialScale, bool initialScaleExplicitlySet, float minimumScale, float maximumScale, float devicePixelRatio) { }
> > 
> > hmm, have you considered creating a WebViewport structure for these values?
> 
> It was initially when we had a larger set of parameters, I'm actually not convinced we need to pass all these parameters now (because WebViewImpl keeps track of them for us now), thanks in part to aelias@) :)
> 
> >> Source/WebKit/chromium/public/WebViewClient.h:120
> >> +    virtual int getDefaultFixedLayoutWidth() const { return 980; }
> > 
> > Why isn't this value pushed down to WebKit?  Why does it need to be a callback?
> 
> The value is very platform and device dependent. This doesn't belong in WebKit in my humble opinion. Some devices have 800px default layout width (if there is no viewport tag), some have 980. This is highly device dependent.

It sounds like you are describing something that is a setting.  See WebSettings.  We also convey some other settings via static setters on WebView.  I don't see why this is different.  You only need a callback for things that are very dynamic, or which may change at subtle times.  This doesn't seem to fit that bill.


> >> Source/WebKit/chromium/src/WebViewImpl.cpp:1053
> >> +            m_page->chrome()->client()->dispatchViewportPropertiesDidChange(viewport);
> > 
> > It is kind of strange to be calling the ChromeClient from WebViewImpl.  ChromeClient is
> > meant to be called by WebCore.  Could it be that some more of this fixed layout processing
> > code should actually move down into WebCore?  Why isn't the FrameView handling this (i.e.,
> > why not do this work from FrameView::resize)?
> 
> I guess we can call dispatchViewportPropertiesDidChange from FrameView::resize? The actual viewport computations are highly platform and device dependent as I mentioned. Not all the details have been standardized for how viewport should work.

Would it make sense for FrameView to have an #if ENABLE(VIEWPORT) code branch?  How can I learn more about viewport?

[Fady Samuel]

Comment on attachment 117289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117289&action=review

>>>> Source/WebKit/chromium/public/WebViewClient.h:115
>>>> +    virtual void didReceiveViewport(float initialScale, bool initialScaleExplicitlySet, float minimumScale, float maximumScale, float devicePixelRatio) { }
>>> 
>>> hmm, have you considered creating a WebViewport structure for these values?
>> 
>> It was initially when we had a larger set of parameters, I'm actually not convinced we need to pass all these parameters now (because WebViewImpl keeps track of them for us now), thanks in part to aelias@) :)
> 
> It sounds like you are describing something that is a setting.  See WebSettings.  We also convey some other settings via static setters on WebView.  I don't see why this is different.  You only need a callback for things that are very dynamic, or which may change at subtle times.  This doesn't seem to fit that bill.

You're right. I will look into WebSettings.

>>>> Source/WebKit/chromium/src/WebViewImpl.cpp:1053
>>>> +            m_page->chrome()->client()->dispatchViewportPropertiesDidChange(viewport);
>>> 
>>> It is kind of strange to be calling the ChromeClient from WebViewImpl.  ChromeClient is
>>> meant to be called by WebCore.  Could it be that some more of this fixed layout processing
>>> code should actually move down into WebCore?  Why isn't the FrameView handling this (i.e.,
>>> why not do this work from FrameView::resize)?
>> 
>> I guess we can call dispatchViewportPropertiesDidChange from FrameView::resize? The actual viewport computations are highly platform and device dependent as I mentioned. Not all the details have been standardized for how viewport should work.
> 
> Would it make sense for FrameView to have an #if ENABLE(VIEWPORT) code branch?  How can I learn more about viewport?

http://developer.android.com/guide/webapps/targeting.html contains most of the details.

I think it makes sense to put it in an ENABLE(VIEWPORT) branch at this point in time.

[Alexandre Elias]

This change looks good to me.  I agree with Darin's suggestions.

As for didReceiveViewport, it looks like at this point, we only need one parameter, initialScale.  Given the other logic that has been pushed down to WebViewImpl, the Chrome-side code only needs to know that the scale has been set at all.  So we can switch to:

virtual void didReceiveViewport(float initialScale);

[Alexandre Elias]

Correction, the boolean is what we need (as the computed value doesn't specify whether it was explicitly set or not), so we should have:

virtual void didReceiveViewport(bool initialScaleExplicitlySet);

[Fady Samuel]

(In reply to comment #8)
> This change looks good to me.  I agree with Darin's suggestions.
> 
> As for didReceiveViewport, it looks like at this point, we only need one parameter, initialScale.  Given the other logic that has been pushed down to WebViewImpl, the Chrome-side code only needs to know that the scale has been set at all.  So we can switch to:
> 
> virtual void didReceiveViewport(float initialScale);

I will make the following changes:

1. default fixed layout width becomes a WebSetting.
2. didReceiveViewport loses a few (unused) parameters.
3. Moving a call to dispatchViewportPropertiesDidChange from WebViewImpl::resize to FrameView::resize behind an ENABLE(VIEWPORT) flag.

[Fady Samuel]

Created attachment 117585 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117585 [details]
Patch

Attachment 117585 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10693969

[Fady Samuel]

Created attachment 117630 [details]
Made it compile with Chromium-side changes

[Fady Samuel]

That last patch should say without rather than with Chromium-side changes.

[Fady Samuel]

*** Bug 70558 has been marked as a duplicate of this bug. ***

[Darin Fisher (:fishd, Google)]

Comment on attachment 117630 [details]
Made it compile with Chromium-side changes

View in context: https://bugs.webkit.org/attachment.cgi?id=117630&action=review

> Source/WebKit/chromium/public/WebViewClient.h:116
> +    virtual void didReceiveViewport(bool initialScaleExplicitlySet) { }

Can this be called more than once?  Or, does this just happen during page load?
Is the <meta viewport> only honored for the top-most frame?

If the viewport parameters can change after the page is loaded (because the tag
could be re-written), then we should probably name this function didUpdateViewport
or didChangeViewport instead.

I'm a little confused about the division of labor between WebKit and the embedder
w.r.t. scaling.  It seems like we should either handle everything inside WebKit
or not.  Are there reasons why the embedder has to know anything about viewport
scaling factors?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:622
> +    // If the window size has not been set yet don't attempt to set the viewport

do you have to worry about setting it later when the deviceRect information is available?
it seems like stopping here could result in buggy behavior.  am i missing something?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:639
> +    m_webView->setPageScaleFactorLimits(computed.minimumScale, computed.maximumScale);

if we are calling these methods here, within WebKit, do we still need these
functions on the public WebView interface?

[Fady Samuel]

Created attachment 117769 [details]
Patch

[Fady Samuel]

(In reply to comment #17)
> Created an attachment (id=117769) [details]
> Patch

This needs more work. Uploading for personal reference.

[Fady Samuel]

Comment on attachment 117630 [details]
Made it compile with Chromium-side changes

View in context: https://bugs.webkit.org/attachment.cgi?id=117630&action=review

>> Source/WebKit/chromium/public/WebViewClient.h:116
>> +    virtual void didReceiveViewport(bool initialScaleExplicitlySet) { }
> 
> Can this be called more than once?  Or, does this just happen during page load?
> Is the <meta viewport> only honored for the top-most frame?
> 
> If the viewport parameters can change after the page is loaded (because the tag
> could be re-written), then we should probably name this function didUpdateViewport
> or didChangeViewport instead.
> 
> I'm a little confused about the division of labor between WebKit and the embedder
> w.r.t. scaling.  It seems like we should either handle everything inside WebKit
> or not.  Are there reasons why the embedder has to know anything about viewport
> scaling factors?

1. I've removed this, so this addresses your point. Things should happen from ChromeClientImpl::layoutUpdated now.
2. I absolutely agree, I would love to move as much as possible into WebKit. Basically, all the embedder should be doing is deciding whether to enable fixed layout or not...

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:622
>> +    // If the window size has not been set yet don't attempt to set the viewport
> 
> do you have to worry about setting it later when the deviceRect information is available?
> it seems like stopping here could result in buggy behavior.  am i missing something?

It seems like there are cases where the size of the WebView is 0 initially, and then it sizes itself appropriately, but we deal with this in FrameView::resize now.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:639
>> +    m_webView->setPageScaleFactorLimits(computed.minimumScale, computed.maximumScale);
> 
> if we are calling these methods here, within WebKit, do we still need these
> functions on the public WebView interface?

Where else would they go? We might want to expose these to our port of DRT for testing, no?

[Fady Samuel]

(In reply to comment #19)
> (From update of attachment 117630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117630&action=review
> 
> >> Source/WebKit/chromium/public/WebViewClient.h:116
> >> +    virtual void didReceiveViewport(bool initialScaleExplicitlySet) { }
> > 
> > Can this be called more than once?  Or, does this just happen during page load?
> > Is the <meta viewport> only honored for the top-most frame?
> > 
> > If the viewport parameters can change after the page is loaded (because the tag
> > could be re-written), then we should probably name this function didUpdateViewport
> > or didChangeViewport instead.
> > 
> > I'm a little confused about the division of labor between WebKit and the embedder
> > w.r.t. scaling.  It seems like we should either handle everything inside WebKit
> > or not.  Are there reasons why the embedder has to know anything about viewport
> > scaling factors?
> 
> 1. I've removed this, so this addresses your point. Things should happen from ChromeClientImpl::layoutUpdated now.
> 2. I absolutely agree, I would love to move as much as possible into WebKit. Basically, all the embedder should be doing is deciding whether to enable fixed layout or not...
> 
> >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:622
> >> +    // If the window size has not been set yet don't attempt to set the viewport
> > 
> > do you have to worry about setting it later when the deviceRect information is available?
> > it seems like stopping here could result in buggy behavior.  am i missing something?
> 
> It seems like there are cases where the size of the WebView is 0 initially, and then it sizes itself appropriately, but we deal with this in FrameView::resize now.
> 
> >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:639
> >> +    m_webView->setPageScaleFactorLimits(computed.minimumScale, computed.maximumScale);
> > 
> > if we are calling these methods here, within WebKit, do we still need these
> > functions on the public WebView interface?
> 
> Where else would they go? We might want to expose these to our port of DRT for testing, no?

Darin, I'd like to add that an embedder may choose to not use fixed layout or use the viewport tag and instead control scaling outside of WebKit. I don't see why we can't expose this to the public API?

[Fady Samuel]

Comment on attachment 117630 [details]
Made it compile with Chromium-side changes

View in context: https://bugs.webkit.org/attachment.cgi?id=117630&action=review

>>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:639
>>> +    m_webView->setPageScaleFactorLimits(computed.minimumScale, computed.maximumScale);
>> 
>> if we are calling these methods here, within WebKit, do we still need these
>> functions on the public WebView interface?
> 
> Where else would they go? We might want to expose these to our port of DRT for testing, no?

Let me repeat inline: An embedder may choose to control scaling outside of WebKit, and disable fixed layout, for example. Not all content will take this code path (if fixed layout is disabled).

[Fady Samuel]

I'd like to use computePageScaleFactorLimits from ChromeClientImpl::layoutUpdated:

https://bugs.webkit.org/show_bug.cgi?id=72983

I'm going to set this bug  as depending on 72983.

[Fady Samuel]

(In reply to comment #22)
> I'd like to use computePageScaleFactorLimits from ChromeClientImpl::layoutUpdated:
> 
> https://bugs.webkit.org/show_bug.cgi?id=72983
> 
> I'm going to set this bug  as depending on 72983.

Hmm, I just noticed computePageScaleFactorLimits is private. It's called anyway if we compute a default viewport on first layout, so I'll leave that part of the code as it is. I'm about to upload a change that makes use of the screen DPI.

[Fady Samuel]

Created attachment 118339 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 118339 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118339&action=review

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:628
> +

nit: extra new line

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:655
> +

nit: extra new line

[Fady Samuel]

Created attachment 118513 [details]
Patch for landing

[Fady Samuel]

Comment on attachment 118339 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118339&action=review

There's a minor issue here where if the user does a same page navigation, the page scale factor will get reset, as pointed out by aelias@. The change is non-trivial, so I will fix it in a separate bug.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:628
>> +
> 
> nit: extra new line

Done.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:655
>> +
> 
> nit: extra new line

Done.

[WebKit Review Bot]

Comment on attachment 118513 [details]
Patch for landing

Clearing flags on attachment: 118513

Committed r102429: <http://trac.webkit.org/changeset/102429>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.