Closed Bug 926289 Opened 12 years ago Closed 12 years ago

Add app/page info in recording-status event

Categories

(Core :: WebRTC, defect)

Product:
Core
Component:
WebRTC
Version:
26 Branch
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
1.2 C3(Oct25)
Project Flags:
blocking-b2g koi+
Tracking Flags:
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: alive, Assigned: schien)

References

Details

(Whiteboard: [qa-])

Attachments

(8 files, 9 obsolete files)

Part 1 - add recording type and request URL in recording-device-events
13.90 KB, patch
schien
: review+
Details | Diff | Splinter Review
Part 4 - Separate recording status for each process
4.92 KB, patch
vingtetun
: review+
jesup
: review+
Details | Diff | Splinter Review
Part 2 - Carry recording type and request URL in recording-device-events in gUM API
11.47 KB, patch
schien
: review+
Details | Diff | Splinter Review
Part 3 - Carry recording type and request URL in recording-device-events in Camera API
6.69 KB, patch
schien
: review+
Details | Diff | Splinter Review
[m-a]Part 1 - add recording type and request URL in recording-device-events
13.04 KB, patch
Details | Diff | Splinter Review
[m-a]Part 2 - Carry recording type and request URL in recording-device-events in gUM API
11.47 KB, patch
Details | Diff | Splinter Review
[m-a]Part 3 - Carry recording type and request URL in recording-device-events in Camera API
6.69 KB, patch
Details | Diff | Splinter Review
[m-a]Part 4 - Separate recording status for each process
5.03 KB, patch
Details | Diff | Splinter Review
See bug 917367, we need to know who is having gUM to have a notification in utility tray. manifestURL + pageURL wanted in evt.detail.
Component: General → WebRTC
Product: Boot2Gecko → Core
Version: unspecified → 26 Branch
Blocks a blocker, so blocking on this automatically.
blocking-b2g: --- → koi+
Here is the list of all changes, 1. move RecordingDeviceStatus from PContent to PBrowser because we need page URL if any page in browser app is requesting for GetUserMedia 2. carry page URL or manifest URL to Gaia for displaying webpage / app that doing the recording. 3. carry audio/video info from MediaManager/DOMCameraControl to Gaia for displaying corresponding type of recording in notification window.
Attachment #819532 - Flags: feedback?(rjesup)
Attachment #819532 - Flags: feedback?(khuey)
Attachment #819532 - Flags: feedback?(21)
Comment on attachment 819532 [details] [diff] [review] WIP - carry-app-page-url-in-chrome-event.patch Review of attachment 819532 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsFrameLoader.h @@ +309,5 @@ > * ensures that we can only add restrictions and never remove them. > */ > void ApplySandboxFlags(uint32_t sandboxFlags); > > + NS_HIDDEN_(void) GetURL(nsString& aURL); While you're here, drop NS_HIDDEN_(). It hasn't done anything useful in the better part of a decade. ::: dom/ipc/TabParent.cpp @@ +1634,5 @@ > + props->SetPropertyAsBool(NS_LITERAL_STRING("isApp"), mManager->IsForApp()); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isForAudio"), aIsForAudio); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isForVideo"), aIsForVideo); > + > + nsAutoString requestURL; Just nsString here. nsAuto[C]String allocates stack storage for a string, but if you're just going to call a getter to get a copy of an existing string that's wasted effort.
Attachment #819532 - Flags: feedback?(khuey) → feedback+
Comment on attachment 819532 [details] [diff] [review] WIP - carry-app-page-url-in-chrome-event.patch Review of attachment 819532 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +1251,5 @@ > let oldCount = gRecordingActiveCount; > > let processId = (!aSubject) ? 'main' > : aSubject.QueryInterface(Ci.nsIPropertyBag2).get('childID'); > + // XXX aSubject might be null I assume this is part of the WIP aspect of this bug ::: dom/ipc/TabParent.cpp @@ +1623,5 @@ > > +bool > +TabParent::RecvRecordingDeviceEvents(const nsString& aRecordingStatus, > + const bool& aIsForAudio, > + const bool& aIsForVideo) Same as MediaManager: aIsVideo/etc ::: dom/media/MediaManager.h @@ +206,5 @@ > GetUserMediaNotificationEvent(GetUserMediaCallbackMediaStreamListener* aListener, > + GetUserMediaStatus aStatus, > + bool aIsForAudio, bool aIsForVideo, uint64_t aWindowID) > + : mListener(aListener) , mStatus(aStatus) , mIsForAudio(aIsForAudio) > + , mIsForVideo(aIsForVideo), mWindowID(aWindowID) {} aIsVideo (etc) instead
Attachment #819532 - Flags: feedback?(rjesup) → feedback+
Comment on attachment 819532 [details] [diff] [review] WIP - carry-app-page-url-in-chrome-event.patch Review of attachment 819532 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +1255,5 @@ > + // XXX aSubject might be null > + let requestURL = aSubject.get('requestURL'); > + let isApp = aSubject.get('isApp'); > + let isForAudio = aSubject.get('isForAudio'); > + let isForVideo = aSubject.get('isForVideo'); You don't really need those variables if you will not send any mozChromeEvent to the system app. Let's declare them in: if ((oldCount === 0 && gRecordingActiveCount > 0) || (gRecordingActiveCount === 0 && oldCount > 0)) { (gRecordingActiveCount === 0 && oldCount > 0)) { ... } Or even better don't use intermediate variable and assign directly: { ... requestURL: aSubject.get('requestURL'), isApp: aSubject.get('isApp'), isAudio: aSubject.get('isForAudio'), isVideo: aSubect.get('isForVideo'), ... } but maybe you will need to have intermediate variable since aSubject can be null.
Attachment #819532 - Flags: feedback?(21) → feedback+
1. Move RecordingDeviceEvents from PContent to PBrowser 2. Expose page URL from nsFrameLoader 3. Expose app manifest URL from nsDocShell 4. Add recording type and request URL in recording-device-events
Attachment #819532 - Attachment is obsolete: true
Attachment #820273 - Flags: review?(khuey)
Add recording type and request URL in recording-device-events.
Attachment #820275 - Flags: review?(rjesup)
Add recording type and request URL in recording-device-events.
Attachment #820278 - Flags: review?(mhabicher)
1. Add recording type and request URL in mozChromeEvent. 2. Seperate recording-status report for each process. 3. Fix unexpected recording-device-ipc-events for bug 928206.
Attachment #820280 - Flags: review?(21)
Comment on attachment 820278 [details] [diff] [review] Part 3 - Carry recording type and request URL in recording-device-events in Camera API Review of attachment 820278 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. r+ with the issues below addressed. ::: dom/camera/DOMCameraControl.cpp @@ +282,5 @@ > } > > + bool isApp; > + nsAutoString requestURL; > + nsresult rv = mWindow->GetDocShell()->GetIsApp(&isApp); Please break up this chained derefence so that we can catch nullptrs; e.g.: MOZ_ASSERT(mWindow); nsCOMPtr<nsIDocShell> docShell = mWindow->GetDocShell(); MOZ_ASSERT(docShell); nsresult rv = docShell->GetIsApp(&isApp); @@ +286,5 @@ > + nsresult rv = mWindow->GetDocShell()->GetIsApp(&isApp); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + > + if (isApp) { > + rv = mWindow->GetDocShell()->GetAppManifestURL(requestURL); rv = docShell->GetAppManifestURL(requestURL); @@ +290,5 @@ > + rv = mWindow->GetDocShell()->GetAppManifestURL(requestURL); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + } else { > + nsCString pageURL; > + rv = mWindow->GetDocumentURI()->GetSpec(pageURL); Please break this one up too. @@ +310,5 @@ > // The events are gathered in chrome process and used for recording indicator > if (XRE_GetProcessType() != GeckoProcessType_Default) { > + unused << TabChild::GetFrom(mWindow)->SendRecordingDeviceEvents(NS_LITERAL_STRING("starting"), > + true, > + true); Please add a comment to document these booleans, e.g.: true /* isAudio */, true /* isVideo */); @@ +364,5 @@ > + nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag(); > + props->SetPropertyAsAString(NS_LITERAL_STRING("requestURL"), requestURL); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isApp"), isApp); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isAudio"), true); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isVideo"), true); The entire above block looks like a duplicate of the code in StartRecording(). Please refactor this into its own function.
Attachment #820278 - Flags: review?(mhabicher) → review+
Comment on attachment 820275 [details] [diff] [review] Part 2 - Carry recording type and request URL in recording-device-events in gUM API Review of attachment 820275 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaManager.cpp @@ +1797,5 @@ > + nsresult rv = window->GetDocShell()->GetIsApp(&isApp); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (isApp) { > + rv = window->GetDocShell()->GetAppManifestURL(requestURL); See mikeh's comments on his patch @@ +1801,5 @@ > + rv = window->GetDocShell()->GetAppManifestURL(requestURL); > + NS_ENSURE_SUCCESS(rv, rv); > + } else { > + nsCString pageURL; > + nsresult rv = window->GetDocumentURI()->GetSpec(pageURL); ditto @@ +1811,5 @@ > + nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag(); > + props->SetPropertyAsAString(NS_LITERAL_STRING("requestURL"), requestURL); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isApp"), isApp); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isAudio"), mIsAudio); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isVideo"), mIsVideo); ditto @@ +1813,5 @@ > + props->SetPropertyAsBool(NS_LITERAL_STRING("isApp"), isApp); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isAudio"), mIsAudio); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isVideo"), mIsVideo); > + > + obs->NotifyObservers((nsIPropertyBag2*)props, space before props
Attachment #820275 - Flags: review?(rjesup) → review+
Does the patch you attached for r? is really for me? I'm not confident to r? all this cpp code :)
Flags: needinfo?(schien)
Comment on attachment 820273 [details] [diff] [review] Part 1 - add recording type and request URL in recording-device-events Review of attachment 820273 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: docshell/base/nsDocShell.cpp @@ +4586,5 @@ > errorPageUrl.AppendLiteral("&d="); > errorPageUrl.AppendASCII(escapedDescription.get()); > > // Append the manifest URL if the error comes from an app. > + nsAutoString manifestURL; I know you just moved this code around, but this should also be a regular nsString. ::: dom/ipc/ContentParent.h @@ +163,5 @@ > */ > void KillHard(); > > uint64_t ChildID() { return mChildID; } > + nsString AppManifestURL() { return mAppManifestURL; } return a 'const nsString&'. I think that will still compile. Really this function should be marked const too, but ContentParent doesn't really try to be const-correct. ::: dom/ipc/TabParent.cpp @@ +1634,5 @@ > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + // recording-device-ipc-events needs to gather more information from content process > + nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag(); > + props->SetPropertyAsUint64(NS_LITERAL_STRING("childID"), mManager->ChildID()); Manager()-> @@ +1635,5 @@ > + if (obs) { > + // recording-device-ipc-events needs to gather more information from content process > + nsRefPtr<nsHashPropertyBag> props = new nsHashPropertyBag(); > + props->SetPropertyAsUint64(NS_LITERAL_STRING("childID"), mManager->ChildID()); > + props->SetPropertyAsBool(NS_LITERAL_STRING("isApp"), mManager->IsForApp()); here too @@ +1641,5 @@ > + props->SetPropertyAsBool(NS_LITERAL_STRING("isVideo"), aIsVideo); > + > + nsString requestURL; > + if (mManager->IsForApp()) { > + requestURL = mManager->AppManifestURL(); and here too
Attachment #820273 - Flags: review?(khuey) → review+
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13) > Does the patch you attached for r? is really for me? I'm not confident to r? > all this cpp code :) I can include @jesup for reviewing the logic of processing recording-device-events, so that you can focus on shell.js coding convention. Is it ok for you?
Flags: needinfo?(schien) → needinfo?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13) > Does the patch you attached for r? is really for me? I'm not confident to r? > all this cpp code :) I can include @jesup for reviewing the logic of processing recording-device-events, so that you can focus on shell.js coding convention. Is it ok for you?
Target Milestone: --- → 1.2 C3(Oct25)
update according to review comment, carry r+
Attachment #820273 - Attachment is obsolete: true
Attachment #820855 - Flags: review+
update according to review comment, carry r+
Attachment #820275 - Attachment is obsolete: true
Attachment #820856 - Flags: review+
update according to review comment, carry r+
Attachment #820278 - Attachment is obsolete: true
Attachment #820858 - Flags: review+
Comment on attachment 820280 [details] [diff] [review] Part 4 - Separate recording status for each process Review of attachment 820280 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Shih-Chiang Chien [:schien] from comment #16) > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13) > > Does the patch you attached for r? is really for me? I'm not confident to r? > > all this cpp code :) > > I can include @jesup for reviewing the logic of processing > recording-device-events, so that you can focus on shell.js coding > convention. Is it ok for you? Sounds good for me but the patch you asked me to r? does not contains any shell.js changes :)
Attachment #820280 - Flags: review?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #20) > Comment on attachment 820280 [details] [diff] [review] > Part 4 - Separate recording status for each process > > Review of attachment 820280 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Shih-Chiang Chien [:schien] from comment #16) > > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13) > > > Does the patch you attached for r? is really for me? I'm not confident to r? > > > all this cpp code :) > > > > I can include @jesup for reviewing the logic of processing > > recording-device-events, so that you can focus on shell.js coding > > convention. Is it ok for you? > > Sounds good for me but the patch you asked me to r? does not contains any > shell.js changes :) Sorry my bad, upload the correct patch file.
Update the correct patch for shell.js and add @jesup to reviewer.
Attachment #820280 - Attachment is obsolete: true
Attachment #820947 - Flags: review?(rjesup)
Attachment #820947 - Flags: review?(21)
Flags: needinfo?(21)
Did you want me to review shell.js as well? I think so, but it's not clear - if so which parts?
(In reply to Randell Jesup [:jesup] from comment #23) > Did you want me to review shell.js as well? I think so, but it's not clear > - if so which parts? Hi @jseup, I would like you to check the logic of maintaining a recording status table for each process, to see if any mismatched usage of recording-device-events in shell.js.
Update patch to clean up a build error for unused variable "nsresult rv", carry r+.
Attachment #820856 - Attachment is obsolete: true
Attachment #821405 - Flags: review+
Update patch to clean up build error for unused variable "nsresult rv", carry r+.
Attachment #820858 - Attachment is obsolete: true
Attachment #821406 - Flags: review+
Attachment #820947 - Flags: review?(rjesup) → review+
window->GetDocShell() will return nullptr while exiting Firefox during recording. Update CreateRecordingDeviceEventsSubject() and make it not assert under this scenario. carry r+.
Attachment #821405 - Attachment is obsolete: true
Attachment #822374 - Flags: review+
window->GetDocShell() will return nullptr while exiting Firefox during recording. Update CreateRecordingDeviceEventsSubject() and make it not assert under this scenario.
Attachment #821406 - Attachment is obsolete: true
Attachment #822376 - Flags: review+
Whiteboard: [qa-]
This has numerous conflicts due to Aurora not having bug 882186 and others. Please post branch-specific patches for v1.2.
status-b2g-v1.2: --- → affected
status-firefox25: --- → wontfix
status-firefox26: --- → affected
status-firefox27: --- → fixed
Flags: needinfo?(schien)
Keywords: branch-patch-needed
patch for m-a is ready.
Keywords: branch-patch-needed → checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: