Bug 47202 – Adding Qt WebKit2 API for dealing with viewport meta info

• Component: WebKit2
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

I'm looking into exposing the viewport meta tag info to WebKit2 and we need to discuss now such an API should look like

How does it work:

1. The user should be notified when the current viewport data gets invalidated so that it can be recomputed.

Currently we have a ChromeClient::dispatchDidChangeViewportData which can be used for that, which is called when the meta info changes (if check for change when new meta data is received and when the body is inserted into the document in case no viewport meta tag was encountered).

2. page->viewportArguments() can be used to get the current web author provided arguments.

The things is that the raw data provided by the web author needs to be treated to be useful for a browser. There is now a method in webcore for that which needs data such as the actual visible view (think the iphone minus ui components), plus the global device specifics such as device height.


An idea for a C API:

1. add a callback to the UI client, much like ViewportAttributesInvalidated or so.
2. add something like the below for calculating the viewport (maximum scale, minimum-scale, viewport layout width, etc)

WK_EXPORT ... WKPageComputeViewportAttributes(WKPageRef page, int visibleWidth, int visibleHeight, int desktopWidth, int deviceWidth, int deviceHeight, int deviceDPI);

I wonder if it makes sense returning just a struct or make a new C type, WKViewportAttributesRef with methods such as WKViewportAttributesGetMaximumScale() etc.

[Kenneth Rohde Christiansen]

Example API:

// Call to set the initial viewport (before first load) and every time the visible area changes in your app - ie. when you rotate your phone.

WK_EXPORT ViewportAttributesRef WKPageComputeViewportAttributes(WKPageRef page, int visibleWidth, int visibleHeight, int desktopWidth, int deviceWidth, int deviceHeight, int deviceDPI);

// Callback called every time the viewport attributes much be updated.

typedef void (*WKPageViewportAttributesInvalidatedCallback)(WKPageRef page, const void *clientInfo);

// Getter methods

WK_EXPORT int WKViewportArgumentsGetLayoutWidth(WKViewportAttributesRef viewportAttr);
WK_EXPORT int WKViewportArgumentsGetLayoutHeight(WKViewportAttributesRef viewportAttr);
WK_EXPORT float WKViewportArgumentsGetInitialScaleFactor(WKViewportAttributesRef viewportAttr);
WK_EXPORT float WKViewportArgumentsGetMinimumScaleFactor(WKViewportAttributesRef viewportAttr);
WK_EXPORT float WKViewportArgumentsGetMaximumScaleFactor(WKViewportAttributesRef viewportAttr);
WK_EXPORT float WKViewportArgumentsGetPixelRatio(WKViewportAttributesRef viewportAttr);
WK_EXPORT bool WKViewportArgumentsGetIsUserScalable(WKViewportAttributesRef viewportAttr);


I guess WKViewportAttributes should map to a new WebKit class or to WebCore::ViewportConfiguration which currently exists, and it being returned by WebCore::findConfigurationForViewportData(ViewportArguments...).

Comments welcome.

[Andreas Kling]

This API looks generally good to me, assuming that we need to leave the door open for future attribute additions.

[Kenneth Rohde Christiansen]

(In reply to comment #2)
> This API looks generally good to me, assuming that we need to leave the door open for future attribute additions.

I'm considering renaming WebCore::ViewportConfiguration to WebCore::ViewportAttributes and WebCore::findConfigurationForViewportData(...) to WebCore::computeViewportAttributes(...)

[Adam Roben (:aroben)]

(In reply to comment #1)
> Example API:
> 
> // Call to set the initial viewport (before first load) and every time the visible area changes in your app - ie. when you rotate your phone.
> 
> WK_EXPORT ViewportAttributesRef WKPageComputeViewportAttributes(WKPageRef page, int visibleWidth, int visibleHeight, int desktopWidth, int deviceWidth, int deviceHeight, int deviceDPI);

ViewportAttributesRef should be prefixed with "WK": WKViewportAttributesRef.

The WebKit2 C API currently follows CoreFoundation's conventions regarding ref-counted types:
1) Types ending in "Ref" are assumed to be ref-counted using WKRetain/WKRelease
2) Functions that return ref-counted types should either have "Get", "Create", or "Copy" in their name

(2) is important so that the caller knows whether they need to call WKRelease on the returned pointer. You have to call WKRelease for Create- or Copy-style functions, but not for Get-style functions.

I'll leave the other issues to Sam and Anders.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (In reply to comment #1)
> > Example API:
> > 
> > // Call to set the initial viewport (before first load) and every time the visible area changes in your app - ie. when you rotate your phone.
> > 
> > WK_EXPORT ViewportAttributesRef WKPageComputeViewportAttributes(WKPageRef page, int visibleWidth, int visibleHeight, int desktopWidth, int deviceWidth, int deviceHeight, int deviceDPI);
> 
> ViewportAttributesRef should be prefixed with "WK": WKViewportAttributesRef.

Oh, that was a typo from my side.

> The WebKit2 C API currently follows CoreFoundation's conventions regarding ref-counted types:
> 1) Types ending in "Ref" are assumed to be ref-counted using WKRetain/WKRelease
> 2) Functions that return ref-counted types should either have "Get", "Create", or "Copy" in their name
> 
> (2) is important so that the caller knows whether they need to call WKRelease on the returned pointer. You have to call WKRelease for Create- or Copy-style functions, but not for Get-style functions.
> 
> I'll leave the other issues to Sam and Anders.

Got it! Thanks for the explanation.

What about?

WK_EXPORT WKViewportAttributesRef WKPageCreateComputedViewportAttributes(...) 

or do you have any better idea?

[Kenneth Rohde Christiansen]

Someone suggested calling this WKViewportFeaturesRef instead of WKViewportAttributesRef. 

I would like to know what name you think best describes the structure.

[Adam Roben (:aroben)]

