Bug 97324 – [GTK] Add support for Page Visibility

• Component: WebKit2
• Status: RESOLVED FIXED

[jaybhaskar]

MiniBrowser(WebKit2) and Gtklauncher(WebKit) both doesnot support Page Visibility on http://html5test.com

[Zan Dobersek]

Restyling the title a bit to make it clearer.

[Anton Obzhirov]

I am going to check and fix this one.

[Anton Obzhirov]

Created attachment 186591 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Comment on attachment 186591 [details]
Patch

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

Nice!  I think we should just ask via JavaScript for the page visibility state rather than making API for it. Some more minor comments follow. Didn't we agree that this would be called something like override_visibility state so that we could have a default implementation that relied on whether or not the widget was mapped?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1645
> +WebCore::PageVisibilityState WebPageProxy::visibilityState()
> +{
> +#if ENABLE(PAGE_VISIBILITY_API)
> +    return m_visibilityState;
> +#endif
> +}

Are we adding this WebCore API just for the sake of the unit tests? Why not simply ask JavaScript for the value of the page visibility state there?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2955
> + * @initial_state: a flag to specify if the visibility state is initial

Hrm. I'm not sure if I totally understand the initial_state flag here. It could definitely use some more documentation.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2957
> + * Set the page visibility of the page in @webView to @visibility_state.

It'd be a good idea to expand this documentation and potentially include a link to the specification.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:170
> + * WebKitPageVisibilityState:
> + * @WEBKIT_PAGE_VISIBILITY_STATE_VISIBLE: The document contained by the page is at least partially visible at on at least one screen. This is the same condition under which the hidden attribute is set to false.
> + * @WEBKIT_PAGE_VISIBILITY_STATE_HIDDEN: The document contained by the page is not visible at all on any screen.
> + * @WEBKIT_PAGE_VISIBILITY_STATE_PRERENDER: Optional. The document is prerendered by being loaded off-screen and not visibly shown.
> + * @WEBKIT_PAGE_VISIBILITY_STATE_PREVIEW: Optional. The document contained by the page is not visible at all and a preview of the document is visible.
> + *
> + * Enum values to specify the different visibility states for a #WebKitWebView
> + * web page.

This is a bit of a nit, but adding some newlines here would improve readability of the documentation. You could probably omit "Optional." here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1088
> +// To test page visibility API. Currently only 'visible' and 'hidden' states are implemented fully in WebCore.

If only visible and hidden are implemented in WebCore, maybe it's better to only expose them? Do the layout tests depend on all states?

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:881
> +void DumpRenderTreeSupportGtk::setPageVisibility(WebKitWebView *webView, const char* visibility)

An enum would be better than a string here. Watch that the location of your asterisks is correct in this file.

> Tools/Scripts/webkitperl/FeatureList.pm:371
> +      define => "ENABLE_PAGE_VISIBILITY_API", default => (isBlackBerry() || isEfl() || isGtk()), value => \$pageVisibilityAPISupport },

Since you are enabling it by default you shouldn't need to override it here as well.

[Anton Obzhirov]

Comment on attachment 186591 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1645
>> +}
> 
> Are we adding this WebCore API just for the sake of the unit tests? Why not simply ask JavaScript for the value of the page visibility state there?

I could do that. I thought if we add webkit_web_view_set_page_visibility to WebKit2 API, shouldn’t we add symmetrically webkit_web_view_get_page_visibility?

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2955
>> + * @initial_state: a flag to specify if the visibility state is initial
> 
> Hrm. I'm not sure if I totally understand the initial_state flag here. It could definitely use some more documentation.

Well, it basically means if the flag is set then override current visibility state with @visibility_state. Will update documentation here.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2957
>> + * Set the page visibility of the page in @webView to @visibility_state.
> 
> It'd be a good idea to expand this documentation and potentially include a link to the specification.

Ok

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:170
>> + * web page.
> 
> This is a bit of a nit, but adding some newlines here would improve readability of the documentation. You could probably omit "Optional." here.

ok

>> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1088
>> +// To test page visibility API. Currently only 'visible' and 'hidden' states are implemented fully in WebCore.
> 
> If only visible and hidden are implemented in WebCore, maybe it's better to only expose them? Do the layout tests depend on all states?

