Alfredo Yang (:alfredo) Reporter
	Description • 11 years ago

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.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 1 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 2 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 3 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 4 • 11 years ago

Support permission options in permission request.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 5 • 11 years ago

Add available device in the options of media permission request.

	Maire Reavy [:mreavy]
	Comment 6 • 11 years ago

We should make this 1.3+

	Jason Smith [:jsmith]
	Comment 7 • 11 years ago

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

Agreed.

	Randell Jesup [:jesup] (needinfo me)
	Comment 8 • 11 years ago

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)

	Jason Smith [:jsmith]
	Comment 9 • 11 years ago

(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"
   }
}

	Jason Smith [:jsmith]
	Comment 10 • 11 years ago

(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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 11 • 11 years ago

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).

	Ivan Tsay (:ITsay)
	Comment 12 • 11 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 13 • 11 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 14 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 15 • 11 years ago

(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.

	Jason Smith [:jsmith]
	Comment 16 • 11 years ago

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.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 17 • 11 years ago

(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.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 18 • 11 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 19 • 11 years ago

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.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 20 • 11 years ago

update according to comment 11, carry r+.

	[:fabrice] Fabrice Desré
	Comment 21 • 11 years ago

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.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 22 • 11 years ago

(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.

	[:fabrice] Fabrice Desré
	Comment 23 • 11 years ago

(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!

	Jason Smith [:jsmith]
	Comment 24 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 25 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 26 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 27 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 28 • 11 years ago

test case is on the way.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 29 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 30 • 11 years ago

test case is available now.

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

	[:fabrice] Fabrice Desré
	Comment 31 • 11 years ago

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...

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 32 • 11 years ago

(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...

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 33 • 11 years ago

@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");
    }
  }

	[:fabrice] Fabrice Desré
	Comment 34 • 11 years ago

Thanks Shih-Chiang, I like that better indeed!

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 35 • 11 years ago

rebase to m-c, carry r+

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 36 • 11 years ago

rebase to m-c, carry r+

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 37 • 11 years ago

update according to review comment.

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

	Jason Smith [:jsmith]
	Comment 38 • 11 years ago

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.

	[:fabrice] Fabrice Desré
	Comment 39 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 40 • 11 years ago

(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.

	Paul Yang [:pyang] (away)
	Comment 41 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 42 • 11 years ago

(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.

	Maire Reavy [:mreavy]
	Comment 43 • 11 years ago

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

	Paul Yang [:pyang] (away)
	Comment 44 • 11 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 45 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 46 • 11 years ago

(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.

	Jason Smith [:jsmith]
	Comment 47 • 11 years ago

(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?

	Eric Rescorla (:ekr)
	Comment 48 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 49 • 11 years ago

(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.

	Paul Yang [:pyang] (away)
	Comment 50 • 11 years ago

(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?

	Jason Smith [:jsmith]
	Comment 51 • 11 years ago

(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.

	Jason Smith [:jsmith]
	Comment 52 • 11 years ago

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.

	Jason Smith [:jsmith]
	Comment 53 • 11 years ago

(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.

	Jason Smith [:jsmith]
	Comment 54 • 11 years ago

We're good to check this in now.

	Paul Yang [:pyang] (away)
	Comment 55 • 11 years ago

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 56 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 57 • 11 years ago

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

	Paul Yang [:pyang] (away)
	Comment 58 • 11 years ago

Verified by master + flame.  Work perfectly.