(In reply to comment #5)
> What about?
> 
> WK_EXPORT WKViewportAttributesRef WKPageCreateComputedViewportAttributes(...) 
> 
> or do you have any better idea?

I'm not sure "Computed" adds anything here. And "Copy" makes a little more sense to me, since these seem to me to be a property of the page, rather than an entirely separate object that the page is just a factory for. So WKPageCopyViewportAttributes seems like a good name to me.

As I said in bug 47256, I think it would be useful to discuss use-cases for the API as well. That way we'll know if the API we're designing will actually work for anyone!

(In reply to comment #6)
> Someone suggested calling this WKViewportFeaturesRef instead of WKViewportAttributesRef. 
> 
> I would like to know what name you think best describes the structure.

"Attributes" seems better than "Features". It's strange to think about something like "width" being a "feature".

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (In reply to comment #5)
> > What about?
> > 
> > WK_EXPORT WKViewportAttributesRef WKPageCreateComputedViewportAttributes(...) 
> > 
> > or do you have any better idea?
> 
> I'm not sure "Computed" adds anything here. And "Copy" makes a little more sense to me, since these seem to me to be a property of the page, rather than an entirely separate object that the page is just a factory for. So WKPageCopyViewportAttributes seems like a good name to me.

Basically Apple added a viewport meta tag to the iphone, which later got augmented with one additional feature by Android. Now everyone seems to be implementing this, including Opera and Firefox - it has thus become a defacto standard.

The web author can provide some hints to how he wants the page layouted which is very useful for making webpages act as real applications, for instance by setting width = device-width.

These input needs to be treated in a specific way to compute the actual decided layout viewport (plus min, max scale etc) given the device sizes, minimum actual visible viewport.

Info: http://people.opera.com/rune/TR/css-viewport/

The app who wants to support this (mostly browser with mobile user agent receives the viewport meta  tags), will need to compute the viewport attributes before first load, and then again everytime it get's invalidates (loading a new page, navigating page cache) or every time on of the browser attributes change, such as when the device is rotates (which changes the actual visible viewport).

[Gustavo Noronha (kov)]

I like attributes better than features, too, actually! Features came to my mind because of WindowFeatures.

[Kenneth Rohde Christiansen]

Created attachment 70113 [details]
WIP Patch

Work in progress patch. It currently depends on the patch from Sam Weinig that made the WebPageProxy messages be generated.

[Kenneth Rohde Christiansen]

Created attachment 70143 [details]
Patch (missing mac build)

[WebKit Review Bot]

Attachment 70143 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/C/WKPage.h:156:  Extra space between WKPageViewportAttributesInvalidatedCallback and viewportAttributesInvalidated  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKViewportAttributes.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/WebViewportAttributes.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 70143 [details]
Patch (missing mac build)

I don't see anything obviously wrong with this patch other than missing a change to WebKit2Generated.make. But Sam and/or Anders and/or Maciej should have a look.

[Kenneth Rohde Christiansen]

Created attachment 70151 [details]
Patch (win fix)

[WebKit Review Bot]

Attachment 70151 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/C/WKPage.h:156:  Extra space between WKPageViewportAttributesInvalidatedCallback and viewportAttributesInvalidated  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKViewportAttributes.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/WebViewportAttributes.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Luiz Agostini]

Created attachment 70216 [details]
p

[Luiz Agostini]

Comment on attachment 70216 [details]
p

latest patch has some minor adjustments to the mac build system.

[WebKit Review Bot]

Attachment 70216 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/UIProcess/API/C/WKPage.h:156:  Extra space between WKPageViewportAttributesInvalidatedCallback and viewportAttributesInvalidated  [whitespace/declaration] [3]
WebKit2/UIProcess/API/C/WKViewportAttributes.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/WebViewportAttributes.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Anders and I took a look at this and we are a bit confused about why the client needs to know about the viewport at all. Couldn't WebKit handle this itself (maybe if it was put into a special mode where it knew to handle viewport args)?

[Kenneth Rohde Christiansen]

(In reply to comment #19)
> Anders and I took a look at this and we are a bit confused about why the client needs to know about the viewport at all. Couldn't WebKit handle this itself (maybe if it was put into a special mode where it knew to handle viewport args)?

Because it is the client that deals with zooming/scaling etc, so the client will have to enforce the limits. It is also the client that knows when a device was rotated and thus can update the layout (we also wants to do that at the right time because of ui animations etc).

[Kenneth Rohde Christiansen]

(In reply to comment #20)
> (In reply to comment #19)
> > Anders and I took a look at this and we are a bit confused about why the client needs to know about the viewport at all. Couldn't WebKit handle this itself (maybe if it was put into a special mode where it knew to handle viewport args)?
> 
> Because it is the client that deals with zooming/scaling etc, so the client will have to enforce the limits. It is also the client that knows when a device was rotated and thus can update the layout (we also wants to do that at the right time because of ui animations etc).

It is also not as simple as just setting the layout size as the client might be using a tiled backing store and thus using the setFixedLayout

[Kenneth Rohde Christiansen]

Would it be possible to discuss this on IRC?

The viewport meta handling depends on the device and UI:

- device height/width
- device DPI
- actual viewport given the UI (if you consider the iPhone, this is the size when all ui components are shown, incl. the address bar) something that changes given orientation.

The UI also is responsible for doing rotation animation, and adding the components, thus affecting the actual viewport.

All even handling such as panning, pinch zoom etc are handled by the UI, so the UI needs to enforce the limits. It is true that could could handle some of this in WebKit itself, but for instance with tiling (which allows non-blocking scrolling) we need to handle this on the UI.

We could make special views in WebKit2 that handles this behind the scenes and thus removes the public C API, but that is not really a possibility for Qt, as Qt and QtWebKit lays below the UI toolkit, in our toolchain.

[Andras Becsi]

(In reply to comment #22)
> We could make special views in WebKit2 that handles this behind the scenes and thus removes the public C API, but that is not really a possibility for Qt, as Qt and QtWebKit lays below the UI toolkit, in our toolchain.

Work around this issue and introduce new layering violations for this would be a big step backward (if it is possible at all), and I think Kenneth explained the use-cases thoroughly enough to justify why this API is needed, so I do not see any real blocker for this.

[Kenneth Rohde Christiansen]

Created attachment 70285 [details]
Patch (Qt API only)

[Kenneth Rohde Christiansen]

Created attachment 70286 [details]
Patch (without garbage)

[Kenneth Rohde Christiansen]

After discussion on IRC we decided to make this a Qt only API for now and make it go through the PageClient. The latest patch does exactly that.

Apple will probably just implement the handling in the view provided to iOS developers, which we unfortunately cannot do for Qt, hence adding the Qt-only API.

[Kenneth Rohde Christiansen]

Ping review?

[WebKit Commit Bot]

Comment on attachment 70286 [details]
Patch (without garbage)

Rejecting patch 70286 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 70286]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=70286&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=47202&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 70286 from bug 47202.
Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Sam Weinig', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4360022

[Kenneth Rohde Christiansen]

Thanks Sam. I will try landing this patch manually tomorrow.

[Kenneth Rohde Christiansen]

Comment on attachment 70286 [details]
Patch (without garbage)

Landed in 69490

[Kenneth Rohde Christiansen]

Win build fix committed in 69491