[chromium] Implement frame rate control portions of CCScheduler

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Implement frame rate control portions of CCScheduler

[Nat Duca]

Created attachment 112161 [details]
Patch

[Nat Duca]

I expect that this patch is going to explode on the EWS bots because it needs CCThread::postDelayedTask: b70712.

The goal of this patch is to establish the right infrastructure for the "time" bits of the scheduler, but not implement the exact ideal algorithm. There are two parts:

* CCSmoothedTimer: ticks at an average rate given that the underlying delayed task mechanism ticks only in 1ms increments.
* CCFrameRateController: built on top of the smoothed timer, the FRC tracks pending frames and converts ticks into "draw calls" (so long as < 2 frames are in flight).

In the future:
* We can put fancier logic for minimizing clock drift into the smoothed timer. Now, I use the 16, 16, 17 technique because my quick attempts to do better ran into difficulties.

* Slowdown logic can be added to FrameRateController, e.g. "we're consistently drawing at 30Hz, so stop trying to draw at 60Hz."

[WebKit Review Bot]

Comment on attachment 112161 [details]
Patch

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

[James Robinson]

Comment on attachment 112161 [details]
Patch

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

First round of comments, mostly nitpicks.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:37
> +    // Dont forward the tick if we are throttled.

Dont -> Don't

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:38
> +    if (m_pendingFrameBeginTimes.size() >= 2)

This seems to be holding us to having 2 frames pending at any given point in time.  It's a bit subtle IMO that this logic is here, also this is a bit of a magic number.  Can you turn '2' into a well-named constant?

