Correct confusing error message from OfflineAudioContext.suspend()

Correct confusing error message from OfflineAudioContext.suspend()

This CL is to clamp the frame number and the time in the error message
that is displayed when OfflineAudioContext.suspend() is failed because
of the invalid suspension time.

BUG=678773
TEST=
LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic.html

Review-Url: https://codereview.chromium.org/2666703002
Cr-Commit-Position: refs/heads/master@{#447367}
Committed: https://chromium.googlesource.com/chromium/src/+/48442972273ae0ba3f6f4cadc31dc5988b91e660

[hongchan, 2017-01-30 20:48:57]

hongchan@chromium.org changed reviewers:
+ rtoy@chromium.org

[hongchan, 2017-01-30 20:48:58]

PTAL at the code change.

Not sure where to add the new test for CL. I am looking at:
offlineaudiocontext-suspend-resume-promise.html

But it still uses many stuffs from js-test. WDYT?

[Raymond Toy, 2017-01-30 21:05:08]

On 2017/01/30 20:48:58, hongchan wrote:
> PTAL at the code change.
> 
> Not sure where to add the new test for CL. I am looking at:
> offlineaudiocontext-suspend-resume-promise.html
> 
> But it still uses many stuffs from js-test. WDYT?

Changes look good. I'm opposed to changing
offlineaudiocontext-suspend-resume-promise.html to testharness in this CL. No
problem if you want to change just that test before or after this CL.

[hongchan, 2017-01-30 21:25:26]

I think the PS2 works for us. I have added a task to 
offlineaudiocontext-suspend-resume-basic.html and updated
the expected error message as well.

[Raymond Toy, 2017-01-30 22:13:07]

https://codereview.chromium.org/2666703002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt
(right):

https://codereview.chromium.org/2666703002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt:11:
PASS Scheduling a suspend after the render completion rejected correctly (with
InvalidStateError: cannot schedule a suspend at frame 44032 (1 seconds) because
it is earlier than the current frame of 44100 (1 seconds)).
The message is still confusing because the time in seconds for both items is
exactly the same but the frame number is not.  Not exactly sure how to phrase
this.

[hongchan, 2017-01-30 23:50:54]

> PASS Scheduling a suspend after the render completion rejected correctly (with
> InvalidStateError: cannot schedule a suspend at frame 44032 (1 seconds)
because
> it is earlier than the current frame of 44100 (1 seconds)).
> The message is still confusing because the time in seconds for both items is
> exactly the same but the frame number is not.  Not exactly sure how to phrase
> this.

How about this?

---
cannot schedule a suspend at frame 44032 (0.9984580499 seconds)
because it is earlier than the current frame of 44100 (1 seconds). Note that the

suspended frame and time are quantized by the render quantum size (128 frames).
---

[hongchan, 2017-01-31 20:29:13]

On 2017/01/30 23:50:54, hongchan wrote:
> > PASS Scheduling a suspend after the render completion rejected correctly
(with
> > InvalidStateError: cannot schedule a suspend at frame 44032 (1 seconds)
> because
> > it is earlier than the current frame of 44100 (1 seconds)).
> > The message is still confusing because the time in seconds for both items is
> > exactly the same but the frame number is not.  Not exactly sure how to
phrase
> > this.
> 
> How about this?
> 
> ---
> cannot schedule a suspend at frame 44032 (0.9984580499 seconds)
> because it is earlier than the current frame of 44100 (1 seconds). Note that
the
> 
> suspended frame and time are quantized by the render quantum size (128
frames).
> ---

This was suggested by rtoy@ via chat:

"suspend(1) failed to suspend at frame 44032 because it is earlier than the
current frame of 44100 (1 sec)."

[Raymond Toy, 2017-01-31 20:43:43]

lgtm, but please update TEST=TBD

One minor nit that does not block this CL in any way.

https://codereview.chromium.org/2666703002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt
(right):

https://codereview.chromium.org/2666703002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt:9:
PASS Scheduling a suspend in the past rejected correctly (with
InvalidStateError: suspend(0.399229) failed to suspend at frame 17536 because it
is earlier than the current frame of 22016 (0.499229 seconds)).
Although not caused by this CL, but can you explain to me why the time is
0.399229?  From a quick check, it seems it should be 0.4?

[hongchan, 2017-01-31 20:54:58]

Description was changed from

==========
Correct confusing error message from OfflineAudioContext.suspend()

This CL is to clamp the frame number and the time in the error message
that is displayed when OfflineAudioContext.suspend() is failed because
of the invalid suspension time.

BUG=678773
TEST=TBD
==========

to

==========
Correct confusing error message from OfflineAudioContext.suspend()

This CL is to clamp the frame number and the time in the error message
that is displayed when OfflineAudioContext.suspend() is failed because
of the invalid suspension time.

BUG=678773
TEST=
LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic.html
==========

[hongchan, 2017-01-31 21:02:49]

On 2017/01/31 20:43:43, Raymond Toy wrote:
> lgtm, but please update TEST=TBD
> 
> One minor nit that does not block this CL in any way.
> 
>
https://codereview.chromium.org/2666703002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2666703002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt:9:
> PASS Scheduling a suspend in the past rejected correctly (with
> InvalidStateError: suspend(0.399229) failed to suspend at frame 17536 because
it
> is earlier than the current frame of 22016 (0.499229 seconds)).
> Although not caused by this CL, but can you explain to me why the time is
> 0.399229?  From a quick check, it seems it should be 0.4?

That's because currentTime is quantized (=0.499...) when the suspension happens.

[hongchan, 2017-01-31 21:03:00]

The CQ bit was checked by hongchan@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:03:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raymond Toy, 2017-01-31 21:18:05]

