[Webkit-unassigned] [Bug 109422] [Qt] Add Page Visibility API support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=109422


Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #189331|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #28 from Simon Hausmann <hausmann at webkit.org>  2013-02-21 05:01:12 PST ---
(From update of attachment 189331)
View in context: https://bugs.webkit.org/attachment.cgi?id=189331&action=review

> Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:164
> +void QWebPageAdapter::setVisibilityState(QWebPageAdapter::PageVisibilityState visibilityState, bool isInitialState)

I suggest to get rid of the bool isInitialState parameter. It looks very confusing on the caller side, it is always false except for _one_ case inside initialWebCorePage. In there I think we should simply call
    page->setVisibilityState(WebCore::PageVisibilityStateHidden, true);

> Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136
> +    enum PageVisibilityState {
> +        PageVisibilityStateVisible,
> +        PageVisibilityStateHidden
> +    };

Given that this is a binary state, why not simply use a boolean instead? It makes the translation to/from WebCore types easier I think. Just call it say setPageVisible(bool visible) or so - I think then you can get rid of some glue code.

> Source/WebKit/qt/WidgetApi/qwebview.cpp:736
> +#if ENABLE(PAGE_VISIBILITY_API)
> +        else if ((e->type() == QEvent::Show) || (e->type() == QEvent::WindowActivate))
> +            d->page->d->setVisibilityState(QWebPagePrivate::PageVisibilityStateVisible, false);
> +        else if ((e->type() == QEvent::Hide) || (e->type() == QEvent::WindowDeactivate))
> +            d->page->d->setVisibilityState(QWebPagePrivate::PageVisibilityStateHidden, false);
> +#endif

Imagine a tabbed browser. Would this still be accurate, i.e. if the QWebView is in a tab that is _not_ visible (because it's not current/active) but the window gets activated. Should the page visibility state change to visible?

I suggest to re-implement showEvent() and hideEvent(), pass on the event to d->page->event and call the base implementation. That gives advanced users the power to control the state if they need to (re-implement and call QWebView::showEvent or not if they like), and it still allows us to centralize the implementation inside QWebPageAdapter.

The same applies to QGraphicsWebView.

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:140
> +    int waitForEvent = 5;

I can almost guarantee you that this is going to fail on the bots. Randomly failing tests on the bot are a PITA.

I suggest to replace this unit test with implementing page visibility support in the Qt DRT. Then we can enable the layout tests for it as well.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.