[webkit-reviews] review requested: [Bug 70897] Use a simple page client/controller for user consent in getUserMedia() : [Attachment 114476] Patch 3

[bugzilla-daemon at webkit.org]

Adam Bergkvist <adam.bergkvist at ericsson.com> has asked	for review:
Bug 70897: Use a simple page client/controller for user consent in
getUserMedia()
https://bugs.webkit.org/show_bug.cgi?id=70897

Attachment 114476: Patch 3
https://bugs.webkit.org/attachment.cgi?id=114476&action=review

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>
(In reply to comment #11)
> (From update of attachment 114178 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=114178&action=review
> 
> Below are a few nits.
> 
> > Source/WebCore/mediastream/UserMediaController.cpp:90
> > +	 DOMWindow* domWindow = request->frame()->existingDOMWindow();
> > +	 if (!domWindow)
> > +	     return;
> > +
> > +	 ScriptExecutionContext* context = domWindow->scriptExecutionContext();

> > +	 if (!context || context != request->scriptExecutionContext())
> > +	     return;
> 
> You should just get the ScriptExecutionContext from the frame without
involving the DOMWindow:
> 
> ScriptExecutionContext* context = request->frame()->document();

Fixed.
 
> > Source/WebCore/mediastream/UserMediaRequest.cpp:57
> > +	 ,
m_scriptExecutionContext(frame->existingDOMWindow()->scriptExecutionContext())
> 
> Please don't go through the DOMWindow.  frame->document() should work just
find here.

Fixed.

> > Source/WebCore/mediastream/UserMediaRequest.cpp:78
> > +	 for (size_t i = 0; i < optionsList.size(); i++) {
> 
> ++i

Fixed (Perhaps this should be added to the style guidelines?)

> > Source/WebCore/mediastream/UserMediaRequest.cpp:80
> > +	     optionsList[i].split(" ", subOptionsList);
> 
> Are you sure this is right?  Usually these parsers use isHTMLSpace.  Can you
put a link the spec in the code?

You're right, the split was a bit naive. I use SpaceSplitString now.

> > Source/WebCore/mediastream/UserMediaRequest.cpp:82
> > +	     if (subOptionsList[0] == "audio")
> 
> This is really supposed to be case sensitive?

Yes. 

"2. If the first token in list of suboptions is a case-sensitive match for the
string "audio", let audio be true."

> > Source/WebCore/mediastream/UserMediaRequest.cpp:99
> > +	 m_frame = 0;
> 
> Do we need to zero out m_controller too?

The controller is zeroed out in the call to cancelUserMediaRequset() on line
before.

> > Source/WebCore/mediastream/UserMediaRequest.h:82
> > +	 ScriptExecutionContext* m_scriptExecutionContext;
> 
> Does this need to be a RefPtr?

We only use this member to compare the pointer value with the context at the
time a request succeeds or fails. 

(In reply to comment #13)
> (From update of attachment 114178 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=114178&action=review
> 
> > Source/WebCore/mediastream/UserMediaController.cpp:53
> > +	     m_client->userMediaControllerDestroyed();
> 
> At this point have all outstanding requests been cancelled? Would be best, I
think, to do so.

They should be canceled at this point.

> >> Source/WebCore/mediastream/UserMediaRequest.cpp:78
> >> +	  for (size_t i = 0; i < optionsList.size(); i++) {
> > 
> > ++i
> 
> FYI: This means that you should change your i++ to ++i. A prefix operator is
generally much better than a postfix one.

I got that, but I was just wondering what the motivation was (it's a pure style
issue in this particular case). I haven't got this comment before and there's
nothing about it in the style guidelines (perhaps it should be added?). It's
anyhow fixed now.

(In reply to comment #19)
> 1) a JS function calls GetUserMedia which forwards the call to
UserMediaController
> 2) The UserMediaController enumerates the available audio/video devices
through the Platform interface. The devices are at this stage unopened.
> 3) The UserMediaController then sends the list for selection and approval
from the user through the Client interface.
> 4) The user selects say a camera and a audio input device and thus approves
the use.
> 
> Have I gotten it right this time? I have been confused by the wording Query
which for me sounds like "asking the user" and not "enumerating all devices".

Yes, that's about it.