Bug 97095 - MediaStream API: Extend UserMediaRequest with a ownerDocument method
Summary: MediaStream API: Extend UserMediaRequest with a ownerDocument method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-09-19 04:45 PDT by Tommy Widenflycht
Modified: 2012-09-20 11:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.82 KB, patch)
2012-09-19 05:01 PDT, Tommy Widenflycht
abarth: review-
Details | Formatted Diff | Diff
Patch (8.90 KB, patch)
2012-09-20 02:47 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-09-19 04:45:18 PDT
Chromium need to know exactly which frame called getUserMedia so that it can clean away the stream when the frame goes away. Since that information is available in webkit add an accessor method.
Comment 1 Tommy Widenflycht 2012-09-19 05:01:26 PDT
Created attachment 164717 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-19 05:07:54 PDT
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 3 Adam Barth 2012-09-19 11:41:35 PDT
Comment on attachment 164717 [details]
Patch

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:77
> +    return ((Document*)m_scriptExecutionContext)->frame();

We use C++ style casts:

static_cast<Document*>(m_scriptExecutionContext)->frame()
Comment 4 Adam Barth 2012-09-19 11:43:51 PDT
Comment on attachment 164717 [details]
Patch

This is fine, but it should return the associated Document rather than the Frame.  DOM-related objects are associated with Documents (the "D" in DOM stands for Document).  View-related objects are associated with Frames.

WebDocument has a frame() function, so the embedder can find the Frame if it needs that for its own purposes.
Comment 5 Adam Barth 2012-09-19 11:44:56 PDT
Also "calling" isn't the right name.  That's a scripting related concept and there's no reason these objects are necessarily products of script calls.  Maybe the term "ownerDocument" is better?
Comment 6 Adam Barth 2012-09-19 11:45:35 PDT
Also, keep in mind that m_scriptExecutionContext can be 0 since this object doesn't keep the Document alive.
Comment 7 Tommy Widenflycht 2012-09-20 02:47:15 PDT
Created attachment 164871 [details]
Patch
Comment 8 Tommy Widenflycht 2012-09-20 02:47:52 PDT
Comment on attachment 164717 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:77
>> +    return ((Document*)m_scriptExecutionContext)->frame();
> 
> We use C++ style casts:
> 
> static_cast<Document*>(m_scriptExecutionContext)->frame()

Fixed.
Comment 9 Tommy Widenflycht 2012-09-20 02:48:24 PDT
(In reply to comment #4)
> (From update of attachment 164717 [details])
> This is fine, but it should return the associated Document rather than the Frame.  DOM-related objects are associated with Documents (the "D" in DOM stands for Document).  View-related objects are associated with Frames.
> 
> WebDocument has a frame() function, so the embedder can find the Frame if it needs that for its own purposes.

Much nicer, thanks!
Comment 10 Tommy Widenflycht 2012-09-20 02:49:56 PDT
(In reply to comment #5)
> Also "calling" isn't the right name.  That's a scripting related concept and there's no reason these objects are necessarily products of script calls.  Maybe the term "ownerDocument" is better?

I'm not arguing with you but these calls are only called as a direct response to a JS call. However that is ownerDocument is much better.
Comment 11 Tommy Widenflycht 2012-09-20 02:50:11 PDT
(In reply to comment #6)
> Also, keep in mind that m_scriptExecutionContext can be 0 since this object doesn't keep the Document alive.

Fixed.
Comment 12 Adam Barth 2012-09-20 11:04:25 PDT
Comment on attachment 164871 [details]
Patch

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

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:80
> +    if (m_scriptExecutionContext) {
> +        ASSERT(m_scriptExecutionContext->isDocument());
> +        return static_cast<Document*>(m_scriptExecutionContext);
> +    }
> +
> +    return 0;

The other way you could have done this, by the way, is:
ASSERT(isMainThread());
return static_cast<Document*>(m_scriptExecutionContext);

But this is fine too.
Comment 13 WebKit Review Bot 2012-09-20 11:22:52 PDT
Comment on attachment 164871 [details]
Patch

Clearing flags on attachment: 164871

Committed r129145: <http://trac.webkit.org/changeset/129145>
Comment 14 WebKit Review Bot 2012-09-20 11:22:56 PDT
All reviewed patches have been landed.  Closing bug.