Closed Bug 926289 Opened 7 years ago Closed 7 years ago

Add app/page info in recording-status event

Categories

(Core :: WebRTC, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
1.2 C3(Oct25)
blocking-b2g koi+
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)

13.90 KB, patch
schien
: review+
Details | Diff | Splinter Review
4.92 KB, patch
vingtetun
: review+
jesup
: review+
Details | Diff | Splinter Review
11.47 KB, patch
schien
: review+
Details | Diff | Splinter Review
6.69 KB, patch
schien
: review+
Details | Diff | Splinter Review
13.04 KB, patch
Details | Diff | Splinter Review
11.47 KB, patch
Details | Diff | Splinter Review
6.69 KB, patch
Details | Diff | Splinter Review
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+
Duplicate of this bug: 928206
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.
Flags: needinfo?(schien)
patch for m-a is ready.
You need to log in before you can comment on or make changes to this bug.