Closed Bug 1452542 Opened 6 years ago Closed 6 years ago

Generate property list from Servo data and use it to replace some uses of nsCSSPropList.h

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1452542 part 1 - Fix the order in nsCSSPropList.h.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1452542 part 2 - Sort nsCSSPropAliasList.h in alphabetic order so that we can list them in Servo side.
59 bytes, text/x-review-board-request
dholbert
: review+
emilio
: review+
Details
Bug 1452542 part 3 - Use snake_case naming for nsCSSPropertyID of alias as well.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1452542 part 4 - Have ENABLED_IN flags in nsCSSPropList.h match those in Servo side.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1452542 part 5 - Generate property list from Servo data.
59 bytes, text/x-review-board-request
emilio
: review+
froydnj
: review+
Details
Bug 1452542 part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1452542 part 7 - Replace some uses of nsCSSPropList.h and nsCSSPropAliasList.h with ServoCSSPropList.h.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Fixup patch
7.39 KB, patch
emilio
: review+
Details | Diff | Splinter Review 
We currently have prefs in both Servo side and Gecko side, which is not helpful. We should strip the gecko side one (because nsCSSPropList.h is gradually going away).

The easiest way forward is to have Servo code list the prefs with their corresponding property id, and Gecko reads the list in nsCSSProps::AddRefTable and add pref cache for items of gPropertyEnabled.

The initial data of nsCSSProps::gPropertyEnabled also relies on the pref in sense of whether one is enabled by default. We need to port that logic to Servo side somehow as well.
I'm going to explore this. Having prefs in two places is a problem for adding new properties.
Assignee: nobody → xidorn+moz
Depends on: 1453180
Depends on: 1453521
I'm trying to remove the pref string from the nsCSSPropList.h and have them provided from Servo side.

My current approach is to have gecko_properties.mako.rs generate a [no_mangle] static array, and have Gecko access that array rather than generate a table itself.

Everything seems to work well until I hit the generated WebIDL for CSS2Properties.

The problem here is that, WebIDL generating is pre-build, while the information is fed from Servo side can only be read from Gecko side in runtime. The WebIDL code is pretty smart for [Pref=], but not so for [Func=], so we probably cannot ask it to invoke a function for every attribute.

I don't quite have idea how to proceed then. Maybe I should instead have some script to generate list header from the data in Servo? We may eventually want to do this anyway?

heycam, what do you think?
Flags: needinfo?(cam)
Is there a way we can run mako before CSS2Properties.webidl is built, so we can extract the prefs and output them in GenerateCSS2PropertiesWebIDL.py?
Flags: needinfo?(cam)
Depends on: 1454297
Repurpose this bug to a wider topic.
Summary: Read list of property prefs from Servo side → Generate property list from Servo data and use it to replace some uses of nsCSSPropList.h
Blocks: 1448759
Blocks: 1454591
Try run mostly looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=242519366f4ce898560a985a7c1674cce89441b5

Most of the build failures above is addressed via moving the generating of ServoCSSPropList.{py,h} out from COMPILE_ENVIRONMENT: https://treeherder.mozilla.org/#/jobs?repo=try&revision=609b47abe2bfbf62556a37de720a5a6eaf6224b3

