Closed Bug 1297693 Opened 3 years ago Closed 3 years ago

Use a more specific selector in tests for blocked permission icons

Categories

(Firefox :: Site Identity, defect, P5)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: johannh, Assigned: udayanbaidya, Mentored)

References

Details

(Whiteboard: [fxprivacy] [good first bug] )

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [fxprivacy] [good first bug] [triage] → [fxprivacy] [good first bug]
Priority: -- → P5
Hello I would like to be assigned to this bug. I am new here and would like to be mentored.
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!
Assignee: nobody → udayanbaidya
Added changes as per instructions to correct the bug
Attachment #8787853 - Flags: review?(jhofmann)
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.
Attachment #8787853 - Flags: review?(jhofmann)
Attachment #8787853 - Flags: review?(dao+bmo)
Attachment #8787853 - Flags: review+
Sure :-) will do
Attachment #8787853 - Attachment is obsolete: true
Attachment #8787853 - Flags: review?(dao+bmo)
Attachment #8788212 - Flags: review?(jhofmann)
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.
Attachment #8788212 - Flags: review?(jhofmann)
Attachment #8788212 - Flags: review?(dao+bmo)
Attachment #8788212 - Flags: review+
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=...]?
Attachment #8788212 - Flags: review?(dao+bmo) → review+
(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.
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.
Keywords: checkin-needed
Sounds good to me :-) Thanks for all the help and guidance :-)
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d3d05b94bc1b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
[bugday-20170118] Bug Verified
You need to log in before you can comment on or make changes to this bug.