898949 - [B2G getUserMedia] Display front/back camera list on permission prompt

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Alfredo Yang (:alfredo)]

ContentPermissionPrompt service needs to add function to display front/back camera list.

Add protocol on PContentPermissionRequest.ipdl to display device list on b2g process and renturn the user choice to content tab.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

The current nsIContentPermissionRequest interface only have type information and allow/cancel operations. For a displaying device list, we will need add choice information and choose operation in nsIContentPermissionRequest.

Comment 2

[Jason Smith [:jsmith]]

This isn't a blocker for release. We only need to support gUM audio for 1.2 and we're past feature complete.

Comment 3

[Jason Smith [:jsmith]]

This will be needed however for 1.3, as 1.3 is intended to support full webrtc support.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - add optios in permission prompt for Video/Audio device selection.

Support permission options in permission request.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - add available devices in the options of media permission request

Add available device in the options of media permission request.

Comment 6

[Maire Reavy [:mreavy]]

We should make this 1.3+

Comment 7

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #6)
> We should make this 1.3+

Agreed.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 823946 [details] [diff] [review]
Part 2 - add available devices in the options of media permission request

Review of attachment 823946 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but adding review by jib to look at the JS/rooting stuff that I know less about (and I'm have a very bad cold, so I'm generally fuzzy and would welcome a cross-check)

Comment 9

[Jason Smith [:jsmith]]

