Remove backward compatibility guard for isNameValid

RESOLVED FIXED in Firefox 67

Status

enhancement
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: gl, Assigned: andrewcylaw, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 67

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 months ago

In https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/models/text-property.js#238-243, we have a backwards compatibility guard that returns true for older versions of Firefox. Since we are already on FF67, we don't need this guard anymore. We should place the return with

return this.rule.domRule.declarations[selfIndex].isNameValid;
Reporter

Updated

3 months ago
No longer blocks: dt-rules
Reporter

Updated

3 months ago
Mentor: gl
Assignee

Comment 1

3 months ago

Hello!

I'm looking to learn more about working with the Mozilla code base. I would like to be assigned this bug, and would appreciate any help on it.

Just to double check the original functionality - if I remove the compatibility guard in <FF61, the inspector would accept all names for any new css property that we throw at it? If not, what would be a good way to double check that the changes from the guard worked?

Thanks!
Andrew

Reporter

Comment 2

3 months ago

We expect to always have this accessor isNameValid. This was added in https://hg.mozilla.org/mozreview/gecko/rev/1419520f2e7ca7d41edc486e7a7f852867d253d4#index_header. Running the test devtools/client/inspector/rules/test/browser_rules_authored.js will also help check this. The inspector would not accept all names since it is checking this accessor to see if the name is valid. We are removing the guard because the accessor is available in the recent FF. You can also manually test this by throwing in some invalid declaration names when editing the rules.

Assignee: nobody → andrewcylaw
Status: NEW → ASSIGNED
Assignee

Comment 3

3 months ago

Hello again, sorry for the long delay. I made the code change and updated the comments. I used a command I found on the Mozilla 'Contributing' page that lets me use Git to make a Mercurial patch so I hope that's valid. The tests also passed using the following command.

./mach mochitest -f browser devtools/client/inspector/rules/test/browser_rules_authored.js

If I missed anything please let me know! Thanks in advance.

Attachment #9049406 - Flags: review+
Attachment #9049406 - Flags: feedback+
Reporter

Updated

3 months ago
Flags: needinfo?(gl)
Reporter

Comment 4

3 months ago

r+ from me.

Flags: needinfo?(gl)

Comment 5

3 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/211218d95e2c
Remove compatibility workaround for validating property names.

Comment 6

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.