cancelVibration can be called in the visibilitychange listener even if page visibility is changed to hidden. Therefore cancelVibration have to work when page visibility is hidden.
Created attachment 204010 [details] Patch
Comment on attachment 204010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204010&action=review > Source/WebCore/ChangeLog:11 > + No new tests, covered by existing tests. It is not covered by existing tests, otherwise tests would be unskipped / rebaselined with this fix. I believe you need a layout test.
(In reply to comment #2) > (From update of attachment 204010 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204010&action=review > > > Source/WebCore/ChangeLog:11 > > + No new tests, covered by existing tests. > > It is not covered by existing tests, otherwise tests would be unskipped / rebaselined with this fix. I believe you need a layout test. IMHO, we couldn't make more test cases for vibration api, because is is only for signal to the device and no response or return value for checking.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 204010 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=204010&action=review > > > > > Source/WebCore/ChangeLog:11 > > > + No new tests, covered by existing tests. > > > > It is not covered by existing tests, otherwise tests would be unskipped / rebaselined with this fix. I believe you need a layout test. > > IMHO, we couldn't make more test cases for vibration api, because is is only for signal to the device and no response or return value for checking. Actually, I believe this is currently a signal to the browser. What prevents you from extending DRT / WKTR and dump information when cancelVibration is called?
Created attachment 204565 [details] Patch
Comment on attachment 204565 [details] Patch Attachment 204565 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/859059
Comment on attachment 204565 [details] Patch Attachment 204565 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/848072
Created attachment 204571 [details] Patch
Comment on attachment 204571 [details] Patch Attachment 204571 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/850143 New failing tests: vibration/navigator-vibration.html fast/repaint/table-cell-collapsed-border-scroll.html vibration/cancelVibration-after-pagevisibility-changed-to-hidden.html
Created attachment 204586 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Comment on attachment 204571 [details] Patch Attachment 204571 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/893020 New failing tests: vibration/navigator-vibration.html vibration/cancelVibration-after-pagevisibility-changed-to-hidden.html
Created attachment 204635 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Created attachment 204659 [details] Patch
Comment on attachment 204659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204659&action=review > Source/WebCore/ChangeLog:9 > + Therefore cancelVibration need to work when page visibility is hidden. need -> needs ? > Source/WebCore/Modules/vibration/NavigatorVibration.cpp:48 > + if (navigator->frame()->page()->visibilityState() == PageVisibilityStateHidden && time) Can't we use Vibration::isActive() instead of time in order to check if vibration is live ? > Source/WebCore/testing/Internals.cpp:2169 > +bool Internals::isVibrating() Don't you need to use ExceptionCode ?
(In reply to comment #14) > (From update of attachment 204659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204659&action=review > > > Source/WebCore/Modules/vibration/NavigatorVibration.cpp:48 > > + if (navigator->frame()->page()->visibilityState() == PageVisibilityStateHidden && time) > > Can't we use Vibration::isActive() instead of time in order to check if vibration is live ? If time is 0, vibration api works to cancel vibration. It is different from isActive status. > > > Source/WebCore/testing/Internals.cpp:2169 > > +bool Internals::isVibrating() > > Don't you need to use ExceptionCode ? I don't think I need ExceptionCode here. Important thing of isVibrating is vibrating or not currently. Therefore "return false" which is in the if-statement is enough to check that.
Created attachment 204676 [details] Patch
Comment on attachment 204676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204676&action=review > Source/WebCore/testing/Internals.cpp:2175 > + return Vibration::from(document->domWindow()->navigator()->frame()->page())->isVibrating(); You misses to check if page() is null or not. BTW, should you get a page instance by navigator() ? IMHO, navigator has been used in NavigatorXXX mainly.
(In reply to comment #17) > (From update of attachment 204676 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204676&action=review > > > Source/WebCore/testing/Internals.cpp:2175 > > + return Vibration::from(document->domWindow()->navigator()->frame()->page())->isVibrating(); > > You misses to check if page() is null or not. BTW, should you get a page instance by navigator() ? IMHO, navigator has been used in NavigatorXXX mainly. Vibration instance is attached to Supplement<Page>, therefore we can get an instance of Vibration by document.page(); In this case, I think document->page() is enough to get a page instance. In addition, navigator doesn't have page pointer, that's why I used "navigator()->frame()->page()" I will change from "document->domWindow()->navigator()->frame()->page()" to "document->page()" And I will check if page is null or not using ASSERT.
Created attachment 204698 [details] Patch
Comment on attachment 204698 [details] Patch LGTM. But, someone else might wanna have a final look.
If there is no more comment, I will land this today. Thanks Gyuyoung.
Comment on attachment 204698 [details] Patch Clearing flags on attachment: 204698 Committed r151727: <http://trac.webkit.org/changeset/151727>
All reviewed patches have been landed. Closing bug.