If we only support a fixed number of pending frames using a Deque<> seems like massive overkill. Why not just use a double[]?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:53
> +    /* FIXME: do something with the the delta between now and frameBeginTime.

we don't typically use c-style comments

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:36
> +class CCFrameRateControllerClient {

protected d'tor here please, so people don't try to delete through a CCFrameRateControllerClient*

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:41
> +class CCFrameRateController : public CCTimerClient {

this has virtual functions but no virtual d'tor - i sense danger

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:55
> +    /* Use the following methods to adjust target frame rate.

another c-style comment

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:66
> +    // CCTimerClient implementation. Do not call directly.

if it shouldn't be called directly does it need to be public?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:67
> +    void onTimerTick();

virtual keyword, please

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:34
> +class CCTimerClient {

please declare a protected virtual empty d'tor - right now if someone tries to delete a CCTimerClient through a CCTimerClient* they will leak

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:39
> +/*

we don't normally use c-style comments

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:48
> +    CCSmoothedTimer(int goalRate, CCThread* thread)

'goalRate' isn't super clear. What's the unit of 'rate'?  Is this target FPS? Target delay between frames in millis?

Is 'int' precise enough? If this is target FPS some displays might have non-integral framerates

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:58
> +    void getGoalRate(int goalRate) { m_goalRate = goalRate; }

this looks like a setter, not a getter

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:62
> +    virtual double getCurrentTime() const { return monotonicallyIncreasingTime(); }

we don't use 'get' prefix for getters

> Source/WebKit/chromium/tests/CCFrameRateControllerTest.cpp:44
> +    void onVSyncTick() { m_tickCalled = true; }

virtual

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:42
> +    void onTimerTick() { m_tickCalled = true; }

virtual keyword please

[Adrienne Walker]

Comment on attachment 112161 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:58
> +    } else if (m_goalRate == 30) {
> +        if (!m_tickStep)
> +            delay = 33;
> +        else
> +            delay = 32;

Er, don't you mean 34 and 33, rather than 33 and 32?

(All this logic seems a bit dodgy.  Wouldn't you start drifting if task posting timing is not perfect?)

[Nat Duca]

Ah, makes sense, thanks for sorting me out.

Two high level things:
1. Lets hold off making this webcore wide. Amongst other things, we might need to go from category strings to something more like LogChannels so it makes more sense to other ports.
2. You'll need two types for every type you introduce in your solution: the WebCore type, and the type you add to WebKit/public/ that you use to get from WebKit back to Chromium. That reality might influence your API design a bit...

[Nat Duca]

(In reply to comment #6)

@*%&@)(* wrong friggin bug. @)@&)*@)$#

[Nat Duca]

> If we only support a fixed number of pending frames using a Deque<> seems like massive overkill. Why not just use a double[]?

Yes it is, but its not performance critical code, and to do [], I need more error prone code. Can we keep it pretty please?

> Frame rates

I would like to start with integral frame rates, actually. If there is something that doesn't use that, we can pass in a ratio instead. E.g. 89.285714hz is 480000 / 5376. The reason for this is I'm trying to avoid numerical precison errors.


> The voodoo with 16 17 17
Stand by for a better patch, with a cooler algorithm.

[Nat Duca]

Created attachment 112309 [details]
Patch

[Nat Duca]

Created attachment 112310 [details]
Patch

[Nat Duca]

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

Note, I gave up on the idea of using integer math for this. It was a stupid idea. :)

[WebKit Review Bot]

Comment on attachment 112310 [details]
Patch

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

[Nat Duca]

Created attachment 112312 [details]
sigh

[Vangelis Kokkevis]

Comment on attachment 112312 [details]
sigh

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

Looks good!

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:61
> +    double now = currentTime();

nit: you could move this up and use now in line 57 instead of calling currentTime() twice

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:66
> +    double amountLate = now - m_nextTickTime;

Should you be worried about timer wrap-around?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:70
> +        // is our tolerance factor for "rasonable" delays due to the thread being busy.

typo: rasonable

[James Robinson]

Comment on attachment 112312 [details]
sigh

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

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:48
> +    CCFrameRateController(PassOwnPtr<CCSmoothedTimer>);

explicit

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:31
> +#include <math.h>

are we using this include?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:62
> +    const float maxLateBeforeDroppingFrame = 2.5;

why float? we're about to compare it to a double

could be static

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:65
> +    bool tick;

Could you initialize this here? Or just drop it if you drop the 'else if' clause below?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:68
> +    if (amountLate >= -1.0 && amountLate <= maxLateBeforeDroppingFrame) {
> +        // Timer was (mostly) on time. We penalize to -1.0 fixed on the early side because

Wouldn't this be a lot easier to reason about if we floored ourselves?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:76
> +    } else if (amountLate < -1.0) {
> +        // Timer came back really early. Unlikely but better to handle then explode.
> +        newNextTickTime = m_nextTickTime;
> +        tick = false;

This should be impossible now that the timers are all base::TimeTicks based - the code in render_widget was to deal with base::Time / base::TimeTicks mismatches. I think you want to ASSERT() at a minimum here if you want to write code at all

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:59
> +    void setInterval(double intervalMs)

Is anything else supposed to happen when the interval changes? What if the interval is 50ms, the last tick was 30ms ago, and the caller sets the interval to 20? Should they get a callback sooner?

I don't see any callers of this in this patch.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:67
> +    virtual double currentTime() const { return monotonicallyIncreasingTime(); }

This is super confusing - there's a function in the global namespace from wtf/CurrentTime.h called 'currentTime()' which happens to have really bad behavior for what we want here. Either match the name of the function you're wrapping or come up with another name

[Nat Duca]

Comment on attachment 112312 [details]
sigh

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

>> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:66
>> +    double amountLate = now - m_nextTickTime;
> 
> Should you be worried about timer wrap-around?

currentTime() would be toast by that point, right?

>> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:68
>> +        // Timer was (mostly) on time. We penalize to -1.0 fixed on the early side because
> 
> Wouldn't this be a lot easier to reason about if we floored ourselves?

Not sure I follow.

[Nat Duca]

Created attachment 112408 [details]
Patch

[WebKit Review Bot]

Comment on attachment 112408 [details]
Patch

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

[Nat Duca]

Created attachment 112426 [details]
MakeItBuild

[WebKit Review Bot]

Comment on attachment 112426 [details]
MakeItBuild

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

[James Robinson]

Comment on attachment 112426 [details]
MakeItBuild

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

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:91
> +    if (tick && m_client)

there is no tick

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:62
> +    virtual double monotonicallyIncreasingTime() const { return monotonicallyIncreasingTime(); }

this is a bit of a trip. can you say "return ::monotonicallyIncreasingTime()" or "return WTF::monotonicallyIncreasingTime()" to make it a bit more human-parseable?

[Nat Duca]

(In reply to comment #21)
> there is no tick

But fortunately, there is also no spoon.

Sorry, couldn't resist.

[Nat Duca]

Created attachment 112442 [details]
.

[James Robinson]

Comment on attachment 112442 [details]
.

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

All nits although OwnPtr<T*> is pretty strange looking. R=me but please check that carefully.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:31
> +#include <math.h>

if you aren't using this #include remove it

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.cpp:46
> +            : m_frameRateController(frameRateController) { }

this indentation is odd - would expect 4-space from prev line

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:75
> +    OwnPtr<CCSmoothedTimer*> m_timer;

why is this an OwnPtr<> to a pointer? is this a typo? How does it compile?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:58
> +    double now = monotonicallyIncreasingTime();

nit: move this below the early return please

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:73
> +        // Timer was (mostly) on time. We penalize to -1.0 fixed on the early side because

I think I've finally grokked this but it took me a while :)  Please let me know if my understanding here is correct: The idea here is if the timer callback rate is 'close enough' to the specified interval rate, then we just keep ticking using the same timebase. We rely on the floor behavior to never drift too early, and if we start drifting too late we reset the timebase rather than pass excessively short intervals.  I think the 'penalize' wording here made it hard to see what's going on.
Maybe you could provide a brief example here of how this code would behave in some simple situations?  For example with an interval of 16.67, infinitely fast frames, a completely uncontested thread, and perfectly accurate timers this would go something like:

STATE_STARTING, now=0: m_nextTickTime = 0, m_state -> STATE_ACTIVE
amountLate = 0
newNextTimeTick = 16.67
postTickTask(floor(16.67 - 0) = 16)

STATE_ACTIVE, now=16:
amountLate = -0.67
newNextTimeTick = 33.34
postTickTask(floor(33.34 - 16) = 17)

STATE_ACTIVE, now=33:
amountLate = -0.34
newNextTimeTick = 50.0
postTickTask(floor(50.0 - 33) = 17)

etc.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:50
> +            : m_client(0)

odd indent here too, would normally be 4-space from previous line

can you put this in the .cpp?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:300
> -        webkit_support::PostDelayedTask(CCLayerTreeHostTest::onBeginTest, static_cast<void*>(this), 0);
> +         webkit_support::PostDelayedTask(CCLayerTreeHostTest::onBeginTest, static_cast<void*>(this), 0);

bizarre space here

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:31
> +#include <wtf/CurrentTime.h>

don't think you need this include either

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:88
> +    FakeCCSmoothedTimer(double i, WebCore::CCThread* t)
> +        : CCSmoothedTimer(i, t)

might as well expand these variable names
i->intervalMs
t->thread

[James Robinson]

Also I still don't really see what the Deque is gonna be for.

[Nat Duca]

Great feedback overall, I'll add some explanation based (heavily) on what you wrote [nice job!], and get a new patch up. :)


(In reply to comment #25)
> Also I still don't really see what the Deque is gonna be for.

Maybe you missed my earlier comment amongst my flailing. The operation we are using is a queue: we push frames in, we pop them out. We also use the queue size to make decisions about dropping a tick. 
We use a dequeue in the rate limiting implementation too, gpu/client/gles2_implemenation.cc as well as in the GpuScheduler for the mac swapbuffer rate limiter.

I'm using WTF::Dequeue because I wasn't sure if it was safe to use stl queue directly. I don't need double-endedness.

I could also use a vector, frankly, and push_back/pop_front it. Its gonna be 2-long so NBD.

Are you wondering about dequeue vs another data structure? Or any stl/wtf data structure versus hand-coding it? I'm confused...

[Iain Merrick]

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

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:36
> +class CCFrameRateControllerClient {

Maybe make this a nested class? i.e. CCFrameRateController::Client

I'm not sure if the style guide prefers it one way or the other, but a nested class would make the code a lot shorter in some places, with little impact elsewhere.

> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:48
> +    explicit CCFrameRateController(PassOwnPtr<CCSmoothedTimer>);

I think this is where my idea of smoothing out a jittery SwapBuffers callback would fit in.

Instead of taking a CCSmoothedTimer argument, CCFrameRateController could just expose a public onTimerTick() method. Then the caller can choose to drive that from a CCSmoothedTimer, from a SwapBuffers callback, or something else.

And maybe the method names should be switched around:

// Call this once per screen refresh
void CCFrameRateController::onVSyncTick()

// CCFrameRateController calls this when we should draw a new frame.
// It will be synced with screen refresh, maybe throttled by an integral factor.
virtual void CCFrameRateControllerClient::onAnimationFrameTick()

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:79
> +        newNextTickTime = now + m_intervalMs;

If the thread is overworked, the timer will end up running at a slightly lower rate, but wouldn't it be better to drop to exactly 50% frame rate? Or is that CCFrameRateController's job?

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:81
> +        // Timer came back impossibly early. Sanity check only.

Does this really need its own case? Could just DCHECK(amountLate >= -1.0) on line 73.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:42
> +// This timer implements a time source that achieves the specified interval

Unless I'm missing something, WebCore/platform/Timer.h does this too. It has an m_nextFireTime which is incremented by m_repeatInterval each time it runs. The key difference here isn't really the smoothing, but the fact that we use CCThread rather than being tied to the main thread.

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:49
> +    CCSmoothedTimer(double intervalMs, CCThread* thread)

For consistency with the rest of WebKit and Chrome, I'd expect a double to be seconds, and a long to be milliseconds (or ticks). Since you're working in doubles here, maybe just use seconds everywhere?

> Source/WebKit/chromium/tests/CCSchedulerTestCommon.h:91
> +    void setMonotonicallyIncreasingTime(double time) { m_monotonicallyIncreasingTime = time; }

Would be convenient in these tests to have an incrementTime() method.

[Iain Merrick]

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

> Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:62
> +    virtual double monotonicallyIncreasingTime() const { return WTF::monotonicallyIncreasingTime(); }

In both tests and production code, this is closely coupled with CCThread::postDelayedTask.

Maybe this should actually be a virtual method on CCThread. The real CCThread can use the real clock, the fake CCThread a fake clock, and everyone's happy.

[Adrienne Walker]

Comment on attachment 112442 [details]
.

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

>> Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:75
>> +    OwnPtr<CCSmoothedTimer*> m_timer;
> 
> why is this an OwnPtr<> to a pointer? is this a typo? How does it compile?

OwnPtr<T*> is identical* to OwnPtr<T> in what it stores, what gets deleted, and how it interacts with PassOwnPtr through the magic of templates.  It's just bad style, not incorrect.

* For nearly all types. HBITMAP (i.e. void*) and some other Windows-only types have specific cleanup functions that get invoked.

[James Robinson]

(In reply to comment #27)
> View in context: https://bugs.webkit.org/attachment.cgi?id=112442&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:36
> > +class CCFrameRateControllerClient {
> 
> Maybe make this a nested class? i.e. CCFrameRateController::Client
> 
> I'm not sure if the style guide prefers it one way or the other, but a nested class would make the code a lot shorter in some places, with little impact elsewhere.
> 

You can't forward declare an inner class, which is why we typically don't use them for cases like this.

[James Robinson]

> (In reply to comment #25)
> > Also I still don't really see what the Deque is gonna be for.
> 
> Maybe you missed my earlier comment amongst my flailing. The operation we are using is a queue: we push frames in, we pop them out. We also use the queue size to make decisions about dropping a tick. 
> We use a dequeue in the rate limiting implementation too, gpu/client/gles2_implemenation.cc as well as in the GpuScheduler for the mac swapbuffer rate limiter.
> 
> I'm using WTF::Dequeue because I wasn't sure if it was safe to use stl queue directly. I don't need double-endedness.
> 
> I could also use a vector, frankly, and push_back/pop_front it. Its gonna be 2-long so NBD.
> 
> Are you wondering about dequeue vs another data structure? Or any stl/wtf data structure versus hand-coding it? I'm confused...

It looks like this isn't input into the current scheme and it's unclear exactly how it will interact with everything else. Could you just add the support when you add the logic using it (and the relevant tests)?

[Nat Duca]

(In reply to comment #27)
> View in context: https://bugs.webkit.org/attachment.cgi?id=112442&action=review
> 
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCFrameRateController.h:48
> > +    explicit CCFrameRateController(PassOwnPtr<CCSmoothedTimer>);
> 
> I think this is where my idea of smoothing out a jittery SwapBuffers callback would fit in.

I added a CCTimeSource. Something based on a swapbuffers or vsync would be a subclass of a timesource. The thing formerly called CCSmoothedTimer is now called CCDelayBasedTimeSource.

> And maybe the method names should be switched around:
I did some changes here. I think you'll like it.

> If the thread is overworked, the timer will end up running at a slightly lower rate, but wouldn't it be better to drop to exactly 50% frame rate?

That's the job of CCFrameRateController's job. The timesource gives ticks, the framerate controller decides to ignore them. :)

A *key* problem is deciding when the thread (or the GPU) is overworked. We lack metrics to determine this at runtime.

> > Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.cpp:81
> > +        // Timer came back impossibly early. Sanity check only.
> 
> Does this really need its own case? Could just DCHECK(amountLate >= -1.0) on line 73.
Doh. Thanks man, this is SO much cleaner now.

> > Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:42
> > +// This timer implements a time source that achieves the specified interval
> 
> Unless I'm missing something, WebCore/platform/Timer.h does this too.

Its really that this is CCThread-coupled, platform-coupled. That has a few non-obvious consequences around the timebase [monotonic time vs webkit seconds, which are not the same iirw].

> > Source/WebCore/platform/graphics/chromium/cc/CCSmoothedTimer.h:49
> > +    CCSmoothedTimer(double intervalMs, CCThread* thread)
> For consistency with the rest of WebKit and Chrome, I'd expect a double to be seconds, and a long to be milliseconds (or ticks).
Again, the disconnect is that currentTime() uses a wall-time timebase. The monotonic time uses a special timebase that we guarantee to not move backward, even when the clock changes. It also doesn't count from the epoch, which gives increased numeric precision down in the micro/nano range --- epoch based doubles can't count below 0.3microsecnds.


> CCThread::currentTime()
I buy it. But we should make this work for CCMainThread, too. I think that can be done by having CCMainThread implement CCThread as well. I'm open to a patch to do this, as I agree it is cleaner.

[Nat Duca]

Created attachment 112621 [details]
Cleanup

[James Robinson]

Comment on attachment 112621 [details]
Cleanup

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

R still =me. I like the explanation a lot.

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.cpp:84
> +// We treat delays in tasks differently depending on the amount of delay we encouter. Suppose we

typo 'encouter'->'encounter'. 'encouter' sounds like a completely different sort of thing entirely

> Source/WebCore/platform/graphics/chromium/cc/CCDelayBasedTimeSource.h:40
> +// NOTE: Deleting CCDelayBasedTimeSource when a tick is pending will lead to a crash.
> +//      To fix this, we need to generalize CCScopedMainThreadProxy.

instead of just saying that this crashes, i think you can fix this by making this class refcounted and calling ref() in postTickTask() and deref() before returning from onTick(). it's not much code and one less headache to deal with later

[Nat Duca]

Committed r98700: <http://trac.webkit.org/changeset/98700>