Add Page Visibility API support to WebKit1 implementation for Qt.
Created attachment 187556 [details] [WK1] Add Page Visibility API support.
Comment on attachment 187556 [details] [WK1] Add Page Visibility API support. Attachment 187556 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16497178
Comment on attachment 187556 [details] [WK1] Add Page Visibility API support. Attachment 187556 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16492238
Comment on attachment 187556 [details] [WK1] Add Page Visibility API support. Page no more available in QWebPagePrivate class.
Created attachment 187588 [details] [WK1] Add Page Visibility API support. (2)
Created attachment 187593 [details] [WK1] Add Page Visibility API support. (3)
Comment on attachment 187593 [details] [WK1] Add Page Visibility API support. (3) View in context: https://bugs.webkit.org/attachment.cgi?id=187593&action=review Needs documentation and API test. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:921 > > +void QWebPage::setVisibilityState(int visibilityState, bool isInitialState) Documentation?
Regarding the QWebView implementation I'll attach a patch with documentation and auto test soon. Regarding the QGraphicsWebView, after a lot of tests it seems that QGraphicsScene and QGraphicsView was totally decorrelated from QGraphicsWebView. 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. I can implement visibility API in override setVisible and by using WindowActivate/WindowDeactivate/show/hide. What do you think about this proposal?
Shouldn't be QWebView::setVisible public instead of private? As in other QWidget subclasses? Also, I think that page visibility should be changed according to Show, Hide, WindowActivate and WindowDeactivate events. Otherwise applications using QtWebKit has to show/hide QWebView manually. I have modified this patch for QtWebKit 2.3, here is the patch: https://gist.github.com/nowrep/4966923 Please take a look at QWebView::event
Oops, just noticed that you already wrote about those events. Sorry.
Created attachment 188927 [details] HTML test page
Created attachment 189047 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests
Comment on attachment 189047 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests Attachment 189047 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16611736
Comment on attachment 189047 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests Attachment 189047 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16613658
Created attachment 189052 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (2)
Comment on attachment 189052 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (2) Attachment 189052 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16627493
Comment on attachment 189052 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (2) Attachment 189052 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16627494
Created attachment 189060 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (3)
Comment on attachment 189060 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (3) View in context: https://bugs.webkit.org/attachment.cgi?id=189060&action=review Almost there I think. The only thing I'd like cleaned up still is the introduction of a QWebPage::PageVisibility enum, which should address most of my remaining comments, but I think it's mostly my fault for not making that super clear when I mentioned it before on IRC. > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:1490 > + \sa Page::setVisibilityState(PageVisibilityState visibilityState, bool isInitialState) QWebPageAdapter and friends aren't public API, no documentation needed here. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:930 > + \sa QWebPageAdapter::setVisibilityState(int visibilityState, bool isInitialState) Not public API, so no need for a "See Also" entry here. But there could be mention that QWebView and QGraphicsWebView already do "the right thing"™ and that exposing this as public API in QWebPage is more geared towards "headless" use cases. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:932 > +void QWebPage::setVisibilityState(int visibilityState, bool isInitialState) here, the integer parameter is disturbing because it's a public API and this is not a very Qt-ish thing to do. I think a QWebPage::PageVisibility enum needs to be introduced and documented. For the sake of simplicity, it can follow the values of the corresponding WebCore enum and cast back in forth to and from int through the QtWebKit layer, but in that case it's probably best to add a comment before the enum in QWebPage mentioning it has to be kept in sync with the WebCore one. (not something that's likely to change much on either side I believe). > Source/WebKit/qt/WidgetApi/qwebview.cpp:733 > + d->page->setVisibilityState(WebCore::PageVisibilityStateVisible, false); Small layering violation here: we can't use WebCore types or enums in libQtWebKitWidgets. But the QWebPage version can be used once it's introduced.
Comment on attachment 189060 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (3) View in context: https://bugs.webkit.org/attachment.cgi?id=189060&action=review > Source/WebKit/qt/WidgetApi/qgraphicswebview.cpp:412 > + if (scene()->isActive()) Mhh, this feels a bit flimsy. Don't we risk missing our opportunity to set the proper page visibility later on if the scene is activated ? > Source/WebKit/qt/WidgetApi/qgraphicswebview.cpp:413 > + d->page->setVisibilityState(WebCore::PageVisibilityStateVisible, false); and WebCore enum use here... > Source/WebKit/qt/WidgetApi/qgraphicswebview.cpp:415 > + d->page->setVisibilityState(WebCore::PageVisibilityStateHidden, false); and here.
Comment on attachment 189060 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (3) View in context: https://bugs.webkit.org/attachment.cgi?id=189060&action=review > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:153 > + view.show(); > + QTest::qWait(5); I think this is likely to fail with different windowing systems. I think it would be better to use functionality like qWaitForWindowExposed. Now I suppose we'd need a similar function in QTestLib for waitForWindowObscured or Hidden?
(In reply to comment #21) > (From update of attachment 189060 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189060&action=review > > > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:153 > > + view.show(); > > + QTest::qWait(5); > > I think this is likely to fail with different windowing systems. I think it would be better to use functionality like qWaitForWindowExposed. Now I suppose we'd need a similar function in QTestLib for waitForWindowObscured or Hidden? Yes but qWaitForWindowExposed it's just for the scene is "rendered", correct? What about the QGraphicsWebView show event? Is there any way to know when an event is received? qWait(5) is an empirical way to be sure that QGraphicsWebView has received the show/hide/etc. events, if there is another way to do it I'm very interested about it :) I'll look for adding waitForWindowObscured function.
(In reply to comment #20) > (From update of attachment 189060 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189060&action=review > > > Source/WebKit/qt/WidgetApi/qgraphicswebview.cpp:412 > > + if (scene()->isActive()) > > Mhh, this feels a bit flimsy. > Don't we risk missing our opportunity to set the proper page visibility later on if the scene is activated ? I must check what's happenning if a scene is activated and we add a QGraphicsWebView hidden. > > > Source/WebKit/qt/WidgetApi/qgraphicswebview.cpp:413 > > + d->page->setVisibilityState(WebCore::PageVisibilityStateVisible, false); > > and WebCore enum use here... > > > Source/WebKit/qt/WidgetApi/qgraphicswebview.cpp:415 > > + d->page->setVisibilityState(WebCore::PageVisibilityStateHidden, false); > > and here. Why can't I use WebCore::PageVisibilityState enum here? Is it better to use a 'int' value?
(In reply to comment #23) > (In reply to comment #20) > > (From update of attachment 189060 [details] [details]) (WebCore::PageVisibilityStateHidden, false); > > > > and here. > Why can't I use WebCore::PageVisibilityState enum here? Is it better to use a 'int' value? Oups sorry, I just read your previous comment: "Small layering violation here: we can't use WebCore types or enums in libQtWebKitWidgets. But the QWebPage version can be used once it's introduced."
Created attachment 189327 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (4)
Attachment 189327 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h', u'Source/WebKit/qt/WidgetApi/qgraphicswebview.cpp', u'Source/WebKit/qt/WidgetApi/qgraphicswebview.h', u'Source/WebKit/qt/WidgetApi/qwebpage.cpp', u'Source/WebKit/qt/WidgetApi/qwebview.cpp', u'Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp', u'Source/WebKit/qt/tests/qwebview/tst_qwebview.cpp']" exit_code: 1 Source/WebKit/qt/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 189331 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (5)
Comment on attachment 189331 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView + tests (5) 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.
(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.
> 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.
Created attachment 192436 [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
Comment on attachment 192436 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView 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 :)
(In reply to comment #31) > Created an attachment (id=192436) [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 "
(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? 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." 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. > > > 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.
(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) [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 :)
(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) [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.
(In reply to comment #36) > After seeing the surrounding code I personally like setVisibilityState(bool visible) as _name_. Ok. :) > 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. Ok, thank you for these explanations. I'll try to implement the necessary hooks. > 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. > 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. > > > 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. Now I better understand what you mean. My first implementation contained this kind of public API: https://bugs.webkit.org/attachment.cgi?id=189060&action=prettypatch In this case we just provided a way to implement visibility functionalities as described in W3C, but we don't implement it for "basic" use cases. I don't know what is the best... In our side we implemented it directly in QWebView (as proposed in my last patch) because we have different projects and this way it seemed to be more relevant.
Comment on attachment 192436 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView Attachment 192436 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17113617 New failing tests: editing/selection/selection-modify-crash.html
Created attachment 193151 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView (new API + new Layout test) Existing visibility layout tests are implementation dependant and they test optional states as mandatory states.
Comment on attachment 193151 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView (new API + new Layout test) Attachment 193151 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17191093
Created attachment 193154 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView (new API + new Layout test) (2)
Comment on attachment 193154 [details] [Qt][Wk1] Add page visibility API support in QWebView and QGraphicsWebView (new API + new Layout test) (2) Attachment 193154 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17132158
View in context: https://bugs.webkit.org/attachment.cgi?id=193154&action=review https://developer.mozilla.org/en-US/docs/DOM/Using_the_Page_Visibility_API says, "Developers have historically used imperfect proxies to detect this. For example, registering an onblur/onfocus handler on the window helps you know when your page is not the active page, but it does not tell you that your page is hidden to the user. The Page Visibility API addresses this. (When compared with registering onblur/onfocus handlers on the window, __a key difference is that a page does not become hidden when another window is made active and the browser window loses focus. A page only becomes hidden when the user switches to a different tab or minimizes the browser window__.)" > Source/WebKit/qt/WidgetApi/qwebview.cpp:712 > + if ((e->type() == QEvent::Show) || (e->type() == QEvent::WindowActivate)) > + d->page->d->setVisibilityState(true); > + else if ((e->type() == QEvent::Hide) || (e->type() == QEvent::WindowDeactivate)) > + d->page->d->setVisibilityState(false); WindowActive/WindowDeactivate will be called when the window lost/gains focus. But the main aim of the page __visibilitychange__ is to notify when the window becomes really hidden from user(either tab switching or minimizing). But if we do it implicitly in WindowActivate/WindowDeactivate events then there will be no difference between onfocus/onblur(DOM window events).
(In reply to comment #43) > View in context: https://bugs.webkit.org/attachment.cgi?id=193154&action=review > > https://developer.mozilla.org/en-US/docs/DOM/Using_the_Page_Visibility_API says, > > "Developers have historically used imperfect proxies to detect this. For example, registering an onblur/onfocus handler on the window helps you know when your page is not the active page, but it does not tell you that your page is hidden to the user. The Page Visibility API addresses this. (When compared with registering onblur/onfocus handlers on the window, __a key difference is that a page does not become hidden when another window is made active and the browser window loses focus. A page only becomes hidden when the user switches to a different tab or minimizes the browser window__.)" > > > Source/WebKit/qt/WidgetApi/qwebview.cpp:712 > > + if ((e->type() == QEvent::Show) || (e->type() == QEvent::WindowActivate)) > > + d->page->d->setVisibilityState(true); > > + else if ((e->type() == QEvent::Hide) || (e->type() == QEvent::WindowDeactivate)) > > + d->page->d->setVisibilityState(false); > > WindowActive/WindowDeactivate will be called when the window lost/gains focus. But the main aim of the page __visibilitychange__ is to notify when the window becomes really hidden from user(either tab switching or minimizing). But if we do it implicitly in WindowActivate/WindowDeactivate events then there will be no difference between onfocus/onblur(DOM window events). In my understanding, onfocus/onblur are called on QEvent::FocusIn/QEvent::FocusOut received by the QWebPage. When a QWebView loses the focus, do we receive a focus out event and a WindoDeactivate event?
(In reply to comment #44) > When a QWebView loses the focus, do we receive a focus out event and a WindoDeactivate event? FocusIn & FocusOut is intended for widgets, WindowActivate & WindowDeactivate is intended for Windows.
Created attachment 206447 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(bool visible) New patch regarding all previous comments. The best solution to this issue seems to provide a QWebPage public API that could be called by advanced user (e.g in show/hide/visible functions/events of QGraphicsWebView/QWebView).
Attachment 206447 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h', u'Source/WebKit/qt/WidgetApi/qwebpage.cpp', u'Source/WebKit/qt/WidgetApi/qwebpage.h', u'Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp']" exit_code: 1 Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3151: Tab found; better to use spaces [whitespace/tab] [1] Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3161: Tab found; better to use spaces [whitespace/tab] [1] Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3171: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 206449 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(bool visible) (2) New patch regarding all previous comments. The best solution to this issue seems to provide a QWebPage public API that could be called by advanced user (e.g in show/hide/visible functions/events of QGraphicsWebView/QWebView).
Comment on attachment 206449 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(bool visible) (2) After IRC discussion with carewolf, jturcotte and tronical, I'll provide a new patch with: - enum with different states (also optionals) - Q_PROPERTY - QWebPage::setVisibilityState - QWebPage::visibilityState - more comments regarding: --- the initial state, --- the propagation until the JS --- how/where/when to call it
Created attachment 207923 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state)
Comment on attachment 207923 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) I must remove webkit suffix (eg. webkitHidden -> hidden) from the test...
Created attachment 207926 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (2)
Comment on attachment 207926 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (2) View in context: https://bugs.webkit.org/attachment.cgi?id=207926&action=review > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136 > + // Must match with values of PageVisibilityState enum from PageVisibilityState.h. I think we should do a switch-case matching in QWebPageAdapter::setVisibilityState to make sure that the enum values are independent from WebCore. It's usually not a good idea to have the public header follow internal enums directly. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3161 > + \note The initial state is set to PageVisibilityStateHidden if the page visibility API is enabled, > + otherwise is set to PageVisibilityApiUnavailable. What about all applications out there not knowing about this new API, isn't it a bad default to have page told that they are hidden even though they are shown? > Source/WebKit/qt/WidgetApi/qwebpage.h:220 > + // Must match with values of PageVisibilityState enum from QWebPageAdapter.h. Please leave this information in QWebPageAdapter.h not to pollute the public header. > Source/WebKit/qt/WidgetApi/qwebpage.h:226 > + PageVisibilityApiUnavailable We should simply enable the compile-time flag all the time if we're exposing the API. A feature is usually not enabled only if it isn't implemented or ready.
(In reply to comment #53) > (From update of attachment 207926 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207926&action=review > > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136 > > + // Must match with values of PageVisibilityState enum from PageVisibilityState.h. > > I think we should do a switch-case matching in QWebPageAdapter::setVisibilityState to make sure that the enum values are independent from WebCore. It's usually not a good idea to have the public header follow internal enums directly. Initially I made a switch-case and then I delete it... :) For the default value, should I return the hidden value? > > > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3161 > > + \note The initial state is set to PageVisibilityStateHidden if the page visibility API is enabled, > > + otherwise is set to PageVisibilityApiUnavailable. > > What about all applications out there not knowing about this new API, isn't it a bad default to have page told that they are hidden even though they are shown? I agree, in the current implementation if the visibility API isn't enable the visibility attributes are available even so and set to hidden. That's why I used this default value. > > > Source/WebKit/qt/WidgetApi/qwebpage.h:220 > > + // Must match with values of PageVisibilityState enum from QWebPageAdapter.h. > > Please leave this information in QWebPageAdapter.h not to pollute the public header. Ok. > > > Source/WebKit/qt/WidgetApi/qwebpage.h:226 > > + PageVisibilityApiUnavailable > > We should simply enable the compile-time flag all the time if we're exposing the API. A feature is usually not enabled only if it isn't implemented or ready. In this case should I remove all PAGE_VISIBILITY_API references: Source/WTF/wtf/FeatureDefines.h Source/WebCore/dom/Document.cpp Source/WebCore/dom/Document.h Source/WebCore/page/Frame.cpp Source/WebCore/page/Frame.h Source/WebCore/page/Page.cpp Source/WebCore/page/Page.h Source/WebCore/page/PageVisibilityState.cpp Source/WebCore/page/PageVisibilityState.h Source/WebCore/page/Settings.cpp Source/WebCore/page/Settings.h Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp Tools/Scripts/webkitperl/FeatureList.pm Tools/qmake/mkspecs/features/features.pri And WebKit2 files? Or just doing the assumption that this feature is always enabled in QWebPageAdapter?
(In reply to comment #54) > > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136 > > > + // Must match with values of PageVisibilityState enum from PageVisibilityState.h. > Initially I made a switch-case and then I delete it... :) For the default value, should I return the hidden value? The default shouldn't happen as we should still try to maintain the match between the enums. So just put an ASSERT(FALSE) and return any reasonable default value. > I agree, in the current implementation if the visibility API isn't enable the visibility attributes are available even so and set to hidden. That's why I used this default value. From my understanding, if the visibility API isn't enabled, document.visibilityState and document.hidden both return 'undefined'. When enabled I think that default value should be 'visible'. > > We should simply enable the compile-time flag all the time if we're exposing the API. A feature is usually not enabled only if it isn't implemented or ready. > In this case should I remove all PAGE_VISIBILITY_API references: > Source/WTF/wtf/FeatureDefines.h > Source/WebCore/dom/Document.cpp > Source/WebCore/dom/Document.h > Source/WebCore/page/Frame.cpp > Source/WebCore/page/Frame.h > Source/WebCore/page/Page.cpp > Source/WebCore/page/Page.h > Source/WebCore/page/PageVisibilityState.cpp > Source/WebCore/page/PageVisibilityState.h > Source/WebCore/page/Settings.cpp > Source/WebCore/page/Settings.h > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp > Tools/Scripts/webkitperl/FeatureList.pm > Tools/qmake/mkspecs/features/features.pri > And WebKit2 files? > > Or just doing the assumption that this feature is always enabled in QWebPageAdapter? We should just make the feature enabled by default in our build system and assume that the feature is enabled in Qt-specific sources. We shouldn't change cross-port code as other port might want or not to enable this feature.
Created attachment 208125 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (3)
Comment on attachment 208125 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (3) Attachment 208125 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1369197
Comment on attachment 208125 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (3) Attachment 208125 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1375130
Comment on attachment 208125 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (3) View in context: https://bugs.webkit.org/attachment.cgi?id=208125&action=review > Source/WebKit/qt/ChangeLog:3 > + [Qt] Add Page Visibility API support Ideally this patch should also enable PAGE_VISIBILITY_API by default in the build, but this can also be done in a separate patch as long as you do it soon. > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:159 > + result = WebCore::PageVisibilityStatePrerender; > + break; you can replace the temporary result variable by "return WebCore::PageVisibilityStatePrerender;" directly and also remove the breaks and the final return. > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:247 > + page->setVisibilityState(webPageVisibilityStateToWebCoreVisibilityState(m_visibilityState), true); Page::m_visibilityState is already initialized to PageVisibilityStateVisible, I'm not sure I see a use for this. > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:339 > + if (!page) > + return; By looking at the rest of the code I'd say that you can expect page to be non-null here, am I right? > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:349 > + return m_visibilityState; Why not use WebCore::Page::visibilityState() directly instead? > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136 > + // Must match with values of PageVisibilityState enum from PageVisibilityState.h. It's not true anymore, it must however match QWebPage::PageVisibilityState. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3142 > + Returns the current page visibility state of the current page. "of the current page" is unneeded extra information. I think it's obvious. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3152 > + This method is used to change the page visibility state of the current page. ditto > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3153 > + It should be called by Qt applications who want to notify Javascript application grammar: "JavaScript applications" or "the JavaScript application" > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3154 > + that the visibility state has changed (eg. by reimplementing setVisible function of QWidget). - eg. --> e.g. - "the setVisible function" or simply "QWidget::setVisible" (the later should also add a link automatically in the doc) The rest of the method documentaion is pretty good and descriptive :) > Source/WebKit/qt/WidgetApi/qwebpage.h:220 > + enum PageVisibilityState { VisibilityState would be fine and match with the property name. "Page" isn't helpful here.
Created attachment 208128 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (4)
(In reply to comment #59) > (From update of attachment 208125 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208125&action=review > > > Source/WebKit/qt/ChangeLog:3 > > + [Qt] Add Page Visibility API support > > Ideally this patch should also enable PAGE_VISIBILITY_API by default in the build, but this can also be done in a separate patch as long as you do it soon. I think PAGE_VISIBILITY_API is already activate by default (in Tools/qmake/mkspecs/features/features.pri ENABLE_PAGE_VISIBILITY_API=1). Is there any other places to enable it? > > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:159 > > + result = WebCore::PageVisibilityStatePrerender; > > + break; > > you can replace the temporary result variable by "return WebCore::PageVisibilityStatePrerender;" directly and also remove the breaks and the final return. I totally agree with you, I wanted to be inspired by dragOpToDropAction function but it seems it was a bad idea :) > > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:247 > > + page->setVisibilityState(webPageVisibilityStateToWebCoreVisibilityState(m_visibilityState), true); > > Page::m_visibilityState is already initialized to PageVisibilityStateVisible, I'm not sure I see a use for this. Yes with the visible default value is no more needed. > > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:339 > > + if (!page) > > + return; > > By looking at the rest of the code I'd say that you can expect page to be non-null here, am I right? Yes, but as I saw "return page ? page->viewportArguments() : WebCore::ViewportArguments();", I'd preferred to add this check. > > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:349 > > + return m_visibilityState; > > Why not use WebCore::Page::visibilityState() directly instead? Yes I agree. (getter/setter habit mistake...) I'll remove the m_visibilityState attribute because it's never used... > > > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136 > > + // Must match with values of PageVisibilityState enum from PageVisibilityState.h. > > It's not true anymore, it must however match QWebPage::PageVisibilityState. > Erf I didn't update this comment. Yes, if there is a difference between these values and the ones defined in WebCore it'll generate a compile error into the switch (like for this patch). > > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3153 > > + It should be called by Qt applications who want to notify Javascript application > > grammar: "JavaScript applications" or "the JavaScript application" > Oops, thanks. > > Source/WebKit/qt/WidgetApi/qwebpage.h:220 > > + enum PageVisibilityState { > > VisibilityState would be fine and match with the property name. "Page" isn't helpful here. Ok, is the same for QWebPageAdapter.h?
(In reply to comment #61) > I think PAGE_VISIBILITY_API is already activate by default (in Tools/qmake/mkspecs/features/features.pri ENABLE_PAGE_VISIBILITY_API=1). Is there any other places to enable it? Ah yes you're right! > > VisibilityState would be fine and match with the property name. "Page" isn't helpful here. > Ok, is the same for QWebPageAdapter.h? Since it should match QWebPage more than WebCore I'd say that we should remove Page from this one as well, but I don't have a strong preference.
Created attachment 208132 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (5)
Comment on attachment 208132 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (5) View in context: https://bugs.webkit.org/attachment.cgi?id=208132&action=review Found a few more minor issues on the last comb pass. > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:259 > + VisibilityState visibilityState(); const missing here > Source/WebKit/qt/WidgetApi/qwebpage.h:82 > + Q_PROPERTY(QWebPage::VisibilityState visibilityState READ visibilityState WRITE setVisibilityState) > Q_ENUMS(LinkDelegationPolicy NavigationType WebAction) I think you need to add VisibilityState to the Q_ENUMS line here for the property to work. Also I'm not sure if the "QWebPage::" part is necessary. > Source/WebKit/qt/WidgetApi/qwebpage.h:286 > + VisibilityState visibilityState(); const missing here too.
Created attachment 208173 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (6)
Comment on attachment 208173 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(QWebPage::PageVisibilityState state) (6) View in context: https://bugs.webkit.org/attachment.cgi?id=208173&action=review Looks great! Three small finishing-touches type of nitpicks. But otherwise I love it. Small, compact but straight to the point. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3152 > +/*! > + This method is used to change the page visibility state. You can combine the docs for the setter and the getter into one snippet of documentation for the entire property. See the use of the \property tag in other places in the same file. > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3164 > +void QWebPage::setVisibilityState(QWebPage::VisibilityState state) I think that you can remove the QWebPage:: prefix for the parameter > Source/WebKit/qt/WidgetApi/qwebpage.h:220 > + enum VisibilityState { This new enum needs documentation that explains what the values mean :)
Created attachment 208183 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(VisibilityState state) (7)
Comment on attachment 208183 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(VisibilityState state) (7) Attachment 208183 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1347177
Comment on attachment 208183 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(VisibilityState state) (7) > c++: internal compiler error: Killed (program cc1plus) Seems like the EFL bot is sick, re-cq+ing
Comment on attachment 208183 [details] New QWebPage API (with test): void QWebPage::setVisibilityState(VisibilityState state) (7) Clearing flags on attachment: 208183 Committed r153751: <http://trac.webkit.org/changeset/153751>
All reviewed patches have been landed. Closing bug.
Note, the landed patch broke the minimal build. Seem it requires a minor build fix: /ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp: In member function 'QWebPageAdapter::VisibilityState QWebPageAdapter::visibilityState() const': /ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:330:65: error: 'class WebCore::Page' has no member named 'visibilityState' /ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:331:1: error: control reaches end of non-void function [-Werror=return-type] cc1plus: all warnings being treated as errors