Remove some unused functions from nsComputedDOMStyle as well as their related data

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 3 bugs)

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(7 attachments)

I have some idea on how to harmonize some bits more.
Hmmm... it seems somehow gcc and clang cannot build with part 2...
Comment on attachment 8987799 [details]
Bug 1471114 part 1 - Move CSSPropFlags prefix generation into GenerateServoCSSPropList.py.

https://reviewboard.mozilla.org/r/253080/#review259610
Attachment #8987799 - Flags: review?(emilio) → review+
Comment on attachment 8987800 [details]
Bug 1471114 part 2 - Generate ComputedStyleMap entry list from property data.

https://reviewboard.mozilla.org/r/253082/#review259614

::: layout/style/GenerateComputedDOMStyleGenerated.py:23
(Diff revision 2)
> +
> +    def order_key(p):
> +        # Put prefixed properties after normal properties.
> +        # The spec is unclear about this, and Blink doesn't have any sensible
> +        # order at all, so it probably doesn't matter a lot. But originally
> +        # Gecko put then later so we do so as well.

Mind filing a spec issue to define this and reference it from here?

::: layout/style/GenerateComputedDOMStyleGenerated.py:27
(Diff revision 2)
> +        # order at all, so it probably doesn't matter a lot. But originally
> +        # Gecko put then later so we do so as well.
> +        order = p.name.startswith("-")
> +        return (order, p.name)
> +
> +    # Some special cases we may get rid of later.

Maybe worth filing a bug and reference it from here, just so we don't loose track of this?

::: layout/style/ServoCSSPropList.mako.py:103
(Diff revision 2)
>  
> +def exposed_on_getcs(prop):
> +    if prop.type() == "longhand":
> +        if is_internal(prop):
> +            return False
> +        # We currently don't expose logical properties in GetCS

Can you mention bug 1116638 here?

::: servo/components/style/properties/properties.mako.rs:838
(Diff revision 2)
>          /* The following flags are currently not used in Rust code, they
>           * only need to be listed in corresponding properties so that
>           * they can be checked in the C++ side via ServoCSSPropList.h. */
>          /// This property can be animated on the compositor.
>          const CAN_ANIMATE_ON_COMPOSITOR = 0;
> +        /// This shorthand property is accessable from getComputedStyle.

nit: `accessible`.
Attachment #8987800 - Flags: review?(emilio) → review+
Comment on attachment 8987801 [details]
Bug 1471114 part 3 - Drop the reference to getter functions we don't call.

https://reviewboard.mozilla.org/r/253084/#review259622

::: layout/style/GenerateComputedDOMStyleGenerated.py:50
(Diff revision 2)
>      for p in properties:
> -        output.write(TEMPLATE.format(p.id, method(p)))
> +        m = "DoGet" + method(p)
> +        # Put a dummy getter here instead of nullptr because MSVC seems
> +        # to have bug which ruins the table when we put nullptr for
> +        # pointer-to-member-function.
> +        m = "DummyGetter" if "SerializedByServo" in p.flags else m

Ugh...

::: layout/style/nsComputedDOMStyle.h:615
(Diff revision 2)
>    already_AddRefed<CSSValue> DoGetMaskType();
>    already_AddRefed<CSSValue> DoGetPaintOrder();
>  
>    already_AddRefed<CSSValue> DoGetContextProperties();
>  
> +  already_AddRefed<CSSValue> DummyGetter();

Maybe mention the MSVC comment here as well?

::: layout/style/nsComputedDOMStyle.cpp:7156
(Diff revision 2)
>  }
>  
> +already_AddRefed<CSSValue>
> +nsComputedDOMStyle::DummyGetter()
> +{
> +  MOZ_ASSERT_UNREACHABLE("not supposed to be invoked");

I'd just `MOZ_CRASH` here directly, but not that it matters.
Attachment #8987801 - Flags: review?(emilio) → review+
Comment on attachment 8987802 [details]
Bug 1471114 part 4 - Remove unused getter functions from nsComputedDOMStyle.

https://reviewboard.mozilla.org/r/253086/#review259626

::: commit-message-9d505:4
(Diff revision 2)
> +Bug 1471114 part 4 - Remove unused getter functions from nsComputedDOMStyle. r?emilio
> +
> +This is done with the following script:
> +```python

Maybe it's worth checkin-in this script until we've got rid of most of the others?

We probably want to use it each time we extend the properties serialized by Servo.

Or I guess I can just look up this commit :)
Attachment #8987802 - Flags: review?(emilio) → review+
Comment on attachment 8987802 [details]
Bug 1471114 part 4 - Remove unused getter functions from nsComputedDOMStyle.

https://reviewboard.mozilla.org/r/253086/#review259628

This is awesome btw :)
Comment on attachment 8987803 [details]
Bug 1471114 part 5 - Remove unused keyword tables.

https://reviewboard.mozilla.org/r/253088/#review259636
Attachment #8987803 - Flags: review?(emilio) → review+
Comment on attachment 8987804 [details]
Bug 1471114 part 6 - Remove unused CSS keywords.

https://reviewboard.mozilla.org/r/253090/#review259638
Attachment #8987804 - Flags: review?(emilio) → review+
Blocks: 1471423
Comment on attachment 8987800 [details]
Bug 1471114 part 2 - Generate ComputedStyleMap entry list from property data.

https://reviewboard.mozilla.org/r/253082/#review259614

> Mind filing a spec issue to define this and reference it from here?

https://github.com/w3c/csswg-drafts/issues/2827

> Maybe worth filing a bug and reference it from here, just so we don't loose track of this?

Bug 1471423
Blocks: 1471426
Comment on attachment 8987801 [details]
Bug 1471114 part 3 - Drop the reference to getter functions we don't call.

https://reviewboard.mozilla.org/r/253084/#review259622

> Ugh...

I'll try to construct a reduced testcase and probably file an issue to MS. Filed bug 1471426 for now.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9773c4edc6a6
part 1 - Move CSSPropFlags prefix generation into GenerateServoCSSPropList.py. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6bf8c652f3
part 2 - Generate ComputedStyleMap entry list from property data. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/787a0e28e114
part 3 - Drop the reference to getter functions we don't call. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/f53127621009
part 4 - Remove unused getter functions from nsComputedDOMStyle. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f78db4bdc11
part 5 - Remove unused keyword tables. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/14344ca5c162
part 6 - Remove unused CSS keywords. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b99a48a2b70
part 7 - Scripts used to generated the previous patches. r=emilio
Comment on attachment 8988003 [details]
Bug 1471114 part 7 - Scripts used to generated the previous patches.

https://reviewboard.mozilla.org/r/253264/#review259966

(I had reviewed this over IRC before Xidorn pushed it, but toggling the flag now for completeness)
Attachment #8988003 - Flags: review?(emilio) → review+
You need to log in before you can comment on or make changes to this bug.