See http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/browser/base/content/test/general/browser_temporary_permissions.js#41 We can probably do this in the same test, by making an "ids" array that contains e.g. "geo" and "camera" and run the bulk of the test in a loop over those two elements.

I would like to fix this bug. Could you please assign it to me?

Absolutely! Let me know if you have any questions. Thanks

Hi Johann, I hate to do this; but would you unassign this bug? I bit off a little more than I can chew. Sorry to have wasted your time. Regards, Bianca

(In reply to Bianca Danforth [:bdanforth] from comment #3) > Hi Johann, > I hate to do this; but would you unassign this bug? I bit off a little more > than I can chew. Sorry to have wasted your time. > Regards, > Bianca Sure, don't worry about it. Thanks for letting me know. :)

Hi! Can I work on this bug? I am a beginner and still catching up to things. (Have worked only on a few moz bugs) From an initial glance I see statements like: > let geoIcon = document.querySelector(".blocked-permission-icon[data-permission-id=geo]"); I am guessing that for "camera", these would become something like > let cameraIcon = document.querySelector(".blocked-permission-icon[data-permission-id=camera]"); Also, are there specific statements/checks that have to be added for "camera"? Thanks in advance!

(In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #5) > Hi! Can I work on this bug? Hey Swapnesh, > I am a beginner and still catching up to things. (Have worked only on a few > moz bugs) you can absolutely work on this! I was originally thinking that you only need to update the test function testTempPermissionRequestAfterExpiry, but you can apply the same method to the other two tests as well. I would really recommend starting with testTempPermissionRequestAfterExpiry, though. That test is already configured by setting the "id" variable at the beginning. What we now need is a simple loop that assigns id to "geo" first and then to "camera". > > From an initial glance I see statements like: > > > let geoIcon = document.querySelector(".blocked-permission-icon[data-permission-id=geo]"); > > I am guessing that for "camera", these would become something like > > > let cameraIcon = document.querySelector(".blocked-permission-icon[data-permission-id=camera]"); Not quite, you would rather make it dependent on the id to avoid repeating code. E.g. > let blockedIcon = document.querySelector(`.blocked-permission-icon[data-permission-id=${id}]`); and then iterate over two ids. > Also, are there specific statements/checks that have to be added for > "camera"? Nope, just make everything use id instead of "geo". > Thanks in advance! Thank you!

I couldn't find the file mentioned in the description and a search for testTempPermissionRequestAfterExpiry on SearchFox returned the file in the patch. Also, I was not sure how I could test my changes, so currently the patch contains the changes without any checks. Do let me know what needs to be done regarding the same.

Comment on attachment 8865468 [details] [diff] [review] Test update for WebRTC temporary permission expiry Review of attachment 8865468 [details] [diff] [review]: ----------------------------------------------------------------- This looks good overall, but you added spaces to a couple of blank lines in the test file. Please remove those. :) Thanks! ::: browser/base/content/test/permissions/browser_temporary_permissions_expiry.js @@ +23,2 @@ > > + for(i = 0; i < ids.length; i++) { You can use for(let id of ids) { @@ +25,5 @@ > + yield BrowserTestUtils.withNewTab(PERMISSIONS_PAGE, function*(browser) { > + let id = ids[i]; > + let blockedIcon = gIdentityHandler._identityBox > + .querySelector(`.blocked-permission-icon[data-permission-id='${id}']`); > + This is the first line with two spaces, please remove :)

I've incorporated the requested changes. Do let me know what more needs to be done. :)

Comment on attachment 8865895 [details] [diff] [review] Test update for WebRTC temporary permission expiry Review of attachment 8865895 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks. Please remove the TODO comment in line 13 and 14 and upload another version. In the meantime I'll do a try push (https://wiki.mozilla.org/ReleaseEngineering/TryServer) to make sure the test is passing on all platforms.

Comment on attachment 8865895 [details] [diff] [review] Test update for WebRTC temporary permission expiry Review of attachment 8865895 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/permissions/browser_temporary_permissions_expiry.js @@ +20,5 @@ > > let uri = NetUtil.newURI(ORIGIN); > + let ids = ["geo", "camera"]; > + > + for(let id of ids) { ESLint reports that you should please add a space after "for" :)

Comment on attachment 8866063 [details] [diff] [review] Test update for WebRTC temporary permission expiry (with TODO comment removed) Review of attachment 8866063 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/permissions/browser_temporary_permissions_expiry.js @@ +14,1 @@ > // because they use a different code path. You can also remove this line.

Changes requested in Comment#10 and Comment#13 is contained in this patch.

Sorry for my hasty and impatient actions. This patch contains changes requested in Comment#10, Comment#13 and Comment#14.

Comment on attachment 8866068 [details] [diff] [review] Test update for WebRTC temporary permission expiry (with extra comments removed) Review of attachment 8866068 [details] [diff] [review]: ----------------------------------------------------------------- Actually I'm sorry for not checking that the test actually passes. :) It doesn't pass because you need to add a line similar to this one https://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/browser/tools/mozscreenshots/mozscreenshots/extension/lib/permissionPrompts.html#10 (you should probably change the "id" attribute to "camera") to this file: https://searchfox.org/mozilla-central/source/browser/base/content/test/permissions/permissions.html you can run the test using ./mach mochitest browser/base/content/test/permissions/browser_temporary_permissions_expiry.js Thanks! ::: browser/base/content/test/permissions/browser_temporary_permissions_expiry.js @@ +10,4 @@ > const TIMEOUT_MS = 500; > > // Test that temporary permissions can be re-requested after they expired > +// and that the identity block is updated accordingly nit: please add the dot again so that this line is unchanged

Changes requested in Comment#17 are incorporated in this patch.

I forgot to mention in Comment#18 that the test is passing with the latest changes. Attached file (test_log) is the log for the same. :)

Comment on attachment 8866423 [details] [diff] [review] Test update for WebRTC temporary permission expiry Review of attachment 8866423 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Almost there! :) ::: browser/base/content/test/permissions/browser_temporary_permissions_expiry.js @@ +14,3 @@ > add_task(function* testTempPermissionRequestAfterExpiry() { > yield SpecialPowers.pushPrefEnv({set: [ > ["privacy.temporary_permission_expire_time_ms", EXPIRE_TIME_MS], Please add ["media.navigator.permission.fake", true], here. That's the preference that enables fake media devices in case a machine doesn't have any (like our try servers). ::: browser/base/content/test/permissions/permissions.html @@ +10,4 @@ > <!-- This page could eventually request permissions from content > and make sure that chrome responds appropriately --> > <button id="geo" onclick="navigator.geolocation.getCurrentPosition(() => {})">Geolocation</button> > + <button id="camera" onclick="Notification.requestPermission(() => {})">web-notifications</button> Since we want to test WebRTC this should call e.g. navigator.mediaDevices.getUserMedia({video: true, fake: true}) instead. You also need to push an additional pref for it to work on try servers, please see the other comment.

Changes as per comment#20

Comment on attachment 8866458 [details] [diff] [review] Test update for WebRTC temporary permission expiry Review of attachment 8866458 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now, just one minor thing! Let's do another try run... ::: browser/base/content/test/permissions/permissions.html @@ +10,4 @@ > <!-- This page could eventually request permissions from content > and make sure that chrome responds appropriately --> > <button id="geo" onclick="navigator.geolocation.getCurrentPosition(() => {})">Geolocation</button> > + <button id="camera" onclick="navigator.mediaDevices.getUserMedia({video: true, fake: true})">web-notifications</button> Please change the text of the button ("web-notifications") to something that makes more sense e.g. "Camera"

Incorporated changes as per comment#22

MozReview-Commit-ID: 4BIuz1ikUQ5

Comment on attachment 8866990 [details] [diff] [review] Test update for WebRTC temporary permission expiry Review of attachment 8866990 [details] [diff] [review]: ----------------------------------------------------------------- The test was failing on try due to another test not cleaning up properly. I've fixed that and we can hopefully land soon after another try run. Sorry for that.

Try is looking good, I'll set the checkin-needed flag so that someone lands this for us.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd8c2d22b26 Test update for WebRTC temporary permission expiry. r=johannh