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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

I have some idea on how to harmonize some bits more.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1471114 part 1 - Move CSSPropFlags prefix generation into GenerateServoCSSPropList.py.

Review commit: https://reviewboard.mozilla.org/r/253080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253080/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1471114 part 2 - Generate ComputedStyleMap entry list from property data.

This changes the order of properties returned from gCS. The old order
doesn't make much sense, and other browsers don't agree on an identical
order either, so it should be trivial to change it. Also the spec isn't
super clear / useful in this case.

Several -moz-prefixed properties are excluded from the list due to their
being internal. I suspect they are never accessed anyway, so probably
nothing gets changed by this.

Review commit: https://reviewboard.mozilla.org/r/253082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253082/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1471114 part 3 - Drop the reference to getter functions we don't call.

Review commit: https://reviewboard.mozilla.org/r/253084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253084/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1471114 part 4 - Remove unused getter functions from nsComputedDOMStyle.

This is done with the following script:
```python
#!/usr/bin/env python3

import re
import sys

from pathlib import Path

if len(sys.argv) != 2:
    print("Usage: {} objdir".format(sys.argv[0]))
    exit(1)

generated = Path(sys.argv[1]) / "layout" / "style"
generated = generated / "nsComputedDOMStyleGenerated.cpp"
RE_GENERATED = re.compile(r"DoGet\w+")
keeping = set()
with generated.open() as f:
    for line in f:
        m = RE_GENERATED.search(line)
        if m is not None:
            keeping.add(m.group(0))

HEADER = "layout/style/nsComputedDOMStyle.h"
SOURCE = "layout/style/nsComputedDOMStyle.cpp"

# We need to keep functions invoked by others
RE_DEF = re.compile(r"nsComputedDOMStyle::(DoGet\w+)\(\)")
RE_SRC = re.compile(r"\b(DoGet\w+)\(\)")
with open(SOURCE, "r") as f:
    for line in f:
        m = RE_DEF.search(line)
        if m is not None:
            continue
        m = RE_SRC.search(line)
        if m is not None:
            keeping.add(m.group(1))

removing = set()
remaining_lines = []
with open(HEADER, "r") as f:
    for line in f:
        m = RE_SRC.search(line)
        if m is not None:
            name = m.group(1)
            if name not in keeping:
                print("Removing " + name)
                removing.add(name)
                continue
        remaining_lines.append(line)

with open(HEADER, "w", newline="") as f:
    f.writelines(remaining_lines)

remaining_lines = []
is_removing = False
with open(SOURCE, "r") as f:
    for line in f:
        if is_removing:
            if line == "}\n":
                is_removing = False
            continue
        m = RE_DEF.search(line)
        if m is not None:
            name = m.group(1)
            if name in removing:
                remaining_lines.pop()
                if remaining_lines[-1] == "\n":
                    remaining_lines.pop()
                is_removing = True
                continue
        remaining_lines.append(line)

with open(SOURCE, "w", newline="") as f:
    f.writelines(remaining_lines)
```

Review commit: https://reviewboard.mozilla.org/r/253086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253086/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1471114 part 5 - Remove unused keyword tables.

This is done with the following script:
```python
#!/usr/bin/env python3

import re
import subprocess

from pathlib import Path

HEADER = Path("layout/style/nsCSSProps.h")
SOURCE = Path("layout/style/nsCSSProps.cpp")

RE_TABLE = re.compile(r"\b(k\w+KTable)")
rg_result = subprocess.check_output(["rg", r"\bk\w+KTable"], encoding="UTF-8")
to_keep = set()
all = set()
for item in rg_result.splitlines():
    file, line = item.split(':', 1)
    name = RE_TABLE.search(line).group(1)
    path = Path(file)
    if path != HEADER and path != SOURCE:
        to_keep.add(name)
    else:
        all.add(name)

to_remove = all - to_keep
remaining_lines = []
with HEADER.open() as f:
    for line in f:
        m = RE_TABLE.search(line)
        if m is not None and m.group(1) in to_remove:
            print("Removing " + m.group(1))
            continue
        remaining_lines.append(line)
with HEADER.open("w", newline="") as f:
    f.writelines(remaining_lines)

remaining_lines = []
removing = False
RE_DEF = re.compile(r"KTableEntry nsCSSProps::(k\w+KTable)\[\]")
with SOURCE.open() as f:
    for line in f:
        if removing:
            if line == "};\n":
                removing = False
            continue
        m = RE_DEF.search(line)
        if m is not None and m.group(1) in to_remove:
            if remaining_lines[-1] == "\n":
                remaining_lines.pop()
            removing = True
            continue
        remaining_lines.append(line)
with SOURCE.open("w", newline="") as f:
    f.writelines(remaining_lines)
```

Review commit: https://reviewboard.mozilla.org/r/253088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253088/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1471114 part 6 - Remove unused CSS keywords.

This is done with the following script:
```python
#!/usr/bin/env python3

import re
import subprocess

LIST_FILE = "layout/style/nsCSSKeywordList.h"

RE_KEYWORD = re.compile(r"\beCSSKeyword_(\w+)")
rg_result = subprocess.check_output(["rg", r"eCSSKeyword_\w+"], encoding="UTF-8")
to_keep = set()
for item in rg_result.splitlines():
    file, line = item.split(':', 1)
    for m in RE_KEYWORD.finditer(line):
        to_keep.add(m.group(1))

remaining_lines = []
RE_ITEM = re.compile(r"CSS_KEY\(.+, (\w+)\)")
with open(LIST_FILE, "r") as f:
    for line in f:
        m = RE_ITEM.search(line)
        if m is not None and m.group(1) not in to_keep:
            print("Removing " + m.group(1))
            continue
        remaining_lines.append(line)
with open(LIST_FILE, "w", newline="") as f:
    f.writelines(remaining_lines)
```

Review commit: https://reviewboard.mozilla.org/r/253090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253090/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... it seems somehow gcc and clang cannot build with part 2...

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987800 [details]
Bug 1471114 part 2 - Generate ComputedStyleMap entry list from property data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253082/diff/1-2/

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253084/diff/1-2/

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987802 [details]
Bug 1471114 part 4 - Remove unused getter functions from nsComputedDOMStyle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253086/diff/1-2/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987803 [details]
Bug 1471114 part 5 - Remove unused keyword tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253088/diff/1-2/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987804 [details]
Bug 1471114 part 6 - Remove unused CSS keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253090/diff/1-2/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8987799 [details]
Bug 1471114 part 1 - Move CSSPropFlags prefix generation into GenerateServoCSSPropList.py.

https://reviewboard.mozilla.org/r/253080/#review259610

Comment 14

[Emilio Cobos Álvarez (:emilio)]

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`.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8987803 [details]
Bug 1471114 part 5 - Remove unused keyword tables.

https://reviewboard.mozilla.org/r/253088/#review259636

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8987804 [details]
Bug 1471114 part 6 - Remove unused CSS keywords.

https://reviewboard.mozilla.org/r/253090/#review259638

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1471114 part 7 - Scripts used to generated the previous patches.

Review commit: https://reviewboard.mozilla.org/r/253264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253264/

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987800 [details]
Bug 1471114 part 2 - Generate ComputedStyleMap entry list from property data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253082/diff/2-3/

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253084/diff/2-3/

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987802 [details]
Bug 1471114 part 4 - Remove unused getter functions from nsComputedDOMStyle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253086/diff/2-3/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987803 [details]
Bug 1471114 part 5 - Remove unused keyword tables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253088/diff/2-3/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8987804 [details]
Bug 1471114 part 6 - Remove unused CSS keywords.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253090/diff/2-3/

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8988003 [details]
Bug 1471114 part 7 - Scripts used to generated the previous patches.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253264/diff/1-2/

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8988003 [details]
Bug 1471114 part 7 - Scripts used to generated the previous patches.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253264/diff/2-3/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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 31

[Emilio Cobos Álvarez (: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)

Comment 32

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9773c4edc6a6
https://hg.mozilla.org/mozilla-central/rev/7b6bf8c652f3
https://hg.mozilla.org/mozilla-central/rev/787a0e28e114
https://hg.mozilla.org/mozilla-central/rev/f53127621009
https://hg.mozilla.org/mozilla-central/rev/1f78db4bdc11
https://hg.mozilla.org/mozilla-central/rev/14344ca5c162
https://hg.mozilla.org/mozilla-central/rev/8b99a48a2b70