Bug 117333 – Vibration can be canceled even if page visibility status is hidden

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kihong Kwon]

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.

[Kihong Kwon]

Created attachment 204010 [details]
Patch

[Chris Dumez]

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.

[Kihong Kwon]

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

[Chris Dumez]

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

[Kihong Kwon]

Created attachment 204565 [details]
Patch

[Early Warning System Bot]

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

[Early Warning System Bot]

Comment on attachment 204565 [details]
Patch

Attachment 204565 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/848072

[Kihong Kwon]

Created attachment 204571 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Kihong Kwon]

Created attachment 204659 [details]
Patch

[Gyuyoung Kim]

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 ?

[Kihong Kwon]

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

[Kihong Kwon]

Created attachment 204676 [details]
Patch

[Gyuyoung Kim]

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.

[Kihong Kwon]

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

[Kihong Kwon]

Created attachment 204698 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 204698 [details]
Patch

LGTM. But, someone else might wanna have a final look.

[Kihong Kwon]

If there is no more comment, I will land this today.
Thanks Gyuyoung.

[WebKit Commit Bot]

Comment on attachment 204698 [details]
Patch

Clearing flags on attachment: 204698

Committed r151727: <http://trac.webkit.org/changeset/151727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.