1297693 - Use a more specific selector in tests for blocked permission icons

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P5)

Description

[Johann Hofmann [:johannh]]

The [data-permission-id=...] selector in http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/browser/base/content/test/general/browser_permissions.js#120 and below isn't specific enough and could lead to failures once someone adds that attribute to other elements in the identity block.

We should change it to .blocked-permission-icon[data-permission-id=...]

If you're completely new to contributing to Firefox check out https://developer.mozilla.org/en-US/docs/Tools/Contributing to get started.

Comment 1

[Udayan Baidya]

Hello I would like to be assigned to this bug. I am new here and would like to be mentored.

Comment 2

[Johann Hofmann [:johannh]]

Hi there, thanks for contributing! You can find instructions for building Firefox here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build. You can use any IDE/Editor of your choice.

See comment 0 for instructions on how to solve the bug once you have cloned mozilla-central. If you have _any_ questions you can either ask here or on IRC (https://wiki.mozilla.org/IRC), recommended channels are #introduction and #developers. My nickname is johannh.

https://developer.mozilla.org/en-US/docs/Tools/Contributing gives you a nice summary of everything you need to know, some of it is specific to DevTools but most applies to Firefox development in general.

Let me know if anything is unclear!

Comment 3

[Udayan Baidya]

Attachment: rev1 - Added more specific selectors

Added changes as per instructions to correct the bug

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8787853 [details] [diff] [review]
rev1 - Added more specific selectors

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

Looks great, thanks. See my one comment :)

Dao, can you give this another quick review?

::: browser/base/content/test/general/browser_permissions.js
@@ +183,5 @@
>    SitePermissions.set(gBrowser.currentURI, "camera", SitePermissions.ALLOW);
>    SitePermissions.set(gBrowser.currentURI, "geo", SitePermissions.BLOCK);
>    SitePermissions.set(gBrowser.currentURI, "microphone", SitePermissions.SESSION);
>  
> +  let geoIcon = gIdentityHandler._identityBox.querySelector(".blocked-permission-icon[data-permission-id='geo']");

Small nit: can you move '.querySelector(...' to the next line and indent it by one level? Also for the other two changes, please. Keeps the lines shorter.

Comment 5

[Udayan Baidya]

Sure :-) will do

Comment 6

[Udayan Baidya]

Attachment: rev2- Added proper indentation to specific selectors

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8788212 [details] [diff] [review]
rev2- Added proper indentation to specific selectors

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

Looks great! Since I'm not a peer let's get a quick review from Dao, then we're good to go.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8788212 [details] [diff] [review]
rev2- Added proper indentation to specific selectors

Could also use ".blocked-permission-icon.geo-icon". Is there's any advantage in using [data-permission-id=...]?

Comment 9

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 8788212 [details] [diff] [review]
> rev2- Added proper indentation to specific selectors
> 
> Could also use ".blocked-permission-icon.geo-icon". Is there's any advantage
> in using [data-permission-id=...]?

There's no real advantage for these tests, but it might be a bit more precise. data-permission-id is the exact permission identifier and .geo-icon just defines the style (we could very hypothetically have another .geo-icon or the classname could change), so I think it's fine to leave it like this.

Comment 10

[Johann Hofmann [:johannh]]

Let's get this patch landed! Usually we would have to run it on our try server (https://wiki.mozilla.org/ReleaseEngineering/TryServer) to make sure all tests are still passing, but since this patch only changes a single test I just ran it locally:

./mach mochitest browser/base/content/test/general/browser_permissions.js

I've also set the checkin-needed flag. This flag signals that we want the patch merged. Eventually someone will notice and check it in for us.

Comment 11

[Udayan Baidya]

Sounds good to me :-) Thanks for all the help and guidance :-)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d3d05b94bc1b
Used a more specific selector in tests for blocked permission icons in browser_permissions.js. r=johannh, r=dao

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3d05b94bc1b

Comment 14

[nagarajnaidu921]

[bugday-20170118] Bug Verified