As far as I know no, they test only visible/hidden states. I was more thinking about future releases.

>> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:881
>> +void DumpRenderTreeSupportGtk::setPageVisibility(WebKitWebView *webView, const char* visibility)
> 
> An enum would be better than a string here. Watch that the location of your asterisks is correct in this file.

ok

>> Tools/Scripts/webkitperl/FeatureList.pm:371
>> +      define => "ENABLE_PAGE_VISIBILITY_API", default => (isBlackBerry() || isEfl() || isGtk()), value => \$pageVisibilityAPISupport },
> 
> Since you are enabling it by default you shouldn't need to override it here as well.

ok

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 186591 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186591&action=review
> 
> >> Source/WebKit2/UIProcess/WebPageProxy.cpp:1645
> >> +}
> > 
> > Are we adding this WebCore API just for the sake of the unit tests? Why not simply ask JavaScript for the value of the page visibility state there?
> 
> I could do that. I thought if we add webkit_web_view_set_page_visibility to WebKit2 API, shouldn’t we add symmetrically webkit_web_view_get_page_visibility?

Ah, if WebCore doesn't support that kind of use, it might better to go with the flow and not use it that way in both WebKit1 and WebKit2.

> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2955
> >> + * @initial_state: a flag to specify if the visibility state is initial
> > 
> > Hrm. I'm not sure if I totally understand the initial_state flag here. It could definitely use some more documentation.
> 
> Well, it basically means if the flag is set then override current visibility state with @visibility_state. Will update documentation here.

If you can only override the current state with a TRUE value here, what happens if you don't pass the flag? 

> > If only visible and hidden are implemented in WebCore, maybe it's better to only expose them? Do the layout tests depend on all states?
> 
> As far as I know no, they test only visible/hidden states. I was more thinking about future releases.

It's very easy to add new ENUM values to the API in a later release, but it's very difficult to remove them. :)

[Anton Obzhirov]

Comment on attachment 186591 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2955
>>>> + * @initial_state: a flag to specify if the visibility state is initial
>>> 
>>> Hrm. I'm not sure if I totally understand the initial_state flag here. It could definitely use some more documentation.
>> 
>> Well, it basically means if the flag is set then override current visibility state with @visibility_state. Will update documentation here.
> 
> If you can only override the current state with a TRUE value here, what happens if you don't pass the flag?

It will still change the state but JavaScript onVisibilityChange callback will be triggered additionally to it (may be something else as well, I need to double check). So flag means just update visibility state).

[Martin Robinson]

(In reply to comment #8)

> It will still change the state but JavaScript onVisibilityChange callback will be triggered additionally to it (may be something else as well, I need to double check). So flag means just update visibility state).

Interesting. So if you change the state and then query the visibility state via JavaScript will it give the old value or the new one?

[Anton Obzhirov]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > It will still change the state but JavaScript onVisibilityChange callback will be triggered additionally to it (may be something else as well, I need to double check). So flag means just update visibility state).
> 
> Interesting. So if you change the state and then query the visibility state via JavaScript will it give the old value or the new one?

I think it will still give you old value. I will need to double check it though.

[Anton Obzhirov]

Comment on attachment 186591 [details]
Patch

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

>>> Tools/Scripts/webkitperl/FeatureList.pm:371
>>> +      define => "ENABLE_PAGE_VISIBILITY_API", default => (isBlackBerry() || isEfl() || isGtk()), value => \$pageVisibilityAPISupport },
>> 
>> Since you are enabling it by default you shouldn't need to override it here as well.
> 
> ok

It seems I still need to add isGtk() line here because it overrides ENABLE_PAGE_VISIBILITY_API flag here during configure phase.

[Anton Obzhirov]

Created attachment 187854 [details]
Patch

[Martin Robinson]

Comment on attachment 187854 [details]
Patch

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

Looks good, apart from a couple nits.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2947
> + * Override the page visibility in @webView. The page visibility state will be set to @visibility_state.
> + * Can be used to set the page visibility state before @web_view is mapped.
> + *
> + * When the operation is finished, Java Script visibility change event will be triggered unless @initial_state flag is set.
> + * For more information about Page Visibility see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.

For the sake of readability, do you mind making these lines about as long as the other documentation? "Java Script" should be "JavaScript". :)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:167
> + * @WEBKIT_PAGE_VISIBILITY_STATE_VISIBLE: The document contained by the page is at least partially visible at on at least one screen. This is the same condition under which the hidden attribute is set to false.
> + * @WEBKIT_PAGE_VISIBILITY_STATE_HIDDEN: The document contained by the page is not visible at all on any screen.
> + *
> + * Enum values to specify the different visibility states for a #WebKitWebView

Perhaps make this a bit terser? I'm thinking something like "The page is at least partially visible and the hidden attribute is set to false." and then "The page is not visible at all on the screen."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:171
> +typedef enum
> +{

The curly brace here should be on the same line as the enum.

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h:49
> +    void showInWindow(GtkWindowType = GTK_WINDOW_POPUP);

You aren't using the "mapped" state of the widget here, so why do you actually need to show the window?

> Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp:966
> +    WebKitWebView* webView = webkit_web_frame_get_web_view(mainFrame);
> +    DumpRenderTreeSupportGtk::resetPageVisibility(webView);

Instead of exposing more API, why not just call  DumpRenderTreeSupportGtk::setPageVisibility(webView, WebCore::PageVisibilityStateVisible);

[Anton Obzhirov]

Comment on attachment 187854 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2947
>> + * For more information about Page Visibility see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.
> 
> For the sake of readability, do you mind making these lines about as long as the other documentation? "Java Script" should be "JavaScript". :)

No problem :)

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:167
>> + * Enum values to specify the different visibility states for a #WebKitWebView
> 
> Perhaps make this a bit terser? I'm thinking something like "The page is at least partially visible and the hidden attribute is set to false." and then "The page is not visible at all on the screen."

