Closed Bug 1471114 Opened 6 years ago Closed 6 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla63

Tracking Flags:

Tracking

Status

firefox63

---

fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 3 open bugs)

Details

Attachments

(7 files)

Bug 1471114 part 1 - Move CSSPropFlags prefix generation into GenerateServoCSSPropList.py. 6 years ago Xidorn Quan [:xidorn] UTC+10 59 bytes, text/x-review-board-request	emilio : review+	Details
Bug 1471114 part 2 - Generate ComputedStyleMap entry list from property data. 6 years ago Xidorn Quan [:xidorn] UTC+10 59 bytes, text/x-review-board-request	emilio : review+	Details
Bug 1471114 part 3 - Drop the reference to getter functions we don't call. 6 years ago Xidorn Quan [:xidorn] UTC+10 59 bytes, text/x-review-board-request	emilio : review+	Details
Bug 1471114 part 4 - Remove unused getter functions from nsComputedDOMStyle. 6 years ago Xidorn Quan [:xidorn] UTC+10 59 bytes, text/x-review-board-request	emilio : review+	Details
Bug 1471114 part 5 - Remove unused keyword tables. 6 years ago Xidorn Quan [:xidorn] UTC+10 59 bytes, text/x-review-board-request	emilio : review+	Details
Bug 1471114 part 6 - Remove unused CSS keywords. 6 years ago Xidorn Quan [:xidorn] UTC+10 59 bytes, text/x-review-board-request	emilio : review+	Details
Bug 1471114 part 7 - Scripts used to generated the previous patches. 6 years ago Xidorn Quan [:xidorn] UTC+10 59 bytes, text/x-review-board-request	emilio : review+	Details

Xidorn Quan [:xidorn] UTC+10

Assignee

Description

•

6 years ago

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 7

•

6 years ago

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Emilio Cobos Álvarez (:emilio)

Comment 13

•

6 years ago

mozreview-review

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+

Emilio Cobos Álvarez (:emilio)

Comment 14

•

6 years ago

mozreview-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+

Emilio Cobos Álvarez (:emilio)

Comment 15

•

6 years ago

mozreview-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+

Emilio Cobos Álvarez (:emilio)

Comment 16

•

6 years ago

mozreview-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+

Emilio Cobos Álvarez (:emilio)

Comment 17

•

6 years ago

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

Emilio Cobos Álvarez (:emilio)

Comment 18

•

6 years ago

mozreview-review

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+

Emilio Cobos Álvarez (:emilio)

Comment 19

•

6 years ago

mozreview-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+

Comment hidden (mozreview-request)

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

Xidorn Quan [:xidorn] UTC+10

Assignee

Updated

•

6 years ago

Blocks: 1471423

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 21

•

6 years ago

mozreview-review-reply

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

Xidorn Quan [:xidorn] UTC+10

Assignee

Updated

•

6 years ago

Blocks: 1471426

Xidorn Quan [:xidorn] UTC+10

Assignee

Comment 22

•

6 years ago

mozreview-review-reply

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Pulsebot

Comment 30

•

6 years ago

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

Emilio Cobos Álvarez (:emilio)

Comment 31

•

6 years ago

mozreview-review

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+

Dorel Luca [:dluca]

Comment 32

•

6 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox63: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla63

You need to log in before you can comment on or make changes to this bug.