However, there is still one remaining failure on Btup with:
> tup error: Explicitly named file 'ServoCSSPropList.py' not found in subdir 'obj-firefox/layout/style'
(log: https://treeherder.mozilla.org/logviewer.html#?job_id=174041832&repo=try&lineNumber=16580)

mshal, do you have idea why Btup fails in this case and how should I fix it?
Flags: needinfo?(mshal)
Attachment #8968445 - Flags: review?(mh+mozilla) → review?(core-build-config-reviews)
Attachment #8968446 - Flags: review?(mh+mozilla) → review?(core-build-config-reviews)
Comment on attachment 8968441 [details]
Bug 1452542 part 1 - Fix the order in nsCSSPropList.h.

https://reviewboard.mozilla.org/r/237144/#review242932
Attachment #8968441 - Flags: review?(emilio) → review+
Comment on attachment 8968442 [details]
Bug 1452542 part 2 - Sort nsCSSPropAliasList.h in alphabetic order so that we can list them in Servo side.

https://reviewboard.mozilla.org/r/237146/#review242934

If we're going to start depending on this, should the different build script / generators assert this somehow?
Attachment #8968442 - Flags: review?(emilio) → review+
Comment on attachment 8968443 [details]
Bug 1452542 part 3 - Use snake_case naming for nsCSSPropertyID of alias as well.

https://reviewboard.mozilla.org/r/237148/#review242936
Attachment #8968443 - Flags: review?(emilio) → review+
Comment on attachment 8968444 [details]
Bug 1452542 part 4 - Have ENABLED_IN flags in nsCSSPropList.h match those in Servo side.

https://reviewboard.mozilla.org/r/237150/#review242940

::: layout/style/nsCSSPropList.h:3408
(Diff revision 1)
>  CSS_PROP_(
>      -moz-window-opacity,
>      _moz_window_opacity,
>      CSS_PROP_DOMPROP_PREFIXED(WindowOpacity),
> -    CSS_PROPERTY_INTERNAL | 0,
> +    CSS_PROPERTY_INTERNAL |
> +        CSS_PROPERTY_ENABLED_IN_UA_SHEETS_AND_CHROME,

We couldn't do this before, fwiw, because the old style system didn't transition them, see https://bugzilla.mozilla.org/show_bug.cgi?id=1419695.

Can you add these to test_non_content_accessible_properties.html?
Attachment #8968444 - Flags: review?(emilio) → review+
Comment on attachment 8968445 [details]
Bug 1452542 part 5 - Generate property list from Servo data.

https://reviewboard.mozilla.org/r/237152/#review242942

Ok, so I was sort of confused this didn't get rid of the nsCSSPropList file, but I see this is only an intermediate state.

I wonder if it'd be better to generate some sort of json file just using the standard python stuff instead of generating python code using mako... But I guess you already did the work and changing the setup here if needed is not a big deal.

Haven't reviewed the build system bits.

::: layout/style/GenerateServoCSSPropList.py:17
(Diff revision 1)
> +SERVO_BASE = mozpath.join(buildconfig.topsrcdir, "servo")
> +SERVO_PROP_BASE = mozpath.join(SERVO_BASE, "components", "style", "properties")
> +
> +
> +def generate_data(output, template):
> +    output.write("# THIS IS AN AUTOGENERATED FILE.  DO NOT EDIT\n\n")

Probably mention which file generates this?

::: layout/style/GenerateServoCSSPropList.py:25
(Diff revision 1)
> +        mozpath.join(SERVO_PROP_BASE, "build.py"),
> +        "gecko", "geckolib", template
> +    ], universal_newlines=True))
> +
> +    # Add all relevant files into the dependencies of the generated file.
> +    DEP_EXTS = [".py", ".rs", ".zip"]

Only .mako.rs, right? (Though that'd break the check I think, and we only have a few non-`mako.rs` in that dir, so probably it's fine).

::: layout/style/ServoCSSPropList.mako.py:1
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public

Wow, generating a python file with python, so meta.

::: layout/style/ServoCSSPropList.mako.py:9
(Diff revision 1)
> +
> +<%!
> +# nsCSSPropertyID of longhands and shorthands is ordered alphabetically
> +# with vendor prefixes removed. Note that aliases use their alias name
> +# as order key directly because they may be duplicate without prefix.
> +def order_key(prop):

Can we assert `not isinstance(prop, Alias)` or something like that?

::: layout/style/ServoCSSPropList.mako.py:19
(Diff revision 1)
> +def is_internal(prop):
> +    # A property which is not controlled by pref and not enabled in
> +    # content by default is an internal property.
> +    if not prop.gecko_pref and not prop.enabled_in_content():
> +        return True
> +    # There are some special cases we may want to remove eventually.

File bugs for these? Also I wonder if we should rename the "internal" properties, as of right now the only effect this has is not exposing them in CSSOM, which is not what "internal" usually means.

::: layout/style/ServoCSSPropList.mako.py:24
(Diff revision 1)
> +    # There are some special cases we may want to remove eventually.
> +    OTHER_INTERNALS = [
> +        "-moz-context-properties",
> +        "-moz-control-character-visibility",
> +    ]
> +    if prop.name in OTHER_INTERNALS:

Just `return prop.name in OTHER_INTERNALS`?
Attachment #8968445 - Flags: review?(emilio) → review+
Comment on attachment 8968447 [details]
Bug 1452542 part 7 - Replace some uses of nsCSSPropList.h and nsCSSPropAliasList.h with ServoCSSPropList.h.

