[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 #35 from Benjamin Dupont <bdupont at nds.com>  2013-03-11 07:39:05 PST ---
(In reply to comment #32)
> (From update of attachment 192436 [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);? :)


(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]
> > [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
...?


(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

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

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

> 
> > > > 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?
> 
> It's the shell that is used to run the layout tests. You can find it in Tools/DumpRenderTree with the qt specific bits in Tools/DumpRenderTree/qt.
Yes, I discussed about that with carewolf.

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.