1590196 - Remove actorHasMethod check for hasAccessibilityProperties

Status: RESOLVED FIXED

(DevTools :: Inspector, task, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 1

[Lina R]

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!

Comment 2

[Gabriel Luong [: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.

Comment 3

[Lina R]

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!

Comment 4

[Gabriel Luong [: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

Comment 5

[Lina R]

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 ?

Comment 6

[Gabriel Luong [: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.

Comment 7

[Patrick Brosset <:pbro>]

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!

Comment 8

[Lina R]

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

Comment 9

[Patrick Brosset <:pbro>]

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.

Comment 10

[Patrick Brosset <:pbro>]

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

Comment 11

[Lina R]

Attachment: Bug 1590196 - Remove unneeded backward compatibility check hasAccessibilityProperties; r=pbro

Comment 12

[Lina R]

(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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65eec20b827f
Remove unneeded backward compatibility check hasAccessibilityProperties; r=pbro

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/65eec20b827f