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

Mon Mar 11 07:55:36 PDT 2013

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

--- Comment #36 from Simon Hausmann <hausmann at webkit.org>  2013-03-11 07:58:01 PST ---
(In reply to comment #35)
> (In reply to comment #32)
> > (From update of attachment 192436 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=192436&action=review
> > 
> > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:160
> > > +void QWebPageAdapter::setPageVisible(bool visible)
> > 
> > Since this function doesn't actually make the page visible or not (in the graphical sense of it) but just changes the state, I suggest to call it just like the WebCore::Page function: setVisibilityState
> > 
> > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:247
> > > +    void setPageVisible(bool);
> > 
> > This looks much neater :)
> 
> Thus, please confirm what do you prefer because previously you said me:
> >Just call it say setPageVisible(bool visible) or so - I think then you can get rid of some glue code.
> Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:160
> void QWebPageAdapter::setVisibilityState(bool visible)
> Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:247
> void setVisibilityState(bool);? :)

I'm going to pull the "... or so" card here ;-)

But yeah, what I wanted to express is that I like this change. I think this API is simpler and leaner. (the one you have in your patch now)

After seeing the surrounding code I personally like setVisibilityState(bool visible) as _name_.

> (In reply to comment #33)
> > (In reply to comment #31)
> > > Created an attachment (id=192436)
 --> (https://bugs.webkit.org/attachment.cgi?id=192436&action=review) [details] [details] [details]
> > > [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView
> > > 
> > > New patch regarding comments from #28 to #30.
> > > No more tests API because there is no more new public API.
> > > Layout tests regarding visibility API are already available:
> > > LayoutTests/fast/events/page-visibility-transition-test.html
> > > LayoutTests/fast/events/page-visibility-iframe-delete-test.html
> > > LayoutTests/fast/events/page-visibility-iframe-delete-test.html
> > > LayoutTests/fast/events/page-visibility-iframe-propagation-test.html
> > > LayoutTests/fast/events/page-visibility-iframe-propagation-test.html
> > > LayoutTests/fast/events/page-visibility-iframe-move-test.html
> > 
> > They are available, but skipped. From LayoutTests/platform/qt/TestExpectations:
> > 
> > "
> > # This platform does not support the Page Visibility API.
> > fast/events/page-visibility-iframe-delete-test.html
> > fast/events/page-visibility-iframe-move-test.html
> > fast/events/page-visibility-iframe-propagation-test.html
> > fast/events/page-visibility-null-view.html
> > fast/events/page-visibility-transition-test.html
> > fast/frames/page-visibility-crash.html
> > "
> Ok, can I just enable it like that:
> webkit.org/b/109422 fast/events/page-visibility-iframe-delete-test.html
> webkit.org/b/109422 fast/events/page-visibility-iframe-move-test.html
> ...?

No, that would just continue to skip the test. Instead the necessary hooks need to be implemented in the Qt build of DumpRenderTree and then those lines need to be removed, so that they are not skipped anymore. Tests referred to in this file are skipped from the test runs or are marked here as known to fail - in this case we want them to pass when this patch lands.

> (In reply to comment #34)
> > (In reply to comment #29)
> > [...]
> > > > 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?
> > 
> > I don't see that requirement. Can you elaborate?
> > 
> http://www.w3.org/TR/page-visibility

That's the link to the spec, yes. But I don't see how the spec requires us to act on Qt's event specifically.

> > I do see quite the opposite. Take the examples of the "hidden" attribute for example: "The User Agent is not minimized, but the page is on a background tab."
> I think if a page is on a background tab then it should receive a WindowDeactivate event and then visibility state should be state to "hidden" as David Rosca said:
> (In reply to comment #30)
> > > When the window is activated/deactivated a showEvent/hideEvent is fired to the current QWebView? If yes, I'll remove WindowDeactivate and WindowActivate case.
> > 
> > It works ok with tabbed browser in current implementation. Only current tab will change its state to visible on WindowActivate event.

With what tabbed browser? We don't want to mandate how exactly to design the tabs, i.e. require the use of say QTabWidget.

Think of custom UIs in QGraphicsView for example.

> > We can _not_ provide a fully comprehensive implementation of this attribute that works out of the box in all cases. We do _need_ to give advanced users control over providing the visibility information to the web application.
> If they reimplement "virtual bool event(QEvent*)" they can make this kind of stuff?

Yes, they can. And I want to make it even easier, because ::event() is a catch-all that is tricky to get rid (call the base implementation first? only for some events?).

I think it is a better practice to use specific virtual event functions where we can, and in this case they would combine nicely with our initial page visibility implementation based on Qt's show and hide events. They are basic and well defined.

If we can't figure out a sensible basic implementation then perhaps we should just leave it at a public function in QWebPage and document how application developers can support web applications that use this web-facing visibility API.

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