Initialize AudioContext on constructor to increase currentTime in real-time.

Initialize AudioContext on constructor to increase currentTime in real-time.

The AudioContext is not initialized to prevent unefficiant running of audio device
before any node is created in the context. However, it is mismatched with the specification.
According to the specification, currentTime must be increased when currentTime of context.

"This is a time in seconds which starts at zero when the context is created and increases in real-time."

TEST=manually tested that currentTime progresses by creating  an AudioNode a short time after creating the context and verified that currentTime was not zero.

BUG=377683
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175026

[KhNo, 2014-05-27 10:58:39]

Could you review this patch?

currentTime
"This is a time in seconds which starts at zero when the context is created and
increases in real-time."

Some tricky way for lazy initialization to prevent inefficiant  running of audio
device makes spec mismatching.

[Raymond Toy, 2014-05-27 17:49:45]

Looks good overall. Just a few questions.

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (left):

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:124: m_destinationNode =
DefaultAudioDestinationNode::create(this);
Why are these two lines swapped?

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:190:
m_destinationNode->startRendering();
Don't we still need to call startRendering()?

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:146: constructCommon();
Why is this moved from the beginning to the end?

[Raymond Toy, 2014-05-27 21:04:31]

Oh, please update the title. This CL is really about initializing currentTime to
0 when the context is created instead of when the first node is created.

Also, I assume that with the CL, if I wait, say, 1 sec after creating the
context and then create an audio node, currentTime will be 1 sec, not 0.

Can we add a test for that then?

[KhNo, 2014-05-28 14:39:51]

PTAL, I have tried creating a layout test for this patch. ( please check patch
set #2 )
In my guessing, worker or content shell is not supported rendering actual audio
device.

So, if I tested it that I created with chromium local build, all passed.
But, in content shell, currentTime was not increased since currentTime always
was 0.

Unfortunately, it was not able to add to test case.
Is there any suggestion?

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (left):

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:124: m_destinationNode =
DefaultAudioDestinationNode::create(this);
On 2014/05/27 17:49:45, Raymond Toy wrote:
> Why are these two lines swapped?

It must be initialized after creating m_destinationNode. In initializae().
m_destinationNode is used.

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:190:
m_destinationNode->startRendering();
On 2014/05/27 17:49:45, Raymond Toy wrote:
> Don't we still need to call startRendering()?

This is my big mistake. Sorry about it. It must be there.

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:146: constructCommon();
On 2014/05/27 17:49:45, Raymond Toy wrote:
> Why is this moved from the beginning to the end?

It must be initialized after creating m_destinationNode. In initializae().
m_destinationNode is used.

[Raymond Toy, 2014-05-28 16:39:23]

On 2014/05/28 14:39:51, KhNo wrote:
> PTAL, I have tried creating a layout test for this patch. ( please check patch
> set #2 )
> In my guessing, worker or content shell is not supported rendering actual
audio
> device.

Ok. We should, someday, make content shell be able to run webaudio.
> 
> So, if I tested it that I created with chromium local build, all passed.
> But, in content shell, currentTime was not increased since currentTime always
> was 0.
> 
> Unfortunately, it was not able to add to test case.
> Is there any suggestion?

Yeah, that's unfortunate. I have no ideas on a test, so I guess this is ok as
long as you manually tested that currentTime does progress.

You may want to add a line to the description like

TEST=manually tested that currentTime progresses by creating an AudioNode a
short time after creating the context and verified that currentTime was not
zero.

Or however you tested this.
> 
>
https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
> File Source/modules/webaudio/AudioContext.cpp (left):
> 
>
https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
> Source/modules/webaudio/AudioContext.cpp:124: m_destinationNode =
> DefaultAudioDestinationNode::create(this);
> On 2014/05/27 17:49:45, Raymond Toy wrote:
> > Why are these two lines swapped?
> 
> It must be initialized after creating m_destinationNode. In initializae().
> m_destinationNode is used.
> 
>
https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
> Source/modules/webaudio/AudioContext.cpp:190:
> m_destinationNode->startRendering();
> On 2014/05/27 17:49:45, Raymond Toy wrote:
> > Don't we still need to call startRendering()?
> 
> This is my big mistake. Sorry about it. It must be there.
> 
>
https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
> File Source/modules/webaudio/AudioContext.cpp (right):
> 
>
https://codereview.chromium.org/297313003/diff/1/Source/modules/webaudio/Audi...
> Source/modules/webaudio/AudioContext.cpp:146: constructCommon();
> On 2014/05/27 17:49:45, Raymond Toy wrote:
> > Why is this moved from the beginning to the end?
> 
> It must be initialized after creating m_destinationNode. In initializae().
> m_destinationNode is used.

[Raymond Toy, 2014-05-28 16:43:08]

One nit:  Maybe it is worth renaming constructCommon to initialize and move
everything into that one function.

But it's ok if you want to leave things as they are.

[Raymond Toy, 2014-05-28 16:43:16]

lgtm

[KhNo, 2014-05-29 01:55:58]

On 2014/05/28 16:43:08, Raymond Toy wrote:
> One nit:  Maybe it is worth renaming constructCommon to initialize and move
> everything into that one function.
> 
> But it's ok if you want to leave things as they are.

That was my small concern, I would like to remove it and make initalize as your
suggestion.
I will this job on the next patch with removing m_isAudioThreadFinished
togather.
I believe that has more closed context.

[KhNo, 2014-05-29 01:57:39]

On 2014/05/28 16:43:16, Raymond Toy wrote:
> lgtm

Thanks for reviewing.

[KhNo, 2014-05-29 01:57:49]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-05-29 01:58:10]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/297313003/100001

[commit-bot: I haz the power, 2014-05-29 03:12:09]

Change committed as 175026