Summary: | Adding Qt WebKit2 API for dealing with viewport meta info | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abecsi, andersca, aroben, christian.webkit, commit-queue, darin, dbates, ddkilzer, gustavo, gyuyoung.kim, hausmann, joone, kbalazs, kimmo.t.kinnunen, kling, koivisto, luiz, ossy, sam, simon.fraser, tonikitoo, webkit.review.bot, zoltan | ||||||||||||||
Priority: | P3 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Kenneth Rohde Christiansen
2010-10-05 12:47:43 PDT
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. This API looks generally good to me, assuming that we need to leave the door open for future attribute additions. (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(...) (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. (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? Someone suggested calling this WKViewportFeaturesRef instead of WKViewportAttributesRef. I would like to know what name you think best describes the structure. (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". (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). I like attributes better than features, too, actually! Features came to my mind because of WindowFeatures. 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.
Created attachment 70143 [details]
Patch (missing mac build)
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.
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.
Created attachment 70151 [details]
Patch (win fix)
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.
Created attachment 70216 [details]
p
Comment on attachment 70216 [details]
p
latest patch has some minor adjustments to the mac build system.
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.
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)? (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). (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 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. (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. Created attachment 70285 [details]
Patch (Qt API only)
Created attachment 70286 [details]
Patch (without garbage)
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. Ping review? 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 commit-queue@webkit.org... 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 Thanks Sam. I will try landing this patch manually tomorrow. Comment on attachment 70286 [details]
Patch (without garbage)
Landed in 69490
Win build fix committed in 69491 |