https://reviewboard.mozilla.org/r/237156/#review242952

Looks good, thanks for working on this, can't wait to completely get rid of nsCSSPropList. For now having assertions about both matching is great.

::: dom/base/gen-usecounters.py:68
(Diff revision 1)
>    #define CSS_PROP_PUBLIC_OR_PRIVATE(publicname_, privatename_) privatename_
> -  #define CSS_PROP(name_, id_, method_, ...) \\
> +  // Need an extra level of macro nesting to force expansion of method_
> +  // params before they get pasted.
> +  #define CSS_PROP_USE_COUNTER(method_) \\
>      USE_COUNTER_FOR_CSS_PROPERTY_##method_ = eUseCounter_UNKNOWN,
> -  #include "nsCSSPropList.h"
> +  #define CSS_PROP_LONGHAND(name_, id_, method_, ...) \\

I guess this preserves behavior, but we probably eventually also want use counters for shorthand and aliases... Not that they work right now of course (https://bugzilla.mozilla.org/show_bug.cgi?id=1425700).
Attachment #8968447 - Flags: review?(emilio) → review+
Comment on attachment 8968442 [details]
Bug 1452542 part 2 - Sort nsCSSPropAliasList.h in alphabetic order so that we can list them in Servo side.

https://reviewboard.mozilla.org/r/237146/#review243034

Might be worth updating the commit message to mention the changes to properties-db.js too, so it's clearer that those aren't accidental. (Right now the commit message specifically just mentions one file, but touches 2.)

e.g. maybe add "(and regenerate properties-db.js)" at the end of commit message, or in a second line?

r=me regardless
Attachment #8968442 - Flags: review?(dholbert) → review+
Comment on attachment 8968447 [details]
Bug 1452542 part 7 - Replace some uses of nsCSSPropList.h and nsCSSPropAliasList.h with ServoCSSPropList.h.

https://reviewboard.mozilla.org/r/237156/#review242952

> I guess this preserves behavior, but we probably eventually also want use counters for shorthand and aliases... Not that they work right now of course (https://bugzilla.mozilla.org/show_bug.cgi?id=1425700).

Yes, this preserves behavior. Given that it doesn't work right now, let's have bug 1425700 or other followups to handle this.
Comment on attachment 8968445 [details]
Bug 1452542 part 5 - Generate property list from Servo data.

https://reviewboard.mozilla.org/r/237152/#review242942

> Probably mention which file generates this?

It seems it is pretty common to just leave this message without mentioning what generates it... I'd just follow the convention here. It shouldn't be too hard to find out given the filename, I suppose.

> Can we assert `not isinstance(prop, Alias)` or something like that?

It's tricky, because you don't have `Alias` class in the scope. I'm not convinced that check is very useful.
Comment on attachment 8968445 [details]
Bug 1452542 part 5 - Generate property list from Servo data.

https://reviewboard.mozilla.org/r/237152/#review242942

> Only .mako.rs, right? (Though that'd break the check I think, and we only have a few non-`mako.rs` in that dir, so probably it's fine).

Actually, I'm not too worried about being over conservative here, because output of scripts of generated files are checked before writing, which means if the content isn't really changed, the file wouldn't be touched, and thus its dependents wouldn't get rebuild.
Blocks: 1454831
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> However, there is still one remaining failure on Btup with:
> > tup error: Explicitly named file 'ServoCSSPropList.py' not found in subdir 'obj-firefox/layout/style'
> (log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=174041832&repo=try&lineNumber=16580)
> 
> mshal, do you have idea why Btup fails in this case and how should I fix it?

Thanks for reaching out! We'll fix this in the tup backend - you shouldn't need to update your patches for this. It looks like bugs 1454825 and 1454826 will get your patches working in Btup: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2691a518a29140f0b1c760ff1d45f92bb778fb30

Since the tup backend is tier-2 we don't need to make this bug blocked by those bugs, but they'll probably land soon.
Flags: needinfo?(mshal)
Blocks: 1281614
Attachment #8968445 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8968445 [details]
Bug 1452542 part 5 - Generate property list from Servo data.

https://reviewboard.mozilla.org/r/237152/#review243318

r/rs=me on the build parts.  I'm assuming that emilio reviewed that they DTRT.
Attachment #8968445 - Flags: review?(nfroyd) → review+
Comment on attachment 8968446 [details]
Bug 1452542 part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo.

https://reviewboard.mozilla.org/r/237154/#review243320

This looks like a great cleanup.

::: devtools/shared/css/generated/properties-db.js:9435
(Diff revision 1)
>  
>  /**
>   * A list of the preferences keys for whether a CSS property is enabled or not. This is
>   * exposed for testing purposes.
>   */
>  exports.PREFERENCES = [

Maybe as a followup, we should change the generating script to sort by name, so we don't have these sorts of shuffle-things-around changes later?

::: dom/bindings/moz.build:182
(Diff revision 1)
>      GENERATED_FILES += ['CSS2Properties.webidl']
>      css_props = GENERATED_FILES['CSS2Properties.webidl']
>      css_props.script = 'GenerateCSS2PropertiesWebIDL.py:generate'
>      css_props.inputs = [
>          '/dom/webidl/CSS2Properties.webidl.in',
> -        '/layout/style/PythonCSSProps.h',
> +        '!/layout/style/ServoCSSPropList.py',

The build system typically doesn't know about cross-directory dependencies like this unless we inform it...and I don't see anything in the backend that would explicitly state that `/layout/style` needs to be built prior to `/dom/bindings`.  If that cross-directory dependency hasn't caused problems yet, I think it's only a matter of time before something goes wrong.
Comment on attachment 8968446 [details]
Bug 1452542 part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo.

Canceling the build review for the issue noted.
Attachment #8968446 - Flags: review?(core-build-config-reviews)
Comment on attachment 8968446 [details]
Bug 1452542 part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo.

https://reviewboard.mozilla.org/r/237154/#review243320

> The build system typically doesn't know about cross-directory dependencies like this unless we inform it...and I don't see anything in the backend that would explicitly state that `/layout/style` needs to be built prior to `/dom/bindings`.  If that cross-directory dependency hasn't caused problems yet, I think it's only a matter of time before something goes wrong.

It probably doesn't depend on when is the directory get built, but on when are webidl generated related to include files exported. ServoCSSPropList.py is needed by ServoCSSPropList.h, which is exported in mozilla/, so it gets generated pretty early. I suppose webidl wouldn't be generated that early?

If this is a problem, how should I fix it? Would keeping the dependencies in Makefile.in help? (I have an impression that the dependencies in Makefile.in is not going to help either as far as we don't have cross-directory dependencies check, so this is probably a pre-existing issue which is unrelated to this change?)
Flags: needinfo?(nfroyd)
Oh, maybe it wasn't a pre-existing issue because the files were not generated.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #28)
> Oh, maybe it wasn't a pre-existing issue because the files were not
> generated.

This was my understanding from looking at the diff, but it's possible I might have missed a generated dependency!

There are two ways to fix it:

1) Add a rule in config/recurse.mk that makes layout/style/export depend on dom/bindings/export; see existing examples in the latter part of the file.
2) Make the processing of `GeneratedFile` in recursivemake.py generate such rules automatically if a generated file depends on an input in the object directory.

The latter is obviously more elegant and general, but is more work.  I think I'm happy to have you do #1 and file #2 as a followup.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #29)
> There are two ways to fix it:
> 
> 1) Add a rule in config/recurse.mk that makes layout/style/export depend on
> dom/bindings/export; see existing examples in the latter part of the file.

I suppose you mean making dom/bindings/export depend on layout/style/export?

> 2) Make the processing of `GeneratedFile` in recursivemake.py generate such
> rules automatically if a generated file depends on an input in the object
> directory.
> 
> The latter is obviously more elegant and general, but is more work.  I think
> I'm happy to have you do #1 and file #2 as a followup.

Thanks for the suggestion!
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #30)
> (In reply to Nathan Froyd [:froydnj] from comment #29)
> > There are two ways to fix it:
> > 
> > 1) Add a rule in config/recurse.mk that makes layout/style/export depend on
> > dom/bindings/export; see existing examples in the latter part of the file.
> 
> I suppose you mean making dom/bindings/export depend on layout/style/export?

Doh, of course!
Filed bug 1454953 for #2.
Comment on attachment 8968446 [details]
Bug 1452542 part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo.

https://reviewboard.mozilla.org/r/237154/#review243348

Thank you!
Attachment #8968446 - Flags: review?(nfroyd) → review+
Comment on attachment 8968446 [details]
Bug 1452542 part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo.

https://reviewboard.mozilla.org/r/237154/#review243348

Thanks for reviewing!
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fac55b1a0706
part 1 - Fix the order in nsCSSPropList.h. r=emilio
https://hg.mozilla.org/integration/autoland/rev/255e2feff19f
part 2 - Sort nsCSSPropAliasList.h in alphabetic order so that we can list them in Servo side. r=dholbert,emilio
https://hg.mozilla.org/integration/autoland/rev/e83a797bdc61
part 3 - Use snake_case naming for nsCSSPropertyID of alias as well. r=emilio
https://hg.mozilla.org/integration/autoland/rev/5619bba37cdd
part 4 - Have ENABLED_IN flags in nsCSSPropList.h match those in Servo side. r=emilio
https://hg.mozilla.org/integration/autoland/rev/4d3a028edaed
part 5 - Generate property list from Servo data. r=emilio,froydnj
https://hg.mozilla.org/integration/autoland/rev/be97e652391f
part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/f206acff283f
part 7 - Replace some uses of nsCSSPropList.h and nsCSSPropAliasList.h with ServoCSSPropList.h. r=emilio
Attached patch Fixup patch — — Splinter Review 
This is basically the reverse direction of part 4. In this patch we sync the flag from Gecko to Servo instead because it seems we still cannot do that, but we want to keep the flag sync.

In addition to the reverse of part 4, the list of OTHER_INTERNALS in part 5 also need update because those three are marked INTERNAL in Gecko while they don't match our general condition.
Flags: needinfo?(xidorn+moz)
Attachment #8969148 - Flags: review?(emilio)
Comment on attachment 8969148 [details] [diff] [review]
Fixup patch

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

Le sad... I think the only relevant bit for that is the "all" longhand avoiding transitioning non-content-exposed properties I guess. We'll see.

::: layout/style/nsCSSPropList.h
@@ +3403,5 @@
>  CSS_PROP_(
>      -moz-window-opacity,
>      _moz_window_opacity,
>      CSS_PROP_DOMPROP_PREFIXED(WindowOpacity),
> +    CSS_PROPERTY_INTERNAL | 0,

no need for | 0, right?

::: servo/components/style/properties/longhand/ui.mako.rs
@@ +40,5 @@
>                           animation_value_type="discrete",
>                           enabled_in="chrome",
>                           spec="None (Nonstandard internal property)")}
>  
> +// TODO This should be hidden from content.

