Bug 79701 – Add playback state for AudioBufferSourceNode and add number of active nodes

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

Add playback state for AudioBufferSourceNode and add number of active nodes

[Raymond Toy]

Created attachment 129098 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129098 [details]
Patch

Attachment 129098 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11641188

New failing tests:
css3/filters/effect-invert-hw.html

[Chris Rogers]

Comment on attachment 129098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129098&action=review

The test is starting to look good.  A few comments below:

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:54
> +unsigned int AudioBufferSourceNode::m_activeSourceCount = 0;

"unsigned int" -> "unsigned"

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:133
> +            ++AudioBufferSourceNode::m_activeSourceCount;

no need to use namespace "AudioBufferSourceNode::" here

> Source/WebCore/webaudio/AudioBufferSourceNode.h:46
> +    // These must be defined as in the .idl file.

I think it would be worth a comment describing the valid state-transitions.  I know they're simple, but best to spell this out.

> Source/WebCore/webaudio/AudioBufferSourceNode.h:47
> +    enum playbackState_t {

WebKit style: please don't use "UNIX hacker" style for this type -- should be PlaybackState

> Source/WebCore/webaudio/AudioBufferSourceNode.h:49
> +        INITIAL = 0,

WebKit style: enum values should use camel-casing, for example:

InitialState
ScheduledState
PlayingState
FinishedState

Also, InitialState would be better as UnscheduledState

> Source/WebCore/webaudio/AudioBufferSourceNode.h:52
> +        // SCHEDULED state, silence is actually being played.)

I would remove the "Note" because this is a detail which can be optimized -- let's try to keep the description of the states minimal

(Note that in the SCHEDULED state, silence is actually being played.)

> Source/WebCore/webaudio/AudioBufferSourceNode.h:55
> +        // Currently playing.

Comment does not add much value.  Maybe mention that scheduled time <= context time

> Source/WebCore/webaudio/AudioBufferSourceNode.h:58
> +        // Finished playing.

Comment does not add value.  Please describe what "finished" means.

> Source/WebCore/webaudio/AudioBufferSourceNode.h:88
> +    // Get and reset the number of sources that are currently playing.

I would split this comment into two comments, one for each method

> Source/WebCore/webaudio/AudioBufferSourceNode.h:89
> +    static unsigned int activeSourceCount() { return AudioBufferSourceNode::m_activeSourceCount; }

"unsigned int" -> "unsigned"

> Source/WebCore/webaudio/AudioBufferSourceNode.h:160
> +    enum playbackState_t m_playbackState;

"enum playbackState_t" -> PlaybackState

> Source/WebCore/webaudio/AudioBufferSourceNode.h:163
> +    static unsigned int m_activeSourceCount;

"unsigned int" -> "unsigned"

Also, use the "s_" prefix for static members instead of "m_"

Actually, come to think of it, this can't be a static member variable of AudioBufferSourceNode --- what if there are multiple contexts??
Instead, this will have to be a member variable of the AudioContext

> Source/WebCore/webaudio/AudioBufferSourceNode.idl:34
> +        const unsigned short INITIAL = 0;

INITIAL -> UNSCHEDULED

Please add "_STATE" to all the constants -- for example, UNSCHEDULED_STATE

> LayoutTests/webaudio/audiobuffersource-activeSourceCount.html:37
> +function playbackStateString(state)

This function seems to be unused please remove to make this test shorter.

> LayoutTests/webaudio/audiobuffersource-activeSourceCount.html:100
> +    var suffixMessage = " after " + renderTime + " sec.";

could you please spell out the complete word "seconds" or "second"  here and elsewhere in the test

> LayoutTests/webaudio/audiobuffersource-activeSourceCount.html:159
> +    // Test PLAYING state for looping source. Create .5 sec source starting at time 1.25 and looping

nit: you use 0.5 below, but .5 here

[Raymond Toy]

Comment on attachment 129098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129098&action=review

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:163
>> +    static unsigned int m_activeSourceCount;
> 
> "unsigned int" -> "unsigned"
> 
> Also, use the "s_" prefix for static members instead of "m_"
> 
> Actually, come to think of it, this can't be a static member variable of AudioBufferSourceNode --- what if there are multiple contexts??
> Instead, this will have to be a member variable of the AudioContext

After implementing this, I was wondering if it would be better to have it in the AudioContext, because it is an audio context item, not an audio buffer source node item.

With multiple contexts, it's clear that it should be in the context.

[Raymond Toy]

Comment on attachment 129098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129098&action=review

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:49
>> +        INITIAL = 0,
> 
> WebKit style: enum values should use camel-casing, for example:
> 
> InitialState
> ScheduledState
> PlayingState
> FinishedState
> 
> Also, InitialState would be better as UnscheduledState

But don't these names need to match the names used in the idl?  Or are you also proposing that the idl use camel case names. (This doesn't match the existing idl naming in other nodes.)

