Created attachment 160992 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 160992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160992&action=review > Source/WebCore/bindings/v8/Dictionary.cpp:514 > + String stringKey = toWebCoreString(key); > + names.append(stringKey); I'd combine these two lines.

I need to read this again. MediaConstraintsImpl is an interesting approach.

(In reply to comment #4) > I need to read this again. MediaConstraintsImpl is an interesting approach. Interesting is such a ambivalent word ;) I did it like this since I wanted to keep a Dictionary in an object that will be sent through the WebKit interface. Also a better description of the MediaConstraints objects is in order, it's not trivial. A constraint is a key:value pair, and example is minVideoWidth:1024. The constraint can be either mandator or optional. MediaConstraints contains two members, mandatory and optional, whose structures are not the same. "mandatory" is unordered and therefore is a simple sub-Dictionary. If a web developer specifies a mandatory constraint which isn't supported by the user agent an exception should be thrown. "optional" is an ordered list of key:value pairs and each constraint should be in its own Dictionary. { mandatory: { minVideoWidth:640 }, optional: { [ { minVideoWidth:1024 }, { magic:true }, { awesomeness:"super" } ] } }

Created attachment 161032 [details] Patch

Comment on attachment 160992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160992&action=review >> Source/WebCore/bindings/v8/Dictionary.cpp:514 >> + names.append(stringKey); > > I'd combine these two lines. Done.

> Interesting is such a ambivalent word ;) I didn't mean to be ambivalent. I just meant that I need to look at it in more detail to understand.

Comment on attachment 161032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161032&action=review > Source/WebCore/Modules/mediastream/MediaConstraintsImpl.h:63 > + Dictionary m_mandatoryConstraints; I don't think you can hold a Dictionary are a member variable. I'll explain why in a moment. > Source/WebCore/bindings/js/Dictionary.cpp:88 > + ExecState* exec = m_dictionary.execState(); The problem is that you can't hold an ExecState* as a member. The ExecState's lifetime is limited to the call stack. > Source/WebCore/platform/chromium/support/WebMediaConstraints.cpp:67 > + m_private->getConstraintNames(mandatory, constraintNames); As you've written things, this function requires a executing JavaScript, but given that this is a public API, there's no reason to believe that we're inside a JavaScript context (or even have a HandleScope on the stack). > Source/WebCore/platform/mediastream/MediaConstraints.h:41 > +class MediaConstraints : public RefCounted<MediaConstraints> { I don't understand why we need the abstract base class. There's only one implementation of this class that I can see.

> Also a better description of the MediaConstraints objects is in order, it's not trivial. I see. Maybe it would be better to represent this state explicitly with C++ types rather than leaving it in JavaScript objects? Presumably we need to copy it out of the JavaScript VM at some point. Perhaps we're trying to save a copy by copying it directly into the embedder? The problem is that Dictionary object can't really live past the stack frame in which they are created. They hold onto a bunch of ephemeral state that explodes as you unwind the stack. Maybe you've set things up so that MediaConstraints are only ever used in their own stack frame? I don't see anything that prevents the embedder from holding onto a WebMediaConstraints object. Even if this work today, it's a pretty fragile API. Is there a strong reason not to just copy the data out of JS?

If you're worried that these things are variants and C++ doesn't have a good way of representing variants, we can use InspectorValue. For example, we can use v8ToInspectorValue or ScriptValue::toInspectorValue

Finally, I don't get why we need to separate MediaConstraints from MediaConstraintsImpl. What does that buy us?