TODO(bug XXX):

Here and below please :)
Attachment #8969148 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #46)
> Comment on attachment 8969148 [details] [diff] [review]
> Fixup patch
> 
> Review of attachment 8969148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Le sad... I think the only relevant bit for that is the "all" longhand
> avoiding transitioning non-content-exposed properties I guess. We'll see.
> 
> ::: layout/style/nsCSSPropList.h
> @@ +3403,5 @@
> >  CSS_PROP_(
> >      -moz-window-opacity,
> >      _moz_window_opacity,
> >      CSS_PROP_DOMPROP_PREFIXED(WindowOpacity),
> > +    CSS_PROPERTY_INTERNAL | 0,
> 
> no need for | 0, right?

No need, but I'm just reverting the corresponding code in part 4, and the | 0 here is some leftover from some previous refactor... so I'd just leave it untouched after squashing this into part 4 and part 5.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ef991e3b43a
part 1 - Fix the order in nsCSSPropList.h. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6c3096d42256
part 2 - Sort nsCSSPropAliasList.h in alphabetic order so that we can list them in Servo side. r=dholbert,emilio
https://hg.mozilla.org/integration/autoland/rev/3958926b4737
part 3 - Use snake_case naming for nsCSSPropertyID of alias as well. r=emilio
https://hg.mozilla.org/integration/autoland/rev/647d4e66d67a
part 4 - Have ENABLED_IN flags in nsCSSPropList.h match those in Servo side. r=emilio
https://hg.mozilla.org/integration/autoland/rev/4dbf4e6f1b2f
part 5 - Generate property list from Servo data. r=emilio,froydnj
https://hg.mozilla.org/integration/autoland/rev/cd54075da3d0
part 6 - Replace uses of PythonCSSProps.h with the data file generated from Servo. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/80d84e895939
part 7 - Replace some uses of nsCSSPropList.h and nsCSSPropAliasList.h with ServoCSSPropList.h. r=emilio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: