Bug 81940 - [chromium] Allow the viewport meta tag to be disabled for testing purposes
: [chromium] Allow the viewport meta tag to be disabled for testing purposes
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Terry Anderson
:
Depends on:
Blocks: 70559
  Show dependency treegraph
 
Reported: 2012-03-22 11:38 PDT by Terry Anderson
Modified: 2012-03-26 07:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.00 KB, patch)
2012-03-22 11:44 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (4.46 KB, patch)
2012-03-22 12:08 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch for feedback on the placement of viewport functions (4.05 KB, patch)
2012-03-23 10:44 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Terry Anderson 2012-03-22 11:38:00 PDT
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.
Comment 1 Terry Anderson 2012-03-22 11:44:38 PDT
Created attachment 133306 [details]
Patch
Comment 2 Fady Samuel 2012-03-22 11:53:08 PDT
(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
Comment 3 Terry Anderson 2012-03-22 12:08:28 PDT
Created attachment 133313 [details]
Patch
Comment 4 WebKit Review Bot 2012-03-22 12:09:58 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 Adam Barth 2012-03-22 15:27:00 PDT
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.
Comment 6 Adam Barth 2012-03-22 15:27:47 PDT
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
Comment 7 Adam Barth 2012-03-22 15:28:40 PDT
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?
Comment 8 Terry Anderson 2012-03-23 10:44:57 PDT
Created attachment 133509 [details]
Patch for feedback on the placement of viewport functions
Comment 9 Terry Anderson 2012-03-23 11:58:43 PDT
(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?
Comment 10 Fady Samuel 2012-03-23 14:30:41 PDT
(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.
Comment 11 Adam Barth 2012-03-23 16:26:11 PDT
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).
Comment 12 WebKit Review Bot 2012-03-26 07:16:26 PDT
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>
Comment 13 WebKit Review Bot 2012-03-26 07:16:32 PDT
All reviewed patches have been landed.  Closing bug.