[Raymond Toy]

Created attachment 129472 [details]
Patch

[Raymond Toy]

Comment on attachment 129098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129098&action=review

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:54
>> +unsigned int AudioBufferSourceNode::m_activeSourceCount = 0;
> 
> "unsigned int" -> "unsigned"

Changed to int so we can prevent count from wrapping around from 0 to a very large number.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:133
>> +            ++AudioBufferSourceNode::m_activeSourceCount;
> 
> no need to use namespace "AudioBufferSourceNode::" here

Changed because source count is now in the AudioContext.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:46
>> +    // These must be defined as in the .idl file.
> 
> I think it would be worth a comment describing the valid state-transitions.  I know they're simple, but best to spell this out.

Comments added.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:47
>> +    enum playbackState_t {
> 
> WebKit style: please don't use "UNIX hacker" style for this type -- should be PlaybackState

Done.

>>> Source/WebCore/webaudio/AudioBufferSourceNode.h:49
>>> +        INITIAL = 0,
>> 
>> WebKit style: enum values should use camel-casing, for example:
>> 
>> InitialState
>> ScheduledState
>> PlayingState
>> FinishedState
>> 
>> Also, InitialState would be better as UnscheduledState
> 
> But don't these names need to match the names used in the idl?  Or are you also proposing that the idl use camel case names. (This doesn't match the existing idl naming in other nodes.)

Changed to match IDL names.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:52
>> +        // SCHEDULED state, silence is actually being played.)
> 
> I would remove the "Note" because this is a detail which can be optimized -- let's try to keep the description of the states minimal
> 
> (Note that in the SCHEDULED state, silence is actually being played.)

Removed.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:55
>> +        // Currently playing.
> 
> Comment does not add much value.  Maybe mention that scheduled time <= context time

Removed.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:58
>> +        // Finished playing.
> 
> Comment does not add value.  Please describe what "finished" means.

Removed.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:88
>> +    // Get and reset the number of sources that are currently playing.
> 
> I would split this comment into two comments, one for each method

Removed because the count is in context.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:89
>> +    static unsigned int activeSourceCount() { return AudioBufferSourceNode::m_activeSourceCount; }
> 
> "unsigned int" -> "unsigned"

Removed because the count is in the context.

>>> Source/WebCore/webaudio/AudioBufferSourceNode.h:163
>>> +    static unsigned int m_activeSourceCount;
>> 
>> "unsigned int" -> "unsigned"
>> 
>> Also, use the "s_" prefix for static members instead of "m_"
>> 
>> Actually, come to think of it, this can't be a static member variable of AudioBufferSourceNode --- what if there are multiple contexts??
>> Instead, this will have to be a member variable of the AudioContext
> 
> After implementing this, I was wondering if it would be better to have it in the AudioContext, because it is an audio context item, not an audio buffer source node item.
> 
> With multiple contexts, it's clear that it should be in the context.

Moved from here to AudioContext.

>> Source/WebCore/webaudio/AudioBufferSourceNode.idl:34
>> +        const unsigned short INITIAL = 0;
> 
> INITIAL -> UNSCHEDULED
> 
> Please add "_STATE" to all the constants -- for example, UNSCHEDULED_STATE

Done.

>> LayoutTests/webaudio/audiobuffersource-activeSourceCount.html:37
>> +function playbackStateString(state)
> 
> This function seems to be unused please remove to make this test shorter.

Removed.

>> LayoutTests/webaudio/audiobuffersource-activeSourceCount.html:100
>> +    var suffixMessage = " after " + renderTime + " sec.";
> 
> could you please spell out the complete word "seconds" or "second"  here and elsewhere in the test

Done.

