1659264 - remove layout.css.moz-touch-enabled.enabled

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

We removed this non-standard media feature in bug 1486964. We added this flag in case it was needed but it's about time to remove the code.

In order to fix this you need to remove all references to the preference, then remove this atom too, then this block as well as the supporting function.

Comment 1

[akshay1992kalbhor]

First time contributor here. I have only had about a week of going through the source and wanted to know if I could try working on this one?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Yes, definitely! Feel free to ask any questions you may have either here or in the #introduction channel in chat.mozilla.org

Comment 3

[akshay1992kalbhor]

Thanks. So after going through the source code, I have a few questions.

1. After removing all references to the pref, should I delete it from StaticPrefList.yaml altogether?
2. There are some tests in layout/style/test which query the preference? Am I supposed to delete those tests too?

If the answer to the two questions is yes, can I assume that I need to delete each and every line of code which contains 'layout.css.moz-touch-enabled.enabled' and '-moz-touch-enabled'?

Thanks in advance.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to akshay1992kalbhor from comment #3)

Thanks. So after going through the source code, I have a few questions.

1. After removing all references to the pref, should I delete it from StaticPrefList.yaml altogether?

Yes.

2. There are some tests in layout/style/test which query the preference? Am I supposed to delete those tests too?

Either that or change them to stop querying the pref and assuming it's false. Fine either way for me :)

If the answer to the two questions is yes, can I assume that I need to delete each and every line of code which contains 'layout.css.moz-touch-enabled.enabled' and '-moz-touch-enabled'?

Depends on the above, you may need to leave some stuff like `expression_should_not_be_parseable("-moz-touch-enabled") and such, but removing is also fine :)

Comment 5

[akshay1992kalbhor]

Thanks for the previous answers. I have made the necessary changes and tried building it and it works! But now I want to run all the necessary tests so that I can submit the patch for review. I tried running 'mach mochitest' which was obviously the wrong choice because its still running after 3 hours. So I looked at some of the resources available and found that you can run tests for a particular component by finding the appropriate test directory in its hierarchy and passing its path to the command test command like this 'mach test /path/to/test/directory/for/particular/component'.

So my question is am I on the right path? If not can you point me in the right direction? There is a lot of info available on the docs page but its also kind of confusing

Thank you

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Yeah, I think if it builds would be fine, but you can run ./mach mochitest layout/style/test/test_media_queries.html as a sanity-check.

Comment 7

[akshay1992kalbhor]

Ok almost done! ./mach mochitest layout/style/test/test_media_queries.html passed successfully.
But I also ran ./mach mochitest layout/style/test/ to be extra cautious and two of the tests failed.

Failed test 1 : ./mach mochitest layout/style/test/test_bug412901.html
Failed test 2 : ./mach mochitest layout/style/test/test_visited_reftests.html

So I checked out an earlier stable version and ran the two failed tests again and they still failed so the issue is not with the changes I made.
Just want to make sure that everything's ok

Thanks

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Yeah, those tests should be unrelated. test_visited_reftests in particular is known to be timing dependent (because of async link coloring). The other failure is suspicious, but if it fails before your patch you shouldn't worry about it.

Comment 9

[akshay1992kalbhor]

Thanks, one last question. I have also needed to change two rust files inservo/components/style and one cpp file in layout/style. Do I need to run tests which target these changes?

Comment 10

[harrihaven2]

(In reply to akshay1992kalbhor from comment #9)

Thanks, one last question. I have also needed to change two rust files inservo/components/style and one cpp file in layout/style. Do I need to run tests which target these changes?

Did you ever finish and submit the patch for review?

Comment 11

[akshay1992kalbhor]

Attachment: Bug 1659264 - Remove all references to the preference 'layout.css.moz-touch-enabled.enabled'. r=emilio

Comment 12

[akshay1992kalbhor]

Apologise for the delay. I have submitted the patch!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58a563de637c
Remove all references to the preference 'layout.css.moz-touch-enabled.enabled'. r=emilio

Comment 14

[akshay1992kalbhor]

Hi, is there anything else that I need to do now that I have submitted the patch and it has been approved? I haven't done this before.

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/58a563de637c

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to akshay1992kalbhor from comment #14)

Hi, is there anything else that I need to do now that I have submitted the patch and it has been approved? I haven't done this before.

No, there was nothing else :)

I pushed it to the "autoland" branch in comment 13, and it eventually gets merged into the main branch ("mozilla-central") and the bus gets automatically marked as resolved (comment 15).

Thanks a lot for the fix!

Comment 17

[akshay1992kalbhor]

Cool! Cannot wait to close another one! Thanks