(In reply to Shih-Chiang Chien [:schien] from comment #4)
> Created attachment 823945 [details] [diff] [review]
> Part 1 - add optios in permission prompt for Video/Audio device selection.
> 
> Support permission options in permission request.

See https://bugzilla.mozilla.org/show_bug.cgi?id=919927#c3 - The changes being suggested here are going to completely break compatibility with existing apps across the board. Using a list here doesn't make sense here to indicate front vs. back - we should be using an attribute of the permission to figure this out. Something like:

"permissions": {
   "video-capture": {
     "description": "perm description",
     "access": "front"
   }
}

Comment 10

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #9)
> (In reply to Shih-Chiang Chien [:schien] from comment #4)
> > Created attachment 823945 [details] [diff] [review]
> > Part 1 - add optios in permission prompt for Video/Audio device selection.
> > 
> > Support permission options in permission request.
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=919927#c3 - The changes
> being suggested here are going to completely break compatibility with
> existing apps across the board. Using a list here doesn't make sense here to
> indicate front vs. back - we should be using an attribute of the permission
> to figure this out. Something like:
> 
> "permissions": {
>    "video-capture": {
>      "description": "perm description",
>      "access": "front"
>    }
> }

Ah, made a misinterpretation - the discussion on the blocking bug is talking about mozChromeEvents, not the webapp manifest. Disregard my comment.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 823946 [details] [diff] [review]
Part 2 - add available devices in the options of media permission request

Review of attachment 823946 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't find anything wrong with the code, but see recommendation.

::: dom/media/MediaPermissionGonk.cpp
@@ +269,4 @@
>  {
> +
> +  // check if undefined or null
> +  if (aChoices.isUndefined() || aChoices.isNull()) {

.isUndefined() seems redundant here since you check .isObject() below. Is the difference in assert message important? If not, I would fold this with the test below.

@@ +281,5 @@
> +  // iterate through audio-capture and video-capture
> +  AutoSafeJSContext cx;
> +  JS::Rooted<JSObject*> obj(cx, &aChoices.toObject());
> +  JSAutoCompartment ac(cx, obj);
> +  JS::Rooted<JS::Value> v(cx);

I'll forward the advice I got when attempting manual JS object traversal like this (in bug 825703 comment 14): Please use JS API as little as possible.

I see you already have dictionaries like PermissionChoice defined in the Part 1 patch, so in obj/js/xpconnect/src/DictionaryHelpers.h|.cpp you should find generated PermissionChoice::Init() code that should initialize a c++ representation of a jsval for you, I believe. Using that is regarded to be safer.

Disclaimer: I used a webidl dictionary when I did this, but from my quick inspection, xpidl appears to generate similar stuff, so hopefully that works. Feel free to look at my patch in bug 825703 for an example.

@@ +286,5 @@
> +
> +  // get selected audio device name
> +  nsString audioDevice;
> +  if (mAudio) {
> +    if (!JS_GetProperty(cx, JSVAL_TO_OBJECT(aChoices),

You already have obj you can use here.

@@ +364,5 @@
> +      const nsCString &choice = choices[i].choice();
> +      if (choices[i].type().EqualsLiteral(AUDIO_PERMISSION_NAME)) {
> +        audioDevice = NS_ConvertUTF8toUTF16(choice);
> +      } else if (choices[i].type().EqualsLiteral(VIDEO_PERMISSION_NAME)) {
> +        videoDevice = NS_ConvertUTF8toUTF16(choice);

I see a lot of back-and-forth conversion in this patch. Is defining everything in UTF-16 an option? (It may not be, I just wanted to mention it as I can't tell from the scope whether anything truly needs to be in UTF-8 or not).

Comment 12

[Ivan Tsay (:ITsay)]

Plus this one for v1.3 to complete the gUM permission on video part.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 823945 [details] [diff] [review]
Part 1 - add optios in permission prompt for Video/Audio device selection.

Review of attachment 823945 [details] [diff] [review]:
-----------------------------------------------------------------

I think you should make ContentPermissionType::mOptions an nsTArray<nsString>.  Then you could avoid a lot of converting back and forth between UTF-8/16.  You can handle making that change without my rereview ;-)

I didn't look at the JS file.

::: dom/base/nsContentPermissionHelper.cpp
@@ +92,5 @@
>  NS_IMPL_ISUPPORTS1(ContentPermissionType, nsIContentPermissionType)
>  
>  ContentPermissionType::ContentPermissionType(const nsACString& aType,
> +                                             const nsACString& aAccess,
> +                                             const nsTArray<nsCString>& aOptions)

Is it possible to make aOptions non-const, and then do mOptions.SwapElements(aOptions)?  I didn't look at the other patch, but if you're using arrays you get out of IPDL it might not be.

@@ +321,5 @@
> +      } else {
> +        nsDependentJSString choice;
> +        if (!choice.init(cx, val)) {
> +          MOZ_ASSERT(false, "Couldn't initialize string from aChoices");
> +          return NS_ERROR_FAILURE;

Don't assert here, just fail.  This can happen in low memory situations.

Comment 14

[[:fabrice] Fabrice Desré]

Is that expected to land on top of bug 853356 ? How can I test this patch?

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> Is that expected to land on top of bug 853356 ? How can I test this patch?
Yes, it is based on bug 853356, we should resume the review process after the patch for bug 853356 is landed and becomes stable.

Comment 16

[Jason Smith [:jsmith]]

Per discussion offline - gUM camera support is a targeted feature, not committed, which means we don't need to block on this. We should try to get this in for 1.3 still, however.

Comment 17

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #11)
> @@ +281,5 @@
> > +  // iterate through audio-capture and video-capture
> > +  AutoSafeJSContext cx;
> > +  JS::Rooted<JSObject*> obj(cx, &aChoices.toObject());
> > +  JSAutoCompartment ac(cx, obj);
> > +  JS::Rooted<JS::Value> v(cx);
> 
> I'll forward the advice I got when attempting manual JS object traversal
> like this (in bug 825703 comment 14): Please use JS API as little as
> possible.
> 
> I see you already have dictionaries like PermissionChoice defined in the
> Part 1 patch, so in obj/js/xpconnect/src/DictionaryHelpers.h|.cpp you should
> find generated PermissionChoice::Init() code that should initialize a c++
> representation of a jsval for you, I believe. Using that is regarded to be
> safer.
> 
> Disclaimer: I used a webidl dictionary when I did this, but from my quick
> inspection, xpidl appears to generate similar stuff, so hopefully that
> works. Feel free to look at my patch in bug 825703 for an example.
The interface for carrying chosen microphone/camera is like this:
{
  "audio-capture":"",
  "video-capture":"back"
}
The name of each attribute is different for each permission type, so it's not comply with a dictionary type. The dictionary declaration is not reflect to my code here so I'm going to remove it.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #13)
> ::: dom/base/nsContentPermissionHelper.cpp
> @@ +92,5 @@
> >  NS_IMPL_ISUPPORTS1(ContentPermissionType, nsIContentPermissionType)
> >  
> >  ContentPermissionType::ContentPermissionType(const nsACString& aType,
> > +                                             const nsACString& aAccess,
> > +                                             const nsTArray<nsCString>& aOptions)
> 
> Is it possible to make aOptions non-const, and then do
> mOptions.SwapElements(aOptions)?  I didn't look at the other patch, but if
> you're using arrays you get out of IPDL it might not be.
Yes, the array could be from IPDL so I cannot use SwapElements to reduce memory copy.

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - add options in permission prompt for Video/Audio device selection, v2

Update according to comment 13, carry khuey's r+.
Here is the overview for the change for permission prompt.
1. add an optional argument |choices| for nsIContentPermissionRequest.allow()
2. carry options in mozChromeEvent.
3. pass the choice in mozContentEvent back to permission request.
4. do not remember within a session if user deny to share camera/microphone.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - add available devices in the options of media permission request. v2

update according to comment 11, carry r+.

Comment 21

[[:fabrice] Fabrice Desré]

Comment on attachment 8342090 [details] [diff] [review]
Part 1 - add options in permission prompt for Video/Audio device selection, v2

Review of attachment 8342090 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me, but I'd like another pair of eyes on the c++ parts.

::: dom/base/nsContentPermissionHelper.cpp
@@ +133,5 @@
> +  // copy options into JS array
> +  for (uint32_t i = 0; i < mOptions.Length(); ++i) {
> +    nsCOMPtr<nsISupportsString> isupportsString =
> +      do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS is deprecated, see https://groups.google.com/forum/#!searchin/mozilla.dev.platform/NS_ENSURE_SUCCESS/mozilla.dev.platform/1clMLuuhtWQ/8MxLDZN28Y4J

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +11,5 @@
>  
>  /**
>   *  Interface provides the request type and its access.
>   */
> +[scriptable, builtinclass, uuid(EEC678AD-8A73-4517-896A-4F3481DF5436)]

nit: lower case letters in the uuid.

@@ +36,5 @@
>   * Interface allows access to a content to request
>   * permission to perform a privileged operation such as
>   * geolocation.
>   */
> +[scriptable, uuid(873A081B-AA05-4411-BC36-B7239C2ECF97)]

here also.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Fabrice Desré [:fabrice] from comment #21)
> Comment on attachment 8342090 [details] [diff] [review]
> Part 1 - add options in permission prompt for Video/Audio device selection,
> v2
> 
> Review of attachment 8342090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That looks good to me, but I'd like another pair of eyes on the c++ parts.
@khuey gave r+ for the c++ part in comment 13. Do you have other suggested reviewer if that's not enough?

BTW, I'm going to create some testcases before we land these patches.

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Shih-Chiang Chien [:schien] from comment #22)
> > 
> > That looks good to me, but I'd like another pair of eyes on the c++ parts.
> @khuey gave r+ for the c++ part in comment 13. Do you have other suggested
> reviewer if that's not enough?

I missed that! we're good then.

> BTW, I'm going to create some testcases before we land these patches.

Even better! thanks!

Comment 24

[Jason Smith [:jsmith]]

Shih-Chiang - The dependencies have landed. What's left to be done here to land this?

Comment 25

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I'm doing the rebase and make sure no regression in the permission prompt, will upload the updated patch later.

Comment 26

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - add options in permission prompt for Video/Audio device selection, v3

rebase to m-c tip, include following modifications:
1. use JS::HandleValue instead of JS::Value
2. support options for permission request from dom/permission/PermissionPromptHelper.jsm

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - add available devices in the options of media permission request. v2

rebase to m-c tip, carry r+. (Use JS::HandleValue instead of JS::Value).

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

test case is on the way.

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

try result without testcase for permission options: https://tbpl.mozilla.org/?tree=Try&rev=59e663427b60

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - testcase

test case is available now.

try result: https://tbpl.mozilla.org/?tree=Try&rev=2a4a75c05795

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 8375335 [details] [diff] [review]
Part 3 - testcase

Review of attachment 8375335 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like something cleaner for the permission-request hack...

::: b2g/components/test/mochitest/permission_handler_chrome.js
@@ +37,5 @@
>        return;
>      }
>  
> +    // sendAsyncMesssage will convert array into array-like object
> +    sendAsyncMessage("permission-request", JSON.stringify(evt.detail.permissions));

why do we stringify here?

::: b2g/components/test/mochitest/test_sandbox_permission.html
@@ +68,5 @@
>  
> +gScript.addMessageListener("permission-request", function (permissions) {
> +  var expectedValue = JSON.stringify(gResult.shift());
> +  info('received permission request: ' + permissions);
> +  is(permissions, expectedValue, "receive permission request");

that's a bit ugly tbh...

Comment 32

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Fabrice Desré [:fabrice] from comment #31)
> Comment on attachment 8375335 [details] [diff] [review]
> Part 3 - testcase
> 
> Review of attachment 8375335 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like something cleaner for the permission-request hack...
> 
> ::: b2g/components/test/mochitest/permission_handler_chrome.js
> @@ +37,5 @@
> >        return;
> >      }
> >  
> > +    // sendAsyncMesssage will convert array into array-like object
> > +    sendAsyncMessage("permission-request", JSON.stringify(evt.detail.permissions));
> 
> why do we stringify here?
I observed that passing an array object via sendAsyncMessage will get a array-like object instead of an array object, and I want to verify the original array instead of manually iterate through all the index.
> 
> ::: b2g/components/test/mochitest/test_sandbox_permission.html
> @@ +68,5 @@
> >  
> > +gScript.addMessageListener("permission-request", function (permissions) {
> > +  var expectedValue = JSON.stringify(gResult.shift());
> > +  info('received permission request: ' + permissions);
> > +  is(permissions, expectedValue, "receive permission request");
> 
> that's a bit ugly tbh...

Comment 33

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@fabrice, I can upload a quick change if you think the following code is better:

[permission_handler_chrome.js]
sendAsyncMessage("permission-request", evt.detail.permissions);

[test_sandbox_permission.html]
gScript.addMessageListener("permission-request", function (permissions) {
  let expectedValue = gResult.shift();
  let permissionTypes = Object.keys(permissions);

  is(permissionTypes.length, Object.keys(expectedValue).length, "expected number of permissions");
  for (let type of permissionTypes) {
    ok(expectedValue.hasOwnProperty(type), "expected permission type");
    for (let option of permissions[type]) {
      is(option, expectedValue[type][i], "expected permission option");
    }
  }

Comment 34

[[:fabrice] Fabrice Desré]

Thanks Shih-Chiang, I like that better indeed!

Comment 35

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - add options in permission prompt for Video/Audio device selection, v3

rebase to m-c, carry r+

Comment 36

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - add available devices in the options of media permission request. v2

rebase to m-c, carry r+

Comment 37

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 3 - testcase

update according to review comment.

https://tbpl.mozilla.org/?tree=Try&rev=871ef188076b

Comment 38

[Jason Smith [:jsmith]]

Hold up. This isn't clear to land in 1.4 - we are no longer allowing new features to land in 1.4 unless they hit the QC blocking list & DSDS blocking list. This feature definitely has risk against the permission prompt code, so this definitely isn't safe for 1.4.

Comment 39

[[:fabrice] Fabrice Desré]

Since we don't branch gecko soon we can't block people like that. Shih-Chiang, can you do a build and get it smoketested by your local QA? If he reports here that everything is ok, we'll land.

Comment 40

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #39)
> Since we don't branch gecko soon we can't block people like that.
> Shih-Chiang, can you do a build and get it smoketested by your local QA? If
> he reports here that everything is ok, we'll land.

This is being discussed offline on a mitigation strategy for how to handle the branching problem - I'd suggest raising your concerns on the ongoing thread for this. 

Trying to land this however I don't think is an option - Ivan & I already discussed offline that this wasn't safe to land in the 1.4 timeframe given the release guidelines for 1.4 & the fact that this poses a significant risk towards regressions with WebAPI permission prompts. Not that QC has been upset historically when we've broken permission prompts historically, as it blocked their testing. We really shouldn't be taking that risk given the quality bar we need to hit here in the short time span we have.

Comment 41

[Paul Yang [:pyang] (away)]

Right now I started a test run for this based on SC's local build.
Target test suite is getUserMedia API in moztrap and since it is all about audio permission, might have certain adjustment.

Comment 42

[Jason Smith [:jsmith]]

(In reply to Paul Yang [: pyang] from comment #41)
> Right now I started a test run for this based on SC's local build.
> Target test suite is getUserMedia API in moztrap and since it is all about
> audio permission, might have certain adjustment.

Here's what you'll need to test here to ensure that this is safe:

1. Make sure we didn't regress each WebAPI permission prompt
2. Test gUM video & video+audio requests on a FxOS device with a single camera supported
3. Test gUM video & video+audio requests on a FxOS device with front/back camera supported
4. Regression test gUM audio support

[1] can be verified by pushing https://github.com/jds2501/webapi-permissions-tests/blob/gh-pages/test-webapi-permissions.zip?raw=true to your device using the app manager & running through each WebAPI prompt test in that app - contacts, every device storage option, & geolocation. You'll additionally need to verify that the permission prompt for notifications in the browser still works as well - http://mozilla.github.io/qa-testcase-data/webapi/notifications/.

[2] & [3] can be tested via using http://mozilla.github.io/webrtc-landing/gum_test.html and having a FxOS device that has a single camera supported & a different one with front & back camera supported.

[4] can be tested via using http://mozilla.github.io/webrtc-landing/gum_test.html as well.

Comment 43

[Maire Reavy [:mreavy]]

Jason -- Thanks for the info you provided in your last comment (Comment 42).  It was very helpful.

Comment 44

[Paul Yang [:pyang] (away)]

Just finished test ref[3] [4],
I didn't create new test cases for video, and I test each one by setting [3] and [4].
https://moztrap.mozilla.org/results/cases/?filter-run=3435
1 case failed since video showed hardware unavailable error

Comment 45

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Hi Jason, Paul talked to me about the failed test case #9972. It's pretty weird that this case requires to open media stream on two different tabs at the same time, however we didn't support sharing camera/microphone across multiple processes. I suppose this test case is performed in one single page that contains multiple iframes that have different origins based on Bug 940045.

Comment 46

[Jason Smith [:jsmith]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #45)
> Hi Jason, Paul talked to me about the failed test case #9972. It's pretty
> weird that this case requires to open media stream on two different tabs at
> the same time, however we didn't support sharing camera/microphone across
> multiple processes. I suppose this test case is performed in one single page
> that contains multiple iframes that have different origins based on Bug
> 940045.

Yup - that's a known issue. Not a regression from this patch.

Comment 47

[Jason Smith [:jsmith]]

(In reply to Paul Yang [: pyang] from comment #44)
> Just finished test ref[3] [4],
> I didn't create new test cases for video, and I test each one by setting [3]
> and [4].
> https://moztrap.mozilla.org/results/cases/?filter-run=3435
> 1 case failed since video showed hardware unavailable error

How did you test [3] exactly? Did you test this with single camera & front/back camera supported phone?

Comment 48

[Eric Rescorla (:ekr)]

FWIW, I tested this patch on a flame and it seems to work correctly.

Comment 49

[Jason Smith [:jsmith]]

(In reply to Eric Rescorla (:ekr) from comment #48)
> FWIW, I tested this patch on a flame and it seems to work correctly.

Okay. So that leaves one piece of analysis left here - we need verify that we didn't regress the other major WebAPI permission prompts.

Comment 50

[Paul Yang [:pyang] (away)]

(In reply to Jason Smith [:jsmith] from comment #47)
> (In reply to Paul Yang [: pyang] from comment #44)
> > Just finished test ref[3] [4],
> > I didn't create new test cases for video, and I test each one by setting [3]
> > and [4].
> > https://moztrap.mozilla.org/results/cases/?filter-run=3435
> > 1 case failed since video showed hardware unavailable error
> 
> How did you test [3] exactly? Did you test this with single camera &
> front/back camera supported phone?

[3] Run test suite for both front & back camera.  Test device is nexus4.
Haven't got a JB device with single camera, any suggestion?

Comment 51

[Jason Smith [:jsmith]]

(In reply to Paul Yang [: pyang] from comment #50)
> (In reply to Jason Smith [:jsmith] from comment #47)
> > (In reply to Paul Yang [: pyang] from comment #44)
> > > Just finished test ref[3] [4],
> > > I didn't create new test cases for video, and I test each one by setting [3]
> > > and [4].
> > > https://moztrap.mozilla.org/results/cases/?filter-run=3435
> > > 1 case failed since video showed hardware unavailable error
> > 
> > How did you test [3] exactly? Did you test this with single camera &
> > front/back camera supported phone?
> 
> [3] Run test suite for both front & back camera.  Test device is nexus4.
> Haven't got a JB device with single camera, any suggestion?

Buri would be a good option to use for single camera.

Have you tried the regression tests for other WebAPIs? That's the big thing left here I think we need.

Comment 52

[Jason Smith [:jsmith]]

Paul - here's a detailed summary of what you need to do: https://docs.google.com/a/mozilla.com/document/d/1kqse5dUJ5zt5xZTPF4JZ63azi-ji-iL1qcqrntsJG2I/edit. That doc summarizes the 11 test cases I would run to check for permission prompt regressions.

Comment 53

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #52)
> Paul - here's a detailed summary of what you need to do:
> https://docs.google.com/a/mozilla.com/document/d/1kqse5dUJ5zt5xZTPF4JZ63azi-
> ji-iL1qcqrntsJG2I/edit. That doc summarizes the 11 test cases I would run to
> check for permission prompt regressions.

Nils ended up flashing a build for me & I did verification here myself. This looks good to me - I didn't see any noticeable permission prompt regressions from this patch.

Comment 54

[Jason Smith [:jsmith]]

We're good to check this in now.

Comment 55

[Paul Yang [:pyang] (away)]

Done buri run & WebApi permission test on nexus4, no issue discovered.

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/b2g-inbound/rev/8f7defebd30a
https://hg.mozilla.org/integration/b2g-inbound/rev/ad7684d2890e
https://hg.mozilla.org/integration/b2g-inbound/rev/5d69e8ef8162

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f7defebd30a
https://hg.mozilla.org/mozilla-central/rev/ad7684d2890e
https://hg.mozilla.org/mozilla-central/rev/5d69e8ef8162

Comment 58

[Paul Yang [:pyang] (away)]

Verified by master + flame.  Work perfectly.