>> LayoutTests/webaudio/audiobuffersource-activeSourceCount.html:159
>> +    // Test PLAYING state for looping source. Create .5 sec source starting at time 1.25 and looping
> 
> nit: you use 0.5 below, but .5 here

Done.

[Raymond Toy]

Created attachment 130169 [details]
Patch

[Raymond Toy]

(In reply to comment #8)
> Created an attachment (id=130169) [details]
> Patch

Small update to use atomic increments and decrements of the active source count.

[Chris Rogers]

Comment on attachment 130169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130169&action=review

Looks good overall:

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:117
> +            || (m_playbackState == FINISHED_STATE)

I think the logic in 116:117 can be made more clear if you simply check for (m_playbackState == UNSCHEDULED_STATE || m_playbackState == FINISHED_STATE)

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:128
> +            // SCHEDULED_STATE state to the PLAYING_STATE state.

"the SCHEDULED_STATE state to the PLAYING_STATE state" -> "SCHEDULED_STATE to PLAYING_STATE"

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:356
> +        // Finished playing, so decrement the active source count.

WebKit style is to avoid redundant comments.

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:402
> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)

shouldn't this be:

if (m_playbackState != UNSCHEDULED_STATE)

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:415
> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)

same comment as for noteOn() above

> Source/WebCore/webaudio/AudioBufferSourceNode.h:49
> +    // UNSCHEDULED_STATE - Initial playback state. Node created, but not yet scheduled.

"Node created" -> "Created"

> Source/WebCore/webaudio/AudioBufferSourceNode.h:51
> +    // PLAYING_STATE - The buffer source is currently playing

"The buffer source is currently playing" -> "Generating sound."

> Source/WebCore/webaudio/AudioBufferSourceNode.h:52
> +    // FINISHED_STATE - The buffer source has finished playing

"The buffer source has finished playing" -> "Finished generating sound."

> Source/WebCore/webaudio/AudioBufferSourceNode.h:87
> +    // Playback state

WebKit style is to avoid redundant comments such as this

> Source/WebCore/webaudio/AudioBufferSourceNode.h:162
> +    // Playback state.  

WebKit style is to avoid redundant comments such as this

> Source/WebCore/webaudio/AudioBufferSourceNode.h:163
> +    enum PlaybackState m_playbackState;

the "enum" keyword is not needed here

> Source/WebCore/webaudio/AudioContext.h:99
> +    unsigned int activeSourceCount() { return static_cast<unsigned>(m_activeSourceCount); }

"unsigned int" -> "unsigned long"

> Source/WebCore/webaudio/AudioContext.idl:50
> +        readonly attribute unsigned int activeSourceCount;

"unsigned int" -> "unsigned long"

> LayoutTests/webaudio/audiobuffersource-playbackState.html:98
> +    // Test unscheduled state. Create 3 second source, but don't schedule it.

These tests look great, but it looks like there's an awful lot of boilerplate copy/paste here.  I think it would be better to create a function which takes:
* impulse buffer duration
* noteOn time -- if any
* expected state

I think it's probably overkill to hand-code/construct separate PASS/FAIL messages.  It's obvious to see if the test passed or failed (since it outputs PASS or FAIL already) and
we should be able to construct a generic message that makes sense for either case in the function I'm proposing above.

If a function is used as I propose then I think this code becomes a lot easier to follow.

[Raymond Toy]

Comment on attachment 130169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130169&action=review

Just a few questions; other suggested changes in progress....

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:402
>> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)
> 
> shouldn't this be:
> 
> if (m_playbackState != UNSCHEDULED_STATE)

What if the state is FINISHED_STATE?  What if we call noteOn, wait some time until after the source has played out and call noteOn again?  Should we play the note again?

I was trying to follow the original logic as closely as possible where m_isPlaying is equivalent to SCHEDULED_STATE or PLAYING_STATE.

>> Source/WebCore/webaudio/AudioContext.h:99
>> +    unsigned int activeSourceCount() { return static_cast<unsigned>(m_activeSourceCount); }
> 
> "unsigned int" -> "unsigned long"

Aren't 4 billion active sources enough?

[Chris Rogers]

Comment on attachment 130169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130169&action=review

>>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:402
>>> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)
>> 
>> shouldn't this be:
>> 
>> if (m_playbackState != UNSCHEDULED_STATE)
> 
> What if the state is FINISHED_STATE?  What if we call noteOn, wait some time until after the source has played out and call noteOn again?  Should we play the note again?
> 
> I was trying to follow the original logic as closely as possible where m_isPlaying is equivalent to SCHEDULED_STATE or PLAYING_STATE.

Good point, probably best to keep your version then.

>>> Source/WebCore/webaudio/AudioContext.h:99
>>> +    unsigned int activeSourceCount() { return static_cast<unsigned>(m_activeSourceCount); }
>> 
>> "unsigned int" -> "unsigned long"
> 
> Aren't 4 billion active sources enough?

I was just trying to keep this exactly consistent with the .idl file which is "unsigned long"

[Raymond Toy]

Created attachment 132384 [details]
Patch

[Raymond Toy]

Comment on attachment 130169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130169&action=review

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:117
>> +            || (m_playbackState == FINISHED_STATE)
> 
> I think the logic in 116:117 can be made more clear if you simply check for (m_playbackState == UNSCHEDULED_STATE || m_playbackState == FINISHED_STATE)

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:128
>> +            // SCHEDULED_STATE state to the PLAYING_STATE state.
> 
> "the SCHEDULED_STATE state to the PLAYING_STATE state" -> "SCHEDULED_STATE to PLAYING_STATE"

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:356
>> +        // Finished playing, so decrement the active source count.
> 
> WebKit style is to avoid redundant comments.

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:49
>> +    // UNSCHEDULED_STATE - Initial playback state. Node created, but not yet scheduled.
> 
> "Node created" -> "Created"

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:51
>> +    // PLAYING_STATE - The buffer source is currently playing
> 
> "The buffer source is currently playing" -> "Generating sound."

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:52
>> +    // FINISHED_STATE - The buffer source has finished playing
> 
> "The buffer source has finished playing" -> "Finished generating sound."

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:87
>> +    // Playback state
> 
> WebKit style is to avoid redundant comments such as this

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:162
>> +    // Playback state.  
> 
> WebKit style is to avoid redundant comments such as this

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:163
>> +    enum PlaybackState m_playbackState;
> 
> the "enum" keyword is not needed here

Done.

>>>> Source/WebCore/webaudio/AudioContext.h:99
>>>> +    unsigned int activeSourceCount() { return static_cast<unsigned>(m_activeSourceCount); }
>>> 
>>> "unsigned int" -> "unsigned long"
>> 
>> Aren't 4 billion active sources enough?
> 
> I was just trying to keep this exactly consistent with the .idl file which is "unsigned long"

Done.

>> Source/WebCore/webaudio/AudioContext.idl:50
>> +        readonly attribute unsigned int activeSourceCount;
> 
> "unsigned int" -> "unsigned long"

Done.

>> LayoutTests/webaudio/audiobuffersource-playbackState.html:98
>> +    // Test unscheduled state. Create 3 second source, but don't schedule it.
> 
> These tests look great, but it looks like there's an awful lot of boilerplate copy/paste here.  I think it would be better to create a function which takes:
> * impulse buffer duration
> * noteOn time -- if any
> * expected state
> 
> I think it's probably overkill to hand-code/construct separate PASS/FAIL messages.  It's obvious to see if the test passed or failed (since it outputs PASS or FAIL already) and
> we should be able to construct a generic message that makes sense for either case in the function I'm proposing above.
> 
> If a function is used as I propose then I think this code becomes a lot easier to follow.

Common code factored out, and messages simplified.

[Chris Rogers]

Comment on attachment 132384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132384&action=review

Looks pretty good overall - a few more comments:

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:126
> +            // PLAYING_STATE.

unnecessary line wrapping.

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:399
> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)

Sorry, but I misunderstood your response before, so I stick with my original comment (because it is *not* allowed to issue multiple noteOn() calls - for example after a note has finished playing the first time -- a 2nd time is not allowed)
In fact this is explicitly stated in the header file:
// The state can only transition to the next state, except for the FINISHED_STATE which can
// never be changed.

shouldn't this be:
if (m_playbackState != UNSCHEDULED_STATE)

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:412
> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)

Sorry, but I misunderstood your response before, so I stick with my original comment (because it is *not* allowed to issue multiple noteOn() calls - for example after a note has finished playing the first time -- a 2nd time is not allowed)

shouldn't this be:
if (m_playbackState != UNSCHEDULED_STATE)

> Source/WebCore/webaudio/AudioBufferSourceNode.h:50
> +    // SCHEDULED_STATE - Scheduled to play (via noteOn or noteGrainOn), but not yet playing.

noteOn -> noteOn()
noteGrainOn -> noteGrainOn()

> LayoutTests/webaudio/audiobuffersource-playbackState-expected.txt:1
> +Test AudioContext activeStateCount and AudioBufferSourceNode playbackState.

activeStateCount -> activeSourceCount

> LayoutTests/webaudio/audiobuffersource-playbackState.html:13
> +description("Test AudioContext activeStateCount and AudioBufferSourceNode playbackState.");

activeStateCount -> activeSourceCount

> LayoutTests/webaudio/audiobuffersource-playbackState.html:46
> +    // For each source, verify that the playback state matches our expected play back state.

"play back" -> "playback"

> LayoutTests/webaudio/audiobuffersource-playbackState.html:84
> +//

Lines 83:84 can be removed -- in any case the function is called createTest()

> LayoutTests/webaudio/audiobuffersource-playbackState.html:163
> +    // second.  This will finish before rendering is complete.

This comment looks wrong

> LayoutTests/webaudio/audiobuffersource-playbackState.html:181
> +    // looping afterwards.  This will still be playing when rendering is complete.

This comment looks wrong -- I'm inclined to just remove all of these comments since it's easier to just look at the arguments passed to createTest() to understand what's happening.
The comments add clutter/redundancy and are easy to get wrong (causing confusion)

[Raymond Toy]

Created attachment 132397 [details]
Patch

[Raymond Toy]

Comment on attachment 132384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132384&action=review

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:126
>> +            // PLAYING_STATE.
> 
> unnecessary line wrapping.

Fixed.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:399
>> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)
> 
> Sorry, but I misunderstood your response before, so I stick with my original comment (because it is *not* allowed to issue multiple noteOn() calls - for example after a note has finished playing the first time -- a 2nd time is not allowed)
> In fact this is explicitly stated in the header file:
> // The state can only transition to the next state, except for the FINISHED_STATE which can
> // never be changed.
> 
> shouldn't this be:
> if (m_playbackState != UNSCHEDULED_STATE)

Fixed.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:412
>> +    if (m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)
> 
> Sorry, but I misunderstood your response before, so I stick with my original comment (because it is *not* allowed to issue multiple noteOn() calls - for example after a note has finished playing the first time -- a 2nd time is not allowed)
> 
> shouldn't this be:
> if (m_playbackState != UNSCHEDULED_STATE)

Fixed.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:50
>> +    // SCHEDULED_STATE - Scheduled to play (via noteOn or noteGrainOn), but not yet playing.
> 
> noteOn -> noteOn()
> noteGrainOn -> noteGrainOn()

Done.

>> LayoutTests/webaudio/audiobuffersource-playbackState.html:13
>> +description("Test AudioContext activeStateCount and AudioBufferSourceNode playbackState.");
> 
> activeStateCount -> activeSourceCount

Done.  Expected results updated accordingly.

>> LayoutTests/webaudio/audiobuffersource-playbackState.html:46
>> +    // For each source, verify that the playback state matches our expected play back state.
> 
> "play back" -> "playback"

Done.

>> LayoutTests/webaudio/audiobuffersource-playbackState.html:84
>> +//
> 
> Lines 83:84 can be removed -- in any case the function is called createTest()

Done.

>> LayoutTests/webaudio/audiobuffersource-playbackState.html:181
>> +    // looping afterwards.  This will still be playing when rendering is complete.
> 
> This comment looks wrong -- I'm inclined to just remove all of these comments since it's easier to just look at the arguments passed to createTest() to understand what's happening.
> The comments add clutter/redundancy and are easy to get wrong (causing confusion)

Comments removed here and elsewhere.

[Chris Rogers]

Comment on attachment 132397 [details]
Patch

Thanks Raymond, looks good.

[Raymond Toy]

(In reply to comment #18)
> (From update of attachment 132397 [details])
> Thanks Raymond, looks good.

Thank you!

[WebKit Review Bot]

Comment on attachment 132397 [details]
Patch

Clearing flags on attachment: 132397

Committed r111239: <http://trac.webkit.org/changeset/111239>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Comment on attachment 130169 [details]
Patch

Cleared review? from obsolete attachment 130169 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).