Comment on attachment 161032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161032&action=review >> Source/WebCore/platform/mediastream/MediaConstraints.h:41 >> +class MediaConstraints : public RefCounted<MediaConstraints> { > > I don't understand why we need the abstract base class. There's only one implementation of this class that I can see. It's this platform abstraction again... The abstract base class defines the interface for the object that will go from WebCore/platform to WebKit, whereas the implementation is then free to use WebCore proper objects and APIs.

(In reply to comment #10) > > Also a better description of the MediaConstraints objects is in order, it's not trivial. > > I see. Maybe it would be better to represent this state explicitly with C++ types rather than leaving it in JavaScript objects? Presumably we need to copy it out of the JavaScript VM at some point. Perhaps we're trying to save a copy by copying it directly into the embedder? > > The problem is that Dictionary object can't really live past the stack frame in which they are created. They hold onto a bunch of ephemeral state that explodes as you unwind the stack. Maybe you've set things up so that MediaConstraints are only ever used in their own stack frame? > > I don't see anything that prevents the embedder from holding onto a WebMediaConstraints object. Even if this work today, it's a pretty fragile API. Right, MediaConstraints are only supposed to be used in the context of the call but as you rightly point out the API doesn't stop holding on to the object. > > Is there a strong reason not to just copy the data out of JS? Here's the reason I did it this way: I can create a C++ class but I wanted to push the responsibility for knowing about valid constraints and their types to the user agent, otherwise both WebKit and the UA have to be patched as soon as a new constraint is added. This way only the UA is affected. Another alternative is for webkit to extract everything as strings and let the UA convert to the right type as needed but it adds some extra overhead. No big deal though, and converting to InspectorValues will do the same, so maybe this is a better approach. It is certainly much safer.

No abstraction is required. WebKit can use WebCore types directly.

I'd go with the safer approach. We can optimize later if this is a bottleneck.

Created attachment 161494 [details] Patch

(In reply to comment #16) > I'd go with the safer approach. We can optimize later if this is a bottleneck. Done.

Comment on attachment 161494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161494&action=review This looks good, but we still have the extra MediaConstraints class. > Source/WebCore/bindings/js/Dictionary.h:64 > + bool getOwnPropertyNames(WTF::Vector<String>&) const; WTF::Vector -> Vector

Comment on attachment 161494 [details] Patch Attachment 161494 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13691754

(In reply to comment #15) > No abstraction is required. WebKit can use WebCore types directly. The entire MediaStream feature is built upon these rules: The WebKit API only sees and uses WebCore/platform classes (WebMediaConstraints) WebCore/platform classes doesn't use anything from WebCore proper (MediaConstraints) WebCore proper uses webCore/platform classes (MediaConstraintsImpl) Previously this has been solved by the *Descriptor pattern, whereas here I thought inheritance would work better. So if I understand you correctly you would prefer if MediaConstraints in WebCore/platform directly uses the Dictionary class. I would prefer not so that the WebCore/platform pulls in as little as possible.

Hmm, I don't think the mac build problem is due to my code...

Ah! I misunderstood what you wrote in Comment #13. Now that we're not keeping the Dictionary around, do you think it would be more consistent with the rest of the feature to copy the data into a Descriptor object in the platform layer rather than into the DOM object?

> Hmm, I don't think the mac build problem is due to my code... Yeah, looks like xcode just crashed for some reason.

(In reply to comment #23) > Ah! I misunderstood what you wrote in Comment #13. No problems. > > Now that we're not keeping the Dictionary around, do you think it would be more consistent with the rest of the feature to copy the data into a Descriptor object in the platform layer rather than into the DOM object? If you don't scream I would like to keep it like this, and even use it for two more classes (to be reviewed soon).

Created attachment 161505 [details] Patch

Comment on attachment 161494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161494&action=review >> Source/WebCore/bindings/js/Dictionary.h:64 >> + bool getOwnPropertyNames(WTF::Vector<String>&) const; > > WTF::Vector -> Vector Fixed.

Comment on attachment 161505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161505&action=review Ok. If you think this pattern is better in some cases, we can give it a try. Thanks for being patient with me. :) > Source/WebCore/bindings/v8/Dictionary.cpp:497 > +bool Dictionary::getOwnPropertyNames(WTF::Vector<String>& names) const Technically this doesn't need a WTF either. The WTF headers tend to have "using" statements that import these names.

Comment on attachment 161505 [details] Patch Clearing flags on attachment: 161505 Committed r127165: <http://trac.webkit.org/changeset/127165>

All reviewed patches have been landed. Closing bug.