Remove layout.css.filters.enabled

RESOLVED DUPLICATE of bug 1408841

Status

()

enhancement
P3
normal
RESOLVED DUPLICATE of bug 1408841
a year ago
8 months ago

People

(Reporter: emilio, Unassigned, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

It's true everywhere, and we don't honor it anyway.
(Reporter)

Updated

a year ago
Mentor: emilio
(Reporter)

Comment 1

a year ago
Currently affected code is:

  https://searchfox.org/mozilla-central/search?q=css.filters&case=false&regexp=false&path=

The way to fix this would be:

 * Removing the function nsLayoutUtils::CSSFiltersEnabled(), since it's unused.
 * Change layout/style/test/property_database.js so that it doesn't check IsCSSPropertyPrefEnabled("layout.css.filters.enabled"), and assumes it's true everywhere instead.
 * same for test_transitions_per_property.
 * Remove the other references to the pref from the test manifests, by removing the "pref(..)" and "default-prefs" sections.

Comment 2

a year ago
Hi, I would like to work on this bug. I am pretty new at this so any help would be appreciated.
(Reporter)

Comment 3

a year ago
Sure! Please let me know if you have any question about comment 1, or if you get stuck somewhere. I'm happy to help :)

Once you submit the first patch, we can assign this to you.

Thanks for looking into this!
(Reporter)

Comment 4

a year ago
Oh, some questions that may come up later, regarding testing:

For the first point, removing that function, there's no real testing needed, if it builds it's good to go.

For the second and third, you can run:

  ./mach test layout/style/test/test_transitions_per_property.html

If it still passes it's probably good to go.

For the fourth running that particular test / any test in that directory should work.

Comment 5

a year ago
Hi I removed the function and it builds fine, However, I cant't find the .js file and in the test directory and on trying to run find IsCSSPropertyPrefEnabled("layout.css.filters.enabled") in the html find it returns no match. How should I proceed?
(Reporter)

Comment 6

a year ago
(In reply to keshavmanghat from comment #5)
> Hi I removed the function and it builds fine

Awesome!

> However, I cant't find the .js
> file and in the test directory and on trying to run find
> IsCSSPropertyPrefEnabled("layout.css.filters.enabled") in the html find it
> returns no match. How should I proceed?

The js code I was talking about is in layout/style/test/property_database.js, doesn't that file exist in your tree?

And sorry, in layout/style/test/test_transitions_per_property.html the check is not exactly that, but:

  if (!SpecialPowers.getBoolPref("layout.css.filters.enabled")) {

Ultimately it does the same, and and since that call returns always true, the if and the return inside can be removed.

Does that help?

Comment 7

a year ago
It doesn't exist in my tree, I will take a look at the rest of it, I do not know how to submit a patch. 
I am working on ubuntu 16.04 and downloaded the build 2 days ago.

Comment 8

a year ago
Also, I removed the function sCSSPropertyPrefEnabled("layout.css.filters.enabled") but when I ran ./mach test layout/style/test/test_transitions_per_property.html   it gives me an error saying that symlink path does not exist and that I should consider filing a bug for this issue.

After this failed I ran ./mach build and that was successful even after removing the dead code.

Comment 9

a year ago
UPDATE:
I removed the dead function ran mach ./build and it was successful
I found the .js file after reinstalling the build and removed the check. I did the same for the third file as well.
I ran the tests with your command and it was successful.

I am not sure how to submit a patch and also did not quite understand the last part.
Priority: -- → P3
(Reporter)

Comment 10

a year ago
(In reply to keshavmanghat from comment #9)
> UPDATE:
> I removed the dead function ran mach ./build and it was successful
> I found the .js file after reinstalling the build and removed the check. I
> did the same for the third file as well.
> I ran the tests with your command and it was successful.
> 
> I am not sure how to submit a patch and also did not quite understand the
> last part.

You can export it with:

hg export -r . > bug1465116.patch

Then update that patch to bugzilla.

The last part is about these lines of code:

  https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/layout/style/test/test_transitions_per_property.html#1672-1674

Which can be removed now.

Comment 11

a year ago
I already did that I was referring to this part
Remove the other references to the pref from the test manifests, by removing the "pref(..)" and "default-prefs" sections.
(Reporter)

Comment 12

a year ago
Those files are listed in my link in comment 1, they are:

  layout/reftests/css-blending/reftest.list
  layout/reftests/svg/filters/css-filter-chains/reftest.list
  layout/reftests/svg/filters/css-filters/reftest.list
  layout/reftests/svg/filters/css-svg-filter-chains/reftest.list
  layout/reftests/svg/filters/svg-filter-chains/reftest.list

Thanks for working on this!

Comment 13

a year ago
Ok thank you, I will submit a patch as soon as I fix that.

Comment 15

a year ago
I am still having troubles with creating and exporting the patch, I executed the export command and it creates a text file but the contents of the file are nothing related to what I just did. I attached it to this thread if you want to look at it.
(Reporter)

Comment 16

a year ago
You probably need to commit the changes first using `hg commit`.

Comment 17

a year ago
Yep that was it, I got it done, I think it is ready for review.
(Reporter)

Comment 19

a year ago
Comment on attachment 8981954 [details] [diff] [review]
Patch update removing dead code and unused functions

Review of attachment 8981954 [details] [diff] [review]:
-----------------------------------------------------------------

Nice start! Just a couple nits.

::: layout/base/nsLayoutUtils.cpp
@@ -517,4 @@
>  }
>  
>  bool
> -nsLayoutUtils::CSSFiltersEnabled()

You also need to remove the declaration from nsLayoutUtils.h

::: layout/style/test/property_database.js
@@ +6451,5 @@
>  }
>  
>  
> +
> +gCSSProperties["filter"] = {

This should move to the object itself, that is with the syntax:

 "filter": {
    // ...
  },

Somewhere up.

Comment 20

a year ago
  Removing the function from the .h file causes the build to fail. It exits saying that there are 5 warnings.

Comment 21

a year ago
I think its because I am not correctly moving it to the object, do I just copy it and paste it in "filter"?
(Reporter)

Comment 22

a year ago
That should not make the build error, can you please attach a copy of the error? (and maybe an updated, work-in-progress patch?)

Thanks!

Comment 23

a year ago
I thought so, I am currently rebuilding it and I will try again and post a patch. Could you tell me how exactly I should move that into the object? Do i just copy and paste it in there?
(Reporter)

Comment 24

a year ago
No, you need to use Javascript's object syntax, that is, from:

==========

gCSSProperties = {
  "foo": {
    // ...
  },
  "bar": {
    // ...
  }
};

if (IsCSSPropertyPrefEnabled("layout.css.filters.enabled")) {
  gCSSProperties["filter"] = {
    // ...
  };
}

=========

to:

=========

gCSSProperties = {
  "foo": {
    // ...
  },
  "filter"]: {
    // ...
  },
  "bar": {
    // ...
  }
};

========


The contents inside the braces should remain the same, with the indentation fixed.

Comment 25

a year ago
Alright just got it done please take a look at it again, I had no javascript experience coming into this.

Comment 26

a year ago
Posted patch bug1465116.patch (obsolete) — Splinter Review

Comment 27

a year ago
Hey, How does it look? Also do you know if I  can work on two bugs together cause I would be committing changes for both.
(Reporter)

Comment 28

a year ago
Comment on attachment 8982093 [details] [diff] [review]
bug1465116.patch

Review of attachment 8982093 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for missing your previous email! please use the "review" flag on the attachment details in bugzilla so I don't forget about it next time!

Also, you need to include your previous changes in the patches.

::: layout/style/test/property_database.js
@@ +6444,4 @@
>  
>  
>  
> +gCSSProperties = {

This is still not right unfortunately. You need to remove all the `gCSSProperties["filter"]` block, and put it up there where the lines you just removed were.

Comment 29

a year ago
Are my previous changes not included? How do I include them?
(Reporter)

Comment 30

a year ago
You can see which changes are included in the patch opening it with the text editor. I don't know how to merge patches in mercurial unfortunately :(

Comment 31

a year ago
I found the other blocks which are in If.. else statements, I am not sure where I would put them, also previous changes are present locally.

Comment 33

a year ago
For merging the patches I simply appended one file to the other seemed to work. I am sure there is a better way.
(Reporter)

Comment 34

a year ago
Comment on attachment 8982573 [details] [diff] [review]
Patch including previous changes

Review of attachment 8982573 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, pretty sure there's a better way to merge patches, I'd ask in #introduction.

But anyway, property_database.js still needs fixing. I'll update a patch with the changes that are required ASAP.

::: layout/style/test/property_database.js
@@ +6466,5 @@
> +    "opacity(50%) saturate(1.0)",
> +    "invert(50%) sepia(0.1) brightness(90%)",
> +
> +    // Mixed SVG reference filters and filter functions
> +    "grayscale(1) url(#my-filter-1)",

This file is appearing twice now, which shouldn't happen.

But it's still not quite right, let me post a patch for the property_database.js changes.
Attachment #8982573 - Flags: review?(emilio)
(Reporter)

Comment 35

a year ago
Attachment #8981945 - Attachment is obsolete: true
Attachment #8981954 - Attachment is obsolete: true
Attachment #8982093 - Attachment is obsolete: true

Comment 36

a year ago
Hey, so what is left for me to do again? I see that you have all my changes in your patch as well.

Comment 37

a year ago
I am still unable to find a way to merge patches.Is there anything else I can help with?
I just landed bug 1408841 which does the same thing before noticing there's some progress in this bug. Sorry for that.
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1408841
You need to log in before you can comment on or make changes to this bug.