[webkit-reviews] review denied: [Bug 109422] [Qt] Add Page Visibility API support : [Attachment 189331] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (5)

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Benjamin Dupont
<bdupont at nds.com>'s request for review:
Bug 109422: [Qt] Add Page Visibility API support
https://bugs.webkit.org/show_bug.cgi?id=109422

Attachment 189331: [Qt][Wk1] Add page visibility API support in QWebView and
QGraphicsWebView + tests (5) 
https://bugs.webkit.org/attachment.cgi?id=189331&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.