ok

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:171
>> +{
> 
> The curly brace here should be on the same line as the enum.

ok

>> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.h:49
>> +    void showInWindow(GtkWindowType = GTK_WINDOW_POPUP);
> 
> You aren't using the "mapped" state of the widget here, so why do you actually need to show the window?

To test normal use case (page visibility is changed indirectly via widget API) I want to show the window. To verify that page visibility is updated I need to wait for JavaScript callback and then check the page visibility. I am not sure which event will happen first - 'window mapped' or 'visibility changed', so I ignore first one and wait for the visibility change instead.

>> Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp:966
>> +    DumpRenderTreeSupportGtk::resetPageVisibility(webView);
> 
> Instead of exposing more API, why not just call  DumpRenderTreeSupportGtk::setPageVisibility(webView, WebCore::PageVisibilityStateVisible);

will have to add extra parameter in DumpRenderTreeSupportGtk::setPageVisibility(webView, WebCore::PageVisibilityStateVisible,
 bool)
 ^^^^
but yes can do that.

[Anton Obzhirov]

Created attachment 188901 [details]
Patch

[Martin Robinson]

Comment on attachment 188901 [details]
Patch

Removing r+ since this needs to be reviewed by an owner.

[Carlos Garcia Campos]

Comment on attachment 188901 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2940
> + * @visibility_state: a #WebKitWebViewPageVisibilityState

This says WebKitWebViewPageVisibilityState but the enum is called WebKitPageVisibilityState. I would avoid using Page here in any case, because it might be confused with WebKitWebPage API that is only available in the web extensions API. Maybe WebKitWebViewVisibiltyState, since our WebView is the equivalent to the WKPage in the C API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2951
> + * see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.

After reading this it's still not clear to me why and when a user of the API would call this method. Or is it only useful for testing? In such case it should be added to the C API which is what the tests use.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2960
> +    case WEBKIT_PAGE_VISIBILITY_STATE_VISIBLE:
> +        break;

I think it would be a bit clearer if state is set here instead of relying on the default value.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2965
> +        g_return_if_reached();

This should probably be g_assert_not_reached() instead, or even ASSERT_NOT_REACHED().

[Carlos Garcia Campos]

(In reply to comment #16)
> (From update of attachment 188901 [details])
> Removing r+ since this needs to be reviewed by an owner.

I think the patch could be split and land the wk1 part if it looks good to you and fixes the layout tests.

[Anton Obzhirov]

Comment on attachment 188901 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2940
>> + * @visibility_state: a #WebKitWebViewPageVisibilityState
> 
> This says WebKitWebViewPageVisibilityState but the enum is called WebKitPageVisibilityState. I would avoid using Page here in any case, because it might be confused with WebKitWebPage API that is only available in the web extensions API. Maybe WebKitWebViewVisibiltyState, since our WebView is the equivalent to the WKPage in the C API.

ok

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2951
>> + * see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.
> 
> After reading this it's still not clear to me why and when a user of the API would call this method. Or is it only useful for testing? In such case it should be added to the C API which is what the tests use.

Well, when the view is in background or is not mapped and you want to update visibility to change web application behaviour (for example on a page with video JavaScript onVisibilityChange callback pauses the video when the page is hidden. To get video playback to continue in background (to hear video audio for example) you can update visibility to 'visible'. In general it should be good to have such API for special use cases.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2960
>> +        break;
> 
> I think it would be a bit clearer if state is set here instead of relying on the default value.

ok

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2965
>> +        g_return_if_reached();
> 
> This should probably be g_assert_not_reached() instead, or even ASSERT_NOT_REACHED().

ok

[Carlos Garcia Campos]

(In reply to comment #19)
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2951
> >> + * see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.
> > 
> > After reading this it's still not clear to me why and when a user of the API would call this method. Or is it only useful for testing? In such case it should be added to the C API which is what the tests use.
> 
> Well, when the view is in background or is not mapped and you want to update visibility to change web application behaviour (for example on a page with video JavaScript onVisibilityChange callback pauses the video when the page is hidden. To get video playback to continue in background (to hear video audio for example) you can update visibility to 'visible'. In general it should be good to have such API for special use cases.

Isn't this a workaround for pages using such javascript callback? Is there already any app needing this API? I would avoid adding new API that is not yet needed and even more at this point of the release cycle.

[Anton Obzhirov]

(In reply to comment #20)
> (In reply to comment #19)
> > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2951
> > >> + * see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.
> > > 
> > > After reading this it's still not clear to me why and when a user of the API would call this method. Or is it only useful for testing? In such case it should be added to the C API which is what the tests use.
> > 
> > Well, when the view is in background or is not mapped and you want to update visibility to change web application behaviour (for example on a page with video JavaScript onVisibilityChange callback pauses the video when the page is hidden. To get video playback to continue in background (to hear video audio for example) you can update visibility to 'visible'. In general it should be good to have such API for special use cases.
> 
> Isn't this a workaround for pages using such javascript callback? Is there already any app needing this API? I would avoid adding new API that is not yet needed and even more at this point of the release cycle.

Well, as far as I know no such application exist yet. When speaking about C API do you mean UIProcess/API/C/gtk? If so I can move this API there and if in the future we need it in WebKit2 GTK API I can move it back. If everybody is happy with that I'll do it.

[Carlos Garcia Campos]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2951
> > > >> + * see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.
> > > > 
> > > > After reading this it's still not clear to me why and when a user of the API would call this method. Or is it only useful for testing? In such case it should be added to the C API which is what the tests use.
> > > 
> > > Well, when the view is in background or is not mapped and you want to update visibility to change web application behaviour (for example on a page with video JavaScript onVisibilityChange callback pauses the video when the page is hidden. To get video playback to continue in background (to hear video audio for example) you can update visibility to 'visible'. In general it should be good to have such API for special use cases.
> > 
> > Isn't this a workaround for pages using such javascript callback? Is there already any app needing this API? I would avoid adding new API that is not yet needed and even more at this point of the release cycle.
> 
> Well, as far as I know no such application exist yet. When speaking about C API do you mean UIProcess/API/C/gtk? If so I can move this API there and if in the future we need it in WebKit2 GTK API I can move it back. If everybody is happy with that I'll do it.

No, I mean the cross-platform C API that already has such API. In the WebKit1 API there are cases of APIs that I think have never been used, so for wk2 we decided to add API incrementally and only when there's a real use case or application requiring it, to try to keep the API simple and easier to maintain.

[Anton Obzhirov]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2951
> > > > >> + * see http://www.w3.org/TR/2011/WD-page-visibility-20110602/.
> > > > > 
> > > > > After reading this it's still not clear to me why and when a user of the API would call this method. Or is it only useful for testing? In such case it should be added to the C API which is what the tests use.
> > > > 
> > > > Well, when the view is in background or is not mapped and you want to update visibility to change web application behaviour (for example on a page with video JavaScript onVisibilityChange callback pauses the video when the page is hidden. To get video playback to continue in background (to hear video audio for example) you can update visibility to 'visible'. In general it should be good to have such API for special use cases.
> > > 
> > > Isn't this a workaround for pages using such javascript callback? Is there already any app needing this API? I would avoid adding new API that is not yet needed and even more at this point of the release cycle.
> > 
> > Well, as far as I know no such application exist yet. When speaking about C API do you mean UIProcess/API/C/gtk? If so I can move this API there and if in the future we need it in WebKit2 GTK API I can move it back. If everybody is happy with that I'll do it.
> 
> No, I mean the cross-platform C API that already has such API. In the WebKit1 API there are cases of APIs that I think have never been used, so for wk2 we decided to add API incrementally and only when there's a real use case or application requiring it, to try to keep the API simple and easier to maintain.

Makes sense. I will update the tests to use existing C API and submit new patch.

[Anton Obzhirov]

Created attachment 189506 [details]
Patch

[Martin Robinson]

Comment on attachment 189506 [details]
Patch

Carlos, I think it's fine that the unit tests for WebKit2 are in the same patch as the WebKit1 DRT support. This patch is small enough and the subject matter is the same: "enable page visibility." This looks good to me (perhaps Carlos has some comments), but we need a WebKit2 owner to review the WebKit2 bits.

[Carlos Garcia Campos]

(In reply to comment #25)
> (From update of attachment 189506 [details])
> Carlos, I think it's fine that the unit tests for WebKit2 are in the same patch as the WebKit1 DRT support. This patch is small enough and the subject matter is the same: "enable page visibility." This looks good to me (perhaps Carlos has some comments), but we need a WebKit2 owner to review the WebKit2 bits.

Isn't page visibility already covered by fast/events/page-visibility* layout tests? Unit tests are to test the API and this patch doesn't include new WebKit2 API.

[Anton Obzhirov]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 189506 [details] [details])
> > Carlos, I think it's fine that the unit tests for WebKit2 are in the same patch as the WebKit1 DRT support. This patch is small enough and the subject matter is the same: "enable page visibility." This looks good to me (perhaps Carlos has some comments), but we need a WebKit2 owner to review the WebKit2 bits.
> 
> Isn't page visibility already covered by fast/events/page-visibility* layout tests? Unit tests are to test the API and this patch doesn't include new WebKit2 API.

It is covered but from web application (JavaScript) point of view. I think it still should check that changes in GTK widget visibility triggers right JavaScript callbacks.

[Martin Robinson]

(In reply to comment #27)

> It is covered but from web application (JavaScript) point of view. I think it still should check that changes in GTK widget visibility triggers right JavaScript callbacks.

I agree here. Without this test we have no test coverage of this particular aspect of the widget.

[Carlos Garcia Campos]

(In reply to comment #28)
> (In reply to comment #27)
> 
> > It is covered but from web application (JavaScript) point of view. I think it still should check that changes in GTK widget visibility triggers right JavaScript callbacks.
> 
> I agree here. Without this test we have no test coverage of this particular aspect of the widget.

Ok, if you both think it's a good idea, I'm fine.

[Anton Obzhirov]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > 
> > > It is covered but from web application (JavaScript) point of view. I think it still should check that changes in GTK widget visibility triggers right JavaScript callbacks.
> > 
> > I agree here. Without this test we have no test coverage of this particular aspect of the widget.
> 
> Ok, if you both think it's a good idea, I'm fine.

I am going to resubmit the patch now to fix build problem (probably happened because of incremental build) .

[Anton Obzhirov]

Created attachment 190317 [details]
Patch

[Martin Robinson]

This looks good to me, but a WebKit2 owner needs to do the final review.

[Carlos Garcia Campos]

(In reply to comment #32)
> This looks good to me, but a WebKit2 owner needs to do the final review.

Looks good to me as well, fwiw

[Martin Robinson]

Anders, Sam, do you mind doing a review here for the WebKit2 bits? Two GTK+ reviewers have approved this patch otherwise. Thanks!

[Anton Obzhirov]

Hi, Anders, Sam
do you have something to comment about the patch? If not may be we can just submit it finally.

[Mario Sanchez Prada]

Committed r148088: <http://trac.webkit.org/changeset/148088>