Bug 73495 - [Chromium] Enable viewport metatag
Summary: [Chromium] Enable viewport metatag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
: 70558 (view as bug list)
Depends on: 70556 72983
Blocks: 70559
  Show dependency treegraph
 
Reported: 2011-11-30 16:02 PST by Fady Samuel
Modified: 2011-12-08 21:15 PST (History)
8 users (show)

See Also:


Attachments
Patch (6.68 KB, patch)
2011-11-30 16:09 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.20 KB, patch)
2011-12-02 00:51 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Made it compile with Chromium-side changes (8.44 KB, patch)
2011-12-02 08:29 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (13.14 KB, patch)
2011-12-03 11:06 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (12.98 KB, patch)
2011-12-07 23:45 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch for landing (13.11 KB, patch)
2011-12-08 19:02 PST, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2011-11-30 16:02:59 PST
[Chromium] Enable viewport metatag
Comment 1 Fady Samuel 2011-11-30 16:09:07 PST
Created attachment 117289 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-30 16:10:28 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Fady Samuel 2011-11-30 16:11:11 PST
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.
Comment 4 Darin Fisher (:fishd, Google) 2011-12-01 09:34:46 PST
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)?
Comment 5 Fady Samuel 2011-12-01 09:44:31 PST
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.
Comment 6 Darin Fisher (:fishd, Google) 2011-12-01 10:51:28 PST
(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?
Comment 7 Fady Samuel 2011-12-01 10:59:36 PST
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.
Comment 8 Alexandre Elias 2011-12-01 13:15:52 PST
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);
Comment 9 Alexandre Elias 2011-12-01 13:19:36 PST
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);
Comment 10 Fady Samuel 2011-12-01 13:20:35 PST
(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.
Comment 11 Fady Samuel 2011-12-02 00:51:29 PST
Created attachment 117585 [details]
Patch
Comment 12 WebKit Review Bot 2011-12-02 01:14:18 PST
Comment on attachment 117585 [details]
Patch

Attachment 117585 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10693969
Comment 13 Fady Samuel 2011-12-02 08:29:05 PST
Created attachment 117630 [details]
Made it compile with Chromium-side changes
Comment 14 Fady Samuel 2011-12-02 08:29:45 PST
That last patch should say without rather than with Chromium-side changes.
Comment 15 Fady Samuel 2011-12-02 08:35:50 PST
*** Bug 70558 has been marked as a duplicate of this bug. ***
Comment 16 Darin Fisher (:fishd, Google) 2011-12-02 15:06:58 PST
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?
Comment 17 Fady Samuel 2011-12-03 11:06:01 PST
Created attachment 117769 [details]
Patch
Comment 18 Fady Samuel 2011-12-03 11:10:09 PST
(In reply to comment #17)
> Created an attachment (id=117769) [details]
> Patch

This needs more work. Uploading for personal reference.
Comment 19 Fady Samuel 2011-12-03 11:17:19 PST
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?
Comment 20 Fady Samuel 2011-12-03 11:20:30 PST
(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?
Comment 21 Fady Samuel 2011-12-03 11:23:03 PST
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).
Comment 22 Fady Samuel 2011-12-03 11:36:06 PST
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.
Comment 23 Fady Samuel 2011-12-07 23:36:13 PST
(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.
Comment 24 Fady Samuel 2011-12-07 23:45:56 PST
Created attachment 118339 [details]
Patch
Comment 25 Darin Fisher (:fishd, Google) 2011-12-08 12:12:00 PST
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
Comment 26 Fady Samuel 2011-12-08 19:02:02 PST
Created attachment 118513 [details]
Patch for landing
Comment 27 Fady Samuel 2011-12-08 19:08:08 PST
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.
Comment 28 WebKit Review Bot 2011-12-08 21:15:36 PST
Comment on attachment 118513 [details]
Patch for landing

Clearing flags on attachment: 118513

Committed r102429: <http://trac.webkit.org/changeset/102429>
Comment 29 WebKit Review Bot 2011-12-08 21:15:43 PST
All reviewed patches have been landed.  Closing bug.