WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
97095
MediaStream API: Extend UserMediaRequest with a ownerDocument method
https://bugs.webkit.org/show_bug.cgi?id=97095
Summary
MediaStream API: Extend UserMediaRequest with a ownerDocument method
Tommy Widenflycht
Reported
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.
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
View All
Add attachment
proposed patch, testcase, etc.
Tommy Widenflycht
Comment 1
2012-09-19 05:01:26 PDT
Created
attachment 164717
[details]
Patch
WebKit Review Bot
Comment 2
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
.
Adam Barth
Comment 3
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()
Adam Barth
Comment 4
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.
Adam Barth
Comment 5
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?
Adam Barth
Comment 6
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.
Tommy Widenflycht
Comment 7
2012-09-20 02:47:15 PDT
Created
attachment 164871
[details]
Patch
Tommy Widenflycht
Comment 8
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.
Tommy Widenflycht
Comment 9
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!
Tommy Widenflycht
Comment 10
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.
Tommy Widenflycht
Comment 11
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.
Adam Barth
Comment 12
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.
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2012-09-20 11:22:56 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug