Closed Bug 1590196 Opened 1 year ago Closed 9 months ago

Remove actorHasMethod check for hasAccessibilityProperties

Categories

(DevTools :: Inspector, task, P3)

task

Tracking

(firefox75 fixed)

RESOLVED FIXED
Firefox 75
Tracking Status
firefox75 --- fixed

People

(Reporter: gl, Assigned: lina.refai, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

We added this backward compatibility check for hasAccessibilityProperties in Firefox 61 (Bug 1428441). We no longer need this check so we can remove the hasMethod check here in https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/markup-context-menu.js#953

Hi! I'm a first time contributor, and I'd like to help with this. What steps would you require me to take in order to pick up this bug?

Further, I see that the hasMethod is not used at all in that function, so we'd be removing lines 953-961, not just the check, right?

Thanks!

Flags: needinfo?(gl)

Hi Lina, thanks for helping out. I have assigned you to the bug. We can basically remove lines 953-961 which includes the check and if statement.

Assignee: nobody → lina.refai
Status: NEW → ASSIGNED
Flags: needinfo?(gl)

Hi Gabriel, I've completed the removal, and built the code to make sure it's okay. I also ran ./mach test all, and nothing seems to be failing in relation to this change. For the commit message, should I put you as the reviewer, or leave it blank?

Also do let me know if there is anything additional I must check/run before submitting the patch.

Thanks!

Flags: needinfo?(gl)

(In reply to Lina R from comment #3)

Hi Gabriel, I've completed the removal, and built the code to make sure it's okay. I also ran ./mach test all, and nothing seems to be failing in relation to this change. For the commit message, should I put you as the reviewer, or leave it blank?

Also do let me know if there is anything additional I must check/run before submitting the patch.

Thanks!

Yes, feel free to put me as the reviewer. The commit message should look something like:
Bug 1590196 - Remove actorHasMethod check for hasAccessibilityProperties. r=gl

Flags: needinfo?(gl)

Great, all set! Should I now be submitting the patch via Phabricator? If yes, do I just follow these steps: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html ?

Flags: needinfo?(gl)

(In reply to Lina R from comment #5)

Great, all set! Should I now be submitting the patch via Phabricator? If yes, do I just follow these steps: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html ?

Yep using moz-phab should take care of it.

Flags: needinfo?(gl)

Hi Lina, did you need any further help on pushing to phabricator? Please let us know so we can help you get this reviewed and checked in!

Flags: needinfo?(lina.refai)

(In reply to Patrick Brosset <:pbro> from comment #7)

Hi Lina, did you need any further help on pushing to phabricator? Please let us know so we can help you get this reviewed and checked in!

Hi Patrick, apologies for going MIA! I did have a question, actually - I saw on the docs that using moz-phab off either a git repo or mercurial works, but does it work if I've forked gecko-dev?

Thanks!

Flags: needinfo?(lina.refai)

Hi Lina. You can indeed use moz-phab with either mercurial or git. I personally use it with mercurial, and I'm not too familiar with how it works with git. I think most people who do this actually use git-cinnabar under the hood.
If you've already cloned gecko-dev, you could give it a try though. Make your change there, commit it, push it to phabricator using moz-phab, and we'll be able to see whether that worked fine. If it didn't, then I would suggest trying git-cinnabar.

Some people seem to use phlay in order to push to phabricator from a git repository. That might be an option for you too.

(In reply to Patrick Brosset <:pbro> from comment #9)

Hi Lina. You can indeed use moz-phab with either mercurial or git. I personally use it with mercurial, and I'm not too familiar with how it works with git. I think most people who do this actually use git-cinnabar under the hood.
If you've already cloned gecko-dev, you could give it a try though. Make your change there, commit it, push it to phabricator using moz-phab, and we'll be able to see whether that worked fine. If it didn't, then I would suggest trying git-cinnabar.

Hi Patrick, I've submitted the commit to Phabricator for review, and I added you as the reviewer - I hope that's okay? (moz-phab with git-cinnabar worked :)) Thank you for your advice!
Let me know if there's more left to do, or any comments you have :)
Lina

Attachment #9125400 - Attachment description: removed redundant lines for Bug 1590196 r=pbro → Bug 1590196 - Remove unneeded backward compatibility check hasAccessibilityProperties; r=pbro
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65eec20b827f
Remove unneeded backward compatibility check hasAccessibilityProperties; r=pbro
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75
You need to log in before you can comment on or make changes to this bug.