[Webkit-unassigned] [Bug 70897] Use a simple page client/controller for user consent in getUserMedia()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70897


Adam Bergkvist <adam.bergkvist at ericsson.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114178|0                           |1
        is obsolete|                            |
 Attachment #114178|review?                     |
               Flag|                            |
 Attachment #114476|                            |review?
               Flag|                            |




--- Comment #20 from Adam Bergkvist <adam.bergkvist at ericsson.com>  2011-11-10 05:18:21 PST ---
Created an attachment (id=114476)
 --> (https://bugs.webkit.org/attachment.cgi?id=114476&action=review)
Patch 3

(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.