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

[bugzilla-daemon at webkit.org]

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





--- Comment #29 from Benjamin Dupont <bdupont at nds.com>  2013-03-04 04:59:22 PST ---
(In reply to comment #28)
> (From update of attachment 189331 [details])
> 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);

Ok.

> 
> > 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.

Ok.

> 
> > 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?

No, it shouldn't... We should also manage:
a) if the QWebView is in active tab and the window gets deactivated -> change state from visible to hidden
b) if the QWebView is in active tab and the window gets activated -> change state from hidden to visible

When the window is activated/deactivated a showEvent/hideEvent is fired to the current QWebView? If yes, I'll remove WindowDeactivate and WindowActivate case.

> 
> 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.
> 

It's an HTML5 requirement, why should we want to give to advanced users the power to control the state and thus change the expected behaviour defined by W3C?

> The same applies to QGraphicsWebView.
> 

I'm not sure, as I mentioned previously:
"After adding QGraphicsWebView on a scene, QGraphicsWebView receives a show event even if the bound QGraphicsView isn’t visible.
After a show on QGraphicsView, QGraphicsWebView receive only a WindowActivate event.
Furthermore when a QGraphicsWebView was created, it’s visible by default even if it isn't rendered."
If I only re-implement showEvent() and hideEvent(), when you hide the scene the visibility state won't be changed (because hideEvent isn't called)... 

> > 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.

Yes I agree with you but I don't know how to do it differently...
What is the Qt DRT? Where can I find an example?

Thank you for your feedback.

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