[Webkit-unassigned] [Bug 58787] Draft PeerConnection implementation (experimental feature)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 20 11:38:48 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=58787
--- Comment #7 from Leandro Graciá Gil <leandrogracia at chromium.org> 2011-05-20 11:38:48 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=94070&action=review
> LayoutTests/fast/dom/MediaStream/script-tests/add-remove-streams.js:11
> + pc = new PeerConnection('foobar', function(m) {});
Is there any test checking the behavior of the constructor when different and unexpected types and numbers of arguments are passed? Something like the existing argument-types test.
> LayoutTests/fast/dom/MediaStream/script-tests/add-remove-streams.js:29
> + shouldBe('streams[0]', 'stream');
Maybe it should also try invalid values for indices and see if the proper exceptions are thrown.
> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.
I think the license here and all the other files is not right. It should follow Source/WebKit/LICENSE, where the copyright itself can be set to Google. If you want an example just take a look to the license used in the landed MediaStream code.
> Source/WebCore/bindings/js/JSPeerConnectionCustom.cpp:49
> + RefPtr<SignallingCallback> signallingCallback = createFunctionOnlyCallback<JSSignallingCallback>(exec, static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), exec->argument(1));
Can you make sure that the 'TYPE_MISMATCH_ERR' thrown by this method (see bindings/js/CallbackFunction.cpp) is the same as the TypeError mentioned in the spec?
> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:474
> + return toV8(peerConnection);
Style nit: indentation of 4 spaces instead of 2.
> Source/WebCore/bindings/v8/custom/V8EventCustom.cpp:59
> +#include "V8StreamEvent.h"
Style nit: wrong header ordering.
> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:58
> + if (isUndefinedOrNull(args[1]))
The spec requires the signaling callback to be not null, but doesn't mention undefined. Isn't this situation already covered by checking the number of arguments? As far as I know, for args[1] to be undefined the number of arguments should be less than 2. Please check and correct me if I'm wrong.
In any case, please make sure that this is not either undefined or null when calling createFunctionOnlyCallback as otherwise it will call V8Proxy::throwError(V8Proxy::GeneralError, "TYPE_MISMATCH_ERR: DOM Exception 17").
> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:62
> + AtomicString configuration = v8ValueToAtomicWebCoreString(args[0]);
Make sure this really throws exceptions. I expected toWebCoreString to throw them but found that it didn't, and had to change it for args[0]->ToString().
> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:78
> + if (!peerConnection.get())
You should be able to check a RefPtr without using get.
> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:79
> + return throwError("A PeerConnection object could not be created.", V8Proxy::RangeError);
I see no reference to RangeError in the spec. Is this right?
> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:81
> + // Transform the holder into a wrapper object.
Comment likely to be considered unnecessary by the reviewers as the code is enough self-explanatory.
> Source/WebCore/dom/EventNames.h:186
> + macro(peerconnectionmessage) \
I see no onpeerconnectionmessage on the spec. What is the purpose of this?
> Source/WebCore/dom/GeneratedStream.cpp:68
> + : Stream(frameController, label, false)
You should be able to tell if it's local or remote by calling the isGeneratedStream method. It should be safe now even when coming from the destructor with the lastest proposed changes for the MediaStreamFrameController and Stream/GeneratedStream objects in bug 56666.
This change should no longer be required by this patch.
> Source/WebCore/dom/PeerConnection.cpp:52
> + : PeerConnectionClient(frameController, id), m_readyState(NEW), m_signallingCallback(signallingCallback)
Does not follow the initialization list style. Look in http://www.webkit.org/coding/coding-style.html, section "Other Punctiation".
> Source/WebCore/dom/PeerConnection.cpp:56
> + mediaStreamFrameController()->startNegotiation(m_clientId);
Given the circumstances the frame controller should be always available at this point. However it may be a good idea to assert it before calling it.
> Source/WebCore/dom/PeerConnection.cpp:63
> +PassRefPtr<StreamList> PeerConnection::localStreams()
Should be const. Same for removeStreams.
> Source/WebCore/dom/PeerConnection.cpp:65
> + return StreamList::create(m_localStreams);
I don't think the spec says we should create a new stream list everytime, especially since they are reference-counted objects (they need to as they are script objects). It might be better to keep a reference to them in the object and simple return it everytime, but please check it.
Same for removeStreams.
> Source/WebCore/dom/PeerConnection.cpp:81
> + mediaStreamFrameController()->signalingMessage(m_clientId, message);
Please check that the frame controller is available. It won't be in the detached frame state and no operations should be performed in that situation.
This affects also many other methods in this file.
> Source/WebCore/dom/PeerConnection.cpp:86
> + if (m_readyState == CLOSED) {
The exception code ec should be set to 0 here or before the method finishes. Please check here and in the following methods using ExceptionCode.
> Source/WebCore/dom/PeerConnection.cpp:107
> + // Stream is guaranteed to exist.
Not sure if necessary and possibly a bit misleading since this is the usual way to manipulate PassRefPtr arguments.
> Source/WebCore/dom/PeerConnection.cpp:146
> + dispatchEvent(Event::create(eventNames().closeEvent, false, false));
You should not dispatch the event synchronously but put a task to do it. You can find examples of this in the StringCallback, *TrackList or *Stream classes.
> Source/WebCore/dom/PeerConnection.cpp:151
> + m_readyState = NEGOTIATING;
It may be a good idea to assert that we come from the right state.
> Source/WebCore/dom/PeerConnection.cpp:153
> + dispatchEvent(Event::create(eventNames().connectingEvent, false, false));
Probably the same as before. Please check here and in the following methods.
> Source/WebCore/dom/PeerConnection.cpp:198
> + return mediaStreamFrameController()->scriptExecutionContext();
You should check if the mediaStreamFrameController is available also here.
> Source/WebCore/dom/PeerConnection.h:39
> +#include "ScriptExecutionContext.h"
Forward-declaring should be enough here.
> Source/WebCore/dom/PeerConnection.h:40
> +#include "SignallingCallback.h"
Forward-declaring them should be enough for RefPtr and PassRefPtr, but make sure you're not instantiating anything in an inline header method or the compiler will complain.
You can forward-declare and remove this include if you move the getter to the cpp file.
> Source/WebCore/dom/PeerConnection.h:41
> +#include "Stream.h"
Can forward-declare too.
> Source/WebCore/dom/PeerConnection.h:42
> +#include "StreamContainer.h"
Same as before.
> Source/WebCore/dom/PeerConnection.h:43
> +#include "StreamList.h"
Same as before.
> Source/WebCore/dom/PeerConnection.h:45
> +#include <wtf/HashMap.h>
It doesn't look like you're using this in the header file.
> Source/WebCore/dom/PeerConnection.h:48
> +#include <wtf/Vector.h>
Same as with HashMap.
> Source/WebCore/dom/PeerConnection.h:67
> + PassRefPtr<StreamList> localStreams();
Never do this. Don't hold any PassRefPtr as a local variable, but use RefPtr instead. Using PassRefPtr should be limited to function arguments and return types.
Also, shouldn't this be const?
> Source/WebCore/dom/PeerConnection.h:86
> + PassRefPtr<SignallingCallback> signallingCallback() { return m_signallingCallback; }
Apart of moving it to the cpp file, shouldn't this be const?
> Source/WebCore/dom/PeerConnection.h:99
> + void streamRemoved(String streamLabel);
Argument type should be const String&.
> Source/WebCore/dom/PeerConnection.h:110
> + unsigned short m_readyState;
Is there something inheriting from PeerConnection that requires this to be protected instead of private?
> Source/WebCore/dom/SignallingCallback.h:36
> +class SignallingCallback : public RefCounted<SignallingCallback> {
Is there any situation where a call to this callback can be generated synchronously from something coming from JS? If so, this should inherit from CallbackTask2 and ensure that the call is scheduled in such situations.
Also make sure you add the SignalingCallback to the no-interface-object layout test.
> Source/WebCore/dom/Stream.cpp:36
> +PassRefPtr<Stream> Stream::create(MediaStreamFrameController* frameController, const String& label, bool remoteStream)
No longer required with the changes introduced by bug 56666. Same for the other changes in both cpp and header files.
> Source/WebCore/dom/StreamContainer.h:49
> + unsigned length() const { return m_streams.size(); }
Make sure you don't get signed/unsigned or other int conversion warnings in mac, as the compiler it more picky than gcc and these warnings will become errors.
> Source/WebCore/dom/StreamContainer.h:58
> + return 0;
It's probably better to return PassRefPtr<Stream>().
> Source/WebCore/dom/StreamContainer.h:70
> + m_streams.remove(s->label());
Since remove is silent in case the provided key doesn't exist, it's probably worth to add an assertion here to verify it.
> Source/WebCore/dom/StreamContainer.h:73
> + bool contains(PassRefPtr<Stream> stream)
Should this be const?
> Source/WebCore/dom/StreamEvent.cpp:45
> + : Event(type, canBubble, cancelable), m_stream(stream)
Does not follow the initialization list style. Look in http://www.webkit.org/coding/coding-style.html, section "Other Punctiation".
> Source/WebCore/dom/StreamEvent.h:37
> +#include "Stream.h"
Could replace by a forward declaration if the code of the factory and stream methods were moved to the cpp file.
> Source/WebCore/dom/StreamList.cpp:45
> + return 0;
As I've seen recently in some changes during rebasing, it seems to be better to return PassRefPtr<Stream>() than 0.
> Source/WebCore/dom/StreamList.h:34
> +#include <StreamContainer.h>
Not strictly necessary, but you could replace this with a forward declaration if you move the code for the factory method, the constructor and the destructor to the cpp file.
> Source/WebCore/page/MediaStreamController.cpp:230
> + // Don't assert since the frame controller can have been destroyed
Style nit: comment sentences should end always with a dot.
> Source/WebCore/page/MediaStreamController.cpp:241
> + int pagePeerConnectionId(-1);
Although correct I'm not sure if I've seen this style anywhere else in WebCore. Also, theoretically 0 is an invalid id in case you want to use it instead of -1. Just make sure you never try to access a hash map with 0 or an empty string as the key.
> Source/WebCore/page/MediaStreamController.cpp:271
> + const Request& peerConnection = m_peerConnections.get(controllerPeerConnectionId);
This is fine, but please keep in mind that HashMap is returning a copy of the object it holds (and if it's not right please do correct me). It may be a problem in case you extend the functionality of Request without keeping this in mind.
> Source/WebCore/page/MediaStreamController.h:74
> + void registerPeerConnection(int peerConnectionId, MediaStreamFrameController*);
Minor detail: please make the order of the arguments consistent with similar calls in the file.
> Source/WebCore/page/MediaStreamController.h:75
> + void signalingMessage(MediaStreamFrameController*, int framePeerConnectionId, const AtomicString& message);
Are you sure that you need to add the frame controller in all these calls? You can probably tell already by the peer connection id once it has been registered.
> Source/WebCore/page/MediaStreamController.h:77
> + void addStream(MediaStreamFrameController*, int framePeerConnectionId, const AtomicString& streamLabel);
In this case you can even use the stream label alone to identify the frame controller, as the local stream generation message should have already passed through here at any point this call is made.
Same for removeStream or anything involving local streams.
> Source/WebCore/page/MediaStreamFrameController.cpp:329
> + // TODO: Should this be supported for remote streams too?
I'm afraid the spec doesn't say otherwise, and also includes the record method in Stream instead of StreamRecorder.
It may be a good idea to raise the question to whatwg if you have security concerns.
> Source/WebCore/page/MediaStreamFrameController.cpp:501
> + // and pageController isn't currently valid? Just skip registering?
Please use the isClientAvailable method and ignore any requests if it returns false. This is because we might have access to a valid page controller, but it might not have a MediaStream client. In fact this is the current situation until the WebKit client is added.
Apply this also to similar situations in this file.
> Source/WebCore/page/MediaStreamFrameController.cpp:514
> + peerConnection->signallingCallback()->handleEvent(message, peerConnection.get());
Make sure that this line cannot ever be reached synchronously by anything that comes from Javascript. If that's not the case, you should schedule the callback using the CallbackTask2 features.
> Source/WebCore/page/MediaStreamFrameController.h:130
> + if (!m_isRemoteStream)
I don't think this is right. Is there any reason of why remote streams should not be unregistered? Please note that 'unregistered' means at least to remove them from the corresponding client table, and this table is also used to report clients about the embedder becoming unreachable. It will crash if a remote stream goes out of scope and then the frame containing its associated PeerConnection object is detached from the page.
Instead, I think you should always unregister but define different specific behaviours in the unregister method by using isGeneratedStream(). This method should be safe now even in destruction environments with the lastest changes in bug 56666.
> Source/WebCore/platform/mock/MediaStreamClientMock.cpp:85
> + static PassRefPtr<Stream> create(String label);
Argument type should be const String&.
> Source/WebCore/platform/mock/MediaStreamClientMock.cpp:87
> String label() const { return m_label; }
Return type should probably be const String&.
> Source/WebCore/platform/mock/MediaStreamClientMock.cpp:169
> + , m_peerConnectionId(-1)
Be careful of never using an id with the value 0 in a hash map (I'm not saying it's happening).
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list