https://codereview.chromium.org/2666703002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt
(right):

https://codereview.chromium.org/2666703002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic-expected.txt:9:
PASS Scheduling a suspend in the past rejected correctly (with
InvalidStateError: suspend(0.399229) failed to suspend at frame 17536 because it
is earlier than the current frame of 22016 (0.499229 seconds)).
On 2017/01/31 20:43:43, Raymond Toy wrote:
> Although not caused by this CL, but can you explain to me why the time is
> 0.399229?  From a quick check, it seems it should be 0.4?

Oh, right!  The test is suspending at currentTime - 0.1.

[commit-bot: I haz the power, 2017-01-31 23:44:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485896580739590,
"parent_rev": "ce8370ad0292d36c291a5bbd50bc0577442e8346", "commit_rev":
"2ff0abc6b4c97d910a51127b2a59f6b0578c1ca6"}

[commit-bot: I haz the power, 2017-01-31 23:48:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485896580739590,
"parent_rev": "077a9f76b32bbf8f51d196188bc8b612422f7688", "commit_rev":
"48442972273ae0ba3f6f4cadc31dc5988b91e660"}

[commit-bot: I haz the power, 2017-01-31 23:48:45]

Description was changed from

==========
Correct confusing error message from OfflineAudioContext.suspend()

This CL is to clamp the frame number and the time in the error message
that is displayed when OfflineAudioContext.suspend() is failed because
of the invalid suspension time.

BUG=678773
TEST=
LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic.html
==========

to

==========
Correct confusing error message from OfflineAudioContext.suspend()

This CL is to clamp the frame number and the time in the error message
that is displayed when OfflineAudioContext.suspend() is failed because
of the invalid suspension time.

BUG=678773
TEST=
LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-suspend-resume-basic.html

Review-Url: https://codereview.chromium.org/2666703002
Cr-Commit-Position: refs/heads/master@{#447367}
Committed:
https://chromium.googlesource.com/chromium/src/+/48442972273ae0ba3f6f4cadc31d...
==========

[commit-bot: I haz the power, 2017-01-31 23:48:46]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/48442972273ae0ba3f6f4cadc31d...