Bug 81940 – [chromium] Allow the viewport meta tag to be disabled for testing purposes

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Terry Anderson]

Add a new boolean value in WebView to force an early exit of the code in ChromeClientImpl::dispatchViewportPropertiesDidChange even if VIEWPORT is enabled by default.

[Terry Anderson]

Created attachment 133306 [details]
Patch

[Fady Samuel]

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

please initialize the variable int he constructor of WebViewImpl to false.

Please add a comment in WebView.h

[Terry Anderson]

Created attachment 133313 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 133313 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:263
> +    // Viewport -------------------------------------------------------------
> +
> +    // If the viewport is enabled, then the viewport meta tag will allow
> +    // a page to specify its own layout details
> +
> +    virtual bool isViewportEnabled() const = 0;
> +    virtual void enableViewport(bool enable) = 0;

Should this be part of WebPreferences?  That's where we end to put enable flags.

[Adam Barth]

Comment on attachment 133313 [details]
Patch

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

>> Source/WebKit/chromium/public/WebView.h:263
>> +    virtual void enableViewport(bool enable) = 0;
> 
> Should this be part of WebPreferences?  That's where we end to put enable flags.

Sorry, WebSettings: http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebSettings.h

[Adam Barth]

Comment on attachment 133313 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:255
>      virtual WebSize fixedLayoutSize() const = 0;
>      virtual void setFixedLayoutSize(const WebSize&) = 0;

Seems like this should also be a WebSetting...  Maybe there's some reason it's not?

[Terry Anderson]

Created attachment 133509 [details]
Patch for feedback on the placement of viewport functions

[Terry Anderson]

(In reply to comment #7)
> (From update of attachment 133313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133313&action=review
> 
> > Source/WebKit/chromium/public/WebView.h:255
> >      virtual WebSize fixedLayoutSize() const = 0;
> >      virtual void setFixedLayoutSize(const WebSize&) = 0;
> 
> Seems like this should also be a WebSetting...  Maybe there's some reason it's not?

If these get moved to WebSettings, should

    virtual bool isFixedLayoutModeEnabled() const = 0;
    virtual void enableFixedLayoutMode(bool enable) = 0;

move also or stay in WebView?

[Fady Samuel]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 133313 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133313&action=review
> > 
> > > Source/WebKit/chromium/public/WebView.h:255
> > >      virtual WebSize fixedLayoutSize() const = 0;
> > >      virtual void setFixedLayoutSize(const WebSize&) = 0;
> > 
> > Seems like this should also be a WebSetting...  Maybe there's some reason it's not?
> 
> If these get moved to WebSettings, should
> 
>     virtual bool isFixedLayoutModeEnabled() const = 0;
>     virtual void enableFixedLayoutMode(bool enable) = 0;
> 
> move also or stay in WebView?

Please leave fixed layout stuff in WebView for now because it touches Page. 

We can clean it up in another patch. Making it setting will require some changes in WebCore.

enableViewport is independent of WebCore.

[Adam Barth]

Comment on attachment 133509 [details]
Patch for feedback on the placement of viewport functions

We had a pow-wow with fishd, and it sounds like this is the proper place for this setting (since it's scope is a WebView).

[WebKit Review Bot]

Comment on attachment 133509 [details]
Patch for feedback on the placement of viewport functions

Clearing flags on attachment: 133509

Committed r112088: <http://trac.webkit.org/changeset/112088>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.