Bug 71678 – [chromium] MediaStream API: Adding embedding code for GetUserMedia

• Component: WebKit API
• Status: RESOLVED FIXED

[Tommy Widenflycht]

Adding WebUserMediaRequest and UserMediaClientImpl

[Tommy Widenflycht]

Created attachment 113858 [details]
Patch

[Adam Barth]

Comment on attachment 113858 [details]
Patch

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

I would have expected the Client to implement UserMediaClient rather then having WebKit/chromium/src implement UserMediaClient in terms of WebKitPlatformSupport.  UserMediaClient seems to be asking for a policy decision, which is the purview of the Client.

> Source/WebKit/chromium/public/WebKitPlatformSupport.h:332
> +    virtual void requestUserMedia(const WebUserMediaRequest&) { }
> +    virtual void cancelUserMediaRequest(const WebUserMediaRequest&) { }

I would have expected pointers, not references.

[Tommy Widenflycht]

Created attachment 114065 [details]
Patch

[Tommy Widenflycht]

Something along these lines?

[Adam Barth]

Comment on attachment 114065 [details]
Patch

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

Yes.  This looks great.

> Source/WebKit/chromium/src/UserMediaClientImpl.h:37
> +

Extra blank line here.

[Darin Fisher (:fishd, Google)]

Comment on attachment 114065 [details]
Patch

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

> Source/WebKit/chromium/public/WebUserMediaClient.h:36
> +    virtual void userMediaControllerDestroyed() = 0;

What "user media controller" does this method correspond to?  maybe this method should be named cancelAllUserMediaRequests?  is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called?  do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors?  no need to report an error code of some sort in other words?

> Source/WebKit/chromium/public/WebUserMediaClient.h:38
> +    virtual void cancelUserMediaRequest(const WebUserMediaRequest&) = 0;

it seems like the embedder will need some way to compare WebUserMediaRequest objects for equality.  otherwise, it will be difficult to correlate a cancelUserMediaRequest to a requestUserMedia call.  the WebUserMediaRequest object could be copied, so you cannot use pointer identity.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:49
> +    void requestSucceeded(const WebVector<WebMediaStreamSource>&);

it looks like these will require WEBKIT_EXPORT too

[Tommy Widenflycht]

Comment on attachment 114065 [details]
Patch

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

>> Source/WebKit/chromium/public/WebUserMediaClient.h:36
>> +    virtual void userMediaControllerDestroyed() = 0;
> 
> What "user media controller" does this method correspond to?  maybe this method should be named cancelAllUserMediaRequests?  is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called?  do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors?  no need to report an error code of some sort in other words?

This is the UserMediaController hanging of the page.

I have asked for clarification regarding your second question from the WebCore patch author, and would prefer if cancelUserMediaRequest is called for each outstanding request in addition to this function.

And lastly, no we do not care about what went wrong since unfortunately there isn't a way to convey that error currently in the WebRTC spec.

>> Source/WebKit/chromium/public/WebUserMediaClient.h:38
>> +    virtual void cancelUserMediaRequest(const WebUserMediaRequest&) = 0;
> 
> it seems like the embedder will need some way to compare WebUserMediaRequest objects for equality.  otherwise, it will be difficult to correlate a cancelUserMediaRequest to a requestUserMedia call.  the WebUserMediaRequest object could be copied, so you cannot use pointer identity.

Fixed.

>> Source/WebKit/chromium/public/WebUserMediaRequest.h:49
>> +    void requestSucceeded(const WebVector<WebMediaStreamSource>&);
> 
> it looks like these will require WEBKIT_EXPORT too

Fixed.

>> Source/WebKit/chromium/src/UserMediaClientImpl.h:37
>> +
> 
> Extra blank line here.

Fixed.

[Tommy Widenflycht]

Created attachment 114245 [details]
Patch

[Tommy Widenflycht]

Created attachment 114255 [details]
Patch

[Tommy Widenflycht]

Created attachment 114652 [details]
Patch

[Tommy Widenflycht]

> > Source/WebKit/chromium/public/WebUserMediaClient.h:36
> > +    virtual void userMediaControllerDestroyed() = 0;
> 
> What "user media controller" does this method correspond to?  maybe this method should be named cancelAllUserMediaRequests?  is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called?  do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors?  no need to report an error code of some sort in other words?
> 

All outstanding request will be cancelled before this call. This function is more along the general cleanup line.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> (From update of attachment 114065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114065&action=review
> 
> >> Source/WebKit/chromium/public/WebUserMediaClient.h:36
> >> +    virtual void userMediaControllerDestroyed() = 0;
> > 
> > What "user media controller" does this method correspond to?  maybe this method should be named cancelAllUserMediaRequests?  is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called?  do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors?  no need to report an error code of some sort in other words?
> 
> This is the UserMediaController hanging of the page.

From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist.  Why do we need this method on WebUserMediaClient, and can we give it a name that makes sense from the perspective of the embedder?

[Adam Barth]

> From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist.

... and now it doesn't exist in WebCore either.  :)

[Tommy Widenflycht]

Comment on attachment 114065 [details]
Patch

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

>>>> Source/WebKit/chromium/public/WebUserMediaClient.h:36
>>>> +    virtual void userMediaControllerDestroyed() = 0;
>>> 
>>> What "user media controller" does this method correspond to?  maybe this method should be named cancelAllUserMediaRequests?  is the expectation that a cancelled WebUserMediaRequest will have its requestFailed method called?  do you not care about distinguishing requests that fail due to cancelation from those that fail due to other errors?  no need to report an error code of some sort in other words?
>> 
>> This is the UserMediaController hanging of the page.
>> 
>> I have asked for clarification regarding your second question from the WebCore patch author, and would prefer if cancelUserMediaRequest is called for each outstanding request in addition to this function.
>> 
>> And lastly, no we do not care about what went wrong since unfortunately there isn't a way to convey that error currently in the WebRTC spec.
> 
> From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist.  Why do we need this method on WebUserMediaClient, and can we give it a name that makes sense from the perspective of the embedder?

This functionality is very important to have, because it signals that the system should shut down all LocalMediaStreams and their camera/audio drivers etc. Would calling it close() or shutdown() be better?

[Tommy Widenflycht]

(In reply to comment #13)
> > From the point of view of the embedder, who only sees WebKit APIs, the UserMediaController handing off of the page does not exist.
> 
> ... and now it doesn't exist in WebCore either.  :)

Unfortunately it got removed :(

[Adam Barth]

> This functionality is very important to have, because it signals that the system should shut down all LocalMediaStreams and their camera/audio drivers etc. Would calling it close() or shutdown() be better?

Why doesn't each LocalMediaStream signal that individually?

[Tommy Widenflycht]

(In reply to comment #16)
> > This functionality is very important to have, because it signals that the system should shut down all LocalMediaStreams and their camera/audio drivers etc. Would calling it close() or shutdown() be better?
> 
> Why doesn't each LocalMediaStream signal that individually?

We need to now when the page is going away (page reload, closing page etc) not when the objects are garbage collected. I thought that it was best to detect that in the Document object but I am happy to learn other and better ways to reach the same goal.

[Adam Barth]

It sounds like whatever is holding the LocalMediaStream objects should watch for being disconnected from the Frame (or removed from the document, etc) and then close the LocalMediaStream (which should signal the platform to spin down the devices).

[Adam Barth]

Tommy and I chatted about this topic, and we think the best solution is to make MediaStream an ActiveDOMObject, which will let it get a stop() callback when the Document detaches from the Frame.  It will also let it get suspend/resume calls for browsers that implement bfcache.

Everything should be set for that approach because MediaStream already gets a ScriptExecutionContext in its constructor.

[Adam Bergkvist]

(In reply to comment #19)
> Tommy and I chatted about this topic, and we think the best solution is to make MediaStream an ActiveDOMObject, which will let it get a stop() callback when the Document detaches from the Frame.  It will also let it get suspend/resume calls for browsers that implement bfcache.
> 
> Everything should be set for that approach because MediaStream already gets a ScriptExecutionContext in its constructor.

I'm not so sure that making MediaStream an ActiveDOMObject is the right approach. We see MediaStream/LocalMediaStream as an abstract representation of an actual media stream in the platform (similar to how a Blob can represent a file in the filesystem). The underlying media stream in the platform is set up when the media stream is consumed. The consumers of MediaStream objects are (currently) PeerConnection and the media elements, which have well defined lifetime handling.

Another aspect is that a MediaStream object can go away but the underlying media stream must still be consumable if there is a URL created for it using createObjectURL(). That's why we have MediaStreamDescriptor (to allow MediaStream objects to be garbage collected). 

Perhaps we should move this discussion to the MediaStream platform bug (http://webkit.org/b/70895)?

[Tommy Widenflycht]

Regarding the discussion taking place in the last comments, I have decided to put this on hold to se if we can get the necessary signalling from the video decoder instead.

[Tommy Widenflycht]

Created attachment 115578 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Tommy Widenflycht]

Created attachment 115579 [details]
Patch

[Adam Barth]

Comment on attachment 115579 [details]
Patch

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

> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122
> +    UserMediaRequest* p = other.m_private.get();
> +    if (p)
> +        p->ref();
> +    m_private = p;

Is there a way to do this without manual reference counting?

[Tommy Widenflycht]

Comment on attachment 115579 [details]
Patch

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

>> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122
>> +    m_private = p;
> 
> Is there a way to do this without manual reference counting?

The WebPrivatePtr takes over the refcounted object using leakref() and the calls dererf() at destructor time, so for an object referenced from elsewhere this is needed. Dunno exactly about the design philosophy but hope that Darin can teach me the correct way here.

[Adam Barth]

Comment on attachment 115579 [details]
Patch

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

>>> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122
>>> +    UserMediaRequest* p = other.m_private.get();
>>> +    if (p)
>>> +        p->ref();
>>> +    m_private = p;
>> 
>> Is there a way to do this without manual reference counting?
> 
> The WebPrivatePtr takes over the refcounted object using leakref() and the calls dererf() at destructor time, so for an object referenced from elsewhere this is needed. Dunno exactly about the design philosophy but hope that Darin can teach me the correct way here.

Can we use "RefPtr<UserMediaRequest> p" instead?  (I guess I could look at the definition of WebPrivatePtr in more detail).

[Darin Fisher (:fishd, Google)]

Comment on attachment 115579 [details]
Patch

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

> Source/WebKit/chromium/public/WebUserMediaClient.h:28
> +#include "WebVector.h"

nit: please just forward declare WebVector (see for example WebPageSerializer.h)

> Source/WebKit/chromium/public/WebUserMediaClient.h:39
> +    virtual void pageDestroyed() = 0;

"page" (referring to WebCore::Page) is also a name that doesn't exist in the WebKit API.

I think a name like "cancelAllUserMediaRequests" might be better here.  It is much more specific.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:49
> +    WEBKIT_EXPORT WebUserMediaRequest& operator=(const WebUserMediaRequest&);

as with other interface, please define an assign method, and implement operator=
in terms of that.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:60
> +    WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&);

There was some discussion (over email) about making requestSucceeded and requestFailed
virtual to support mocking in unit tests.  I commented that I thought perhaps that
suggested that these functions really don't belong here on the 'request' object.

Compare this to WebURLRequest, which just describes the parameters of a request.  It
is just data.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:63
> +    WEBKIT_EXPORT bool operator==(const WebUserMediaRequest&) const;

nit: we don't export operators in the WebKit API.  Please add an equals method,
and then define an inline operator== that calls the equals method.  See WebNode.h
for example.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:66
> +    WebUserMediaRequest(WebCore::UserMediaRequest*);

nit: do you really need both forms of constructor and both forms of operator here?
we don't elsewhere in the API.

> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:102
> +    m_private->succeed(s);

I'm trying to study what happens when succeed and fail are called, but I don't see
UserMediaRequest checked into the tree.  Is there a separate bug with that patch?

I'm a little concerned about the idea that UserMediaRequest is both data representing
the request and controller code for what happens when the request succeeds or fails.

[Adam Barth]

> I'm trying to study what happens when succeed and fail are called, but I don't see
> UserMediaRequest checked into the tree.  Is there a separate bug with that patch?

http://trac.webkit.org/browser/trunk/Source/WebCore/mediastream/UserMediaRequest.h
http://trac.webkit.org/browser/trunk/Source/WebCore/mediastream/UserMediaRequest.cpp

It might not have been rolled into Chromium yet.

[Darin Fisher (:fishd, Google)]

(In reply to comment #29)
> It might not have been rolled into Chromium yet.

Ah, yes.  I was using codesearch :-(

So, UserMediaRequest ends up having a m_scriptExecutionContext member precisely for it to handle these events.  It seems unfortunate for UserMediaRequest to be both data representing the request as well as controller that knows how to process completion of the request.

[Adam Barth]

> So, UserMediaRequest ends up having a m_scriptExecutionContext member precisely for it to handle these events.  It seems unfortunate for UserMediaRequest to be both data representing the request as well as controller that knows how to process completion of the request.

I've been pushing this code not to have long-lived (e.g., Document- or Page-scoped) controllers.  Maybe the best solution is to split UserMediaRequest into UserMediaRequester and UserMediaRequest, with the later being just pure data.  That would keep the lifetime of these objects short (i.e., limited to the lifetime of the request).

[Darin Fisher (:fishd, Google)]

(In reply to comment #31)
> > So, UserMediaRequest ends up having a m_scriptExecutionContext member precisely for it to handle these events.  It seems unfortunate for UserMediaRequest to be both data representing the request as well as controller that knows how to process completion of the request.
> 
> I've been pushing this code not to have long-lived (e.g., Document- or Page-scoped) controllers.  Maybe the best solution is to split UserMediaRequest into UserMediaRequester and UserMediaRequest, with the later being just pure data.  That would keep the lifetime of these objects short (i.e., limited to the lifetime of the request).

I see.  Hmm...

[Tommy Widenflycht]

Created attachment 116057 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 115579 [details]
Patch

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

>> Source/WebKit/chromium/public/WebUserMediaClient.h:28
>> +#include "WebVector.h"
> 
> nit: please just forward declare WebVector (see for example WebPageSerializer.h)

Done.

>> Source/WebKit/chromium/public/WebUserMediaClient.h:39
>> +    virtual void pageDestroyed() = 0;
> 
> "page" (referring to WebCore::Page) is also a name that doesn't exist in the WebKit API.
> 
> I think a name like "cancelAllUserMediaRequests" might be better here.  It is much more specific.

Since all outstanding requests should have been cancelled before, I'm just calling it close()

>> Source/WebKit/chromium/public/WebUserMediaRequest.h:49
>> +    WEBKIT_EXPORT WebUserMediaRequest& operator=(const WebUserMediaRequest&);
> 
> as with other interface, please define an assign method, and implement operator=
> in terms of that.

That is exactly what I had done, but I guess you mean to also inline operator=?

>> Source/WebKit/chromium/public/WebUserMediaRequest.h:60
>> +    WEBKIT_EXPORT void requestSucceeded(const WebVector<WebMediaStreamSource>&);
> 
> There was some discussion (over email) about making requestSucceeded and requestFailed
> virtual to support mocking in unit tests.  I commented that I thought perhaps that
> suggested that these functions really don't belong here on the 'request' object.
> 
> Compare this to WebURLRequest, which just describes the parameters of a request.  It
> is just data.

I'm just wrapping existing WebCore code.
Do you want me to split this class into data and controller objects here in the chromium embedder layer or does the WebCore class need to be rewritten before we can try to add the corresponding WebKit code again?

>> Source/WebKit/chromium/public/WebUserMediaRequest.h:63
>> +    WEBKIT_EXPORT bool operator==(const WebUserMediaRequest&) const;
> 
> nit: we don't export operators in the WebKit API.  Please add an equals method,
> and then define an inline operator== that calls the equals method.  See WebNode.h
> for example.

Done.

>> Source/WebKit/chromium/public/WebUserMediaRequest.h:66
>> +    WebUserMediaRequest(WebCore::UserMediaRequest*);
> 
> nit: do you really need both forms of constructor and both forms of operator here?
> we don't elsewhere in the API.

Removed.

>>>> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:122
>>>> +    m_private = p;
>>> 
>>> Is there a way to do this without manual reference counting?
>> 
>> The WebPrivatePtr takes over the refcounted object using leakref() and the calls dererf() at destructor time, so for an object referenced from elsewhere this is needed. Dunno exactly about the design philosophy but hope that Darin can teach me the correct way here.
> 
> Can we use "RefPtr<UserMediaRequest> p" instead?  (I guess I could look at the definition of WebPrivatePtr in more detail).

The pattern is to use WebPrivatePtr

[Tommy Widenflycht]

Created attachment 116081 [details]
Patch

[Tommy Widenflycht]

The difference between the latest patch and the one before it is that I have introduced a WebUserMediaController and made WebUserMediaRequest a data only object. Is this a better approach?

[Darin Fisher (:fishd, Google)]

Comment on attachment 116081 [details]
Patch

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

> Source/WebKit/chromium/public/WebUserMediaClient.h:39
> +    virtual void useController(WebUserMediaController*);

I would think that this should be a WebView::userMediaController getter, just
as you have WebViewClient::userMediaClient.

> Source/WebKit/chromium/public/WebUserMediaClient.h:40
> +    virtual void close() = 0;

If this is not about cancelling pending WebUserMediaRequests, then what is it for?  Why do we need this |close| function?

[Darin Fisher (:fishd, Google)]

Comment on attachment 116081 [details]
Patch

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

> Source/WebKit/chromium/src/UserMediaClientImpl.h:43
> +class UserMediaClientImpl : public WebCore::UserMediaClient, public WebUserMediaController {

If WebUserMediaController is the way to go, then it seems UserMediaController would also
be warranted for the WebCore side.

[Tommy Widenflycht]

Created attachment 116110 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 116110 [details]
Patch

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

> Source/WebKit/chromium/src/UserMediaClientImpl.cpp:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

nit: this copyright notice incorrectly says that this software is provided by "APPLE"

I think you should switch "APPLE" to "GOOGLE" here, and in the other newly added files.

> Source/WebKit/chromium/src/UserMediaClientImpl.h:47
> +    virtual void pageDestroyed();

nit: Maybe we can delete UserMediaClient::pageDestroyed now?

[Tommy Widenflycht]

Comment on attachment 116110 [details]
Patch

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

>> Source/WebKit/chromium/src/UserMediaClientImpl.cpp:14
>> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> 
> nit: this copyright notice incorrectly says that this software is provided by "APPLE"
> 
> I think you should switch "APPLE" to "GOOGLE" here, and in the other newly added files.

Will fix.

>> Source/WebKit/chromium/src/UserMediaClientImpl.h:47
>> +    virtual void pageDestroyed();
> 
> nit: Maybe we can delete UserMediaClient::pageDestroyed now?

I think that Ericsson needs this for their GTK/gstreamer port.

[Tommy Widenflycht]

Created attachment 116121 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116121 [details]
Patch

Clearing flags on attachment: 116121

Committed r100970: <http://trac.webkit.org/changeset/100970>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tommy Widenflycht]

The difference between the latest patch and the one before it is that I have introduced a WebUserMediaController and made WebUserMediaRequest a data only object. Is this a better approach?

[Tommy Widenflycht]

Created attachment 116203 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 116203 [details]
Patch

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

> Source/WebKit/chromium/src/UserMediaClientImpl.cpp:46
> +    : m_webClient(client ? client->userMediaClient() : 0)

This is the only line that differs from the previous patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116203 [details]
Patch

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

> Source/WebKit/chromium/src/UserMediaClientImpl.h:49
> +    UserMediaClientImpl(WebViewClient*);

nit: more conventional to initialize with a WebViewImpl*.  see EditorClientImpl.

> Source/WebKit/chromium/src/UserMediaClientImpl.h:59
> +    WebUserMediaClient* m_webClient;

nit: more conventional to just call this variable m_client.

> Source/WebKit/chromium/src/WebViewImpl.cpp:375
> +    , m_userMediaClient(adoptPtr(new UserMediaClientImpl(client)))

why does m_userMediaClient need to be heap allocated?  see m_editorClientImpl for example.
it would also be a bit more conventional to name m_userMediaClient as m_userMediaClientImpl.

[Tommy Widenflycht]

Comment on attachment 116203 [details]
Patch

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

>> Source/WebKit/chromium/src/UserMediaClientImpl.h:49
>> +    UserMediaClientImpl(WebViewClient*);
> 
> nit: more conventional to initialize with a WebViewImpl*.  see EditorClientImpl.

Done.

>> Source/WebKit/chromium/src/UserMediaClientImpl.h:59
>> +    WebUserMediaClient* m_webClient;
> 
> nit: more conventional to just call this variable m_client.

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:375
>> +    , m_userMediaClient(adoptPtr(new UserMediaClientImpl(client)))
> 
> why does m_userMediaClient need to be heap allocated?  see m_editorClientImpl for example.
> it would also be a bit more conventional to name m_userMediaClient as m_userMediaClientImpl.

Fixed^2. It is a mix of heap allocated and contained objects in this class and I just choose the wrong one.

[Tommy Widenflycht]

Created attachment 116253 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116253 [details]
Patch

Clearing flags on attachment: 116253

Committed r101058: <http://trac.webkit.org/changeset/101058>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.