1418899 - move some static methods out of nsRuleNode

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Cameron McCormack (:heycam)]

We need to keep calling these methods after removing the old style system.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1418899 - Part 1: Remove some unused method declarations on nsRuleNode.

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

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1418899 - Part 2: Move some font-related static methods out of nsRuleNode.

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

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1418899 - Part 3: Use a single image layer array filling function.

We were accidentally using the background one for the mask layers list anyway,
and I don't think the overhead of filling the arrays for the two properties
mask layers don't use is a problem.

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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: style: Update image layer filling function call.

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

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Bug 1418899 - Part 4: Move image layer filling function out of nsRuleNode.

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

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8929987 [details]
Bug 1418899 - Part 2: Move some font-related static methods out of nsRuleNode.

https://reviewboard.mozilla.org/r/201170/#review206266


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: layout/base/nsLayoutUtils.cpp:10167
(Diff revision 1)
> +  float devPerCSS =
> +    (float)nsPresContext::AppUnitsPerCSSPixel() /
> +    aPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom();
> +  nsAutoString systemFontName;
> +  if (LookAndFeel::GetFont(aFontID, systemFontName, fontStyle, devPerCSS)) {
> +    systemFontName.Trim("\"'");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    systemFontName.Trim("\"'");
                        ^
                        R"("')"

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8929986 [details]
Bug 1418899 - Part 1: Remove some unused method declarations on nsRuleNode.

https://reviewboard.mozilla.org/r/201168/#review206280

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8929987 [details]
Bug 1418899 - Part 2: Move some font-related static methods out of nsRuleNode.

https://reviewboard.mozilla.org/r/201170/#review206284

::: layout/base/nsLayoutUtils.h:3058
(Diff revision 1)
>  
>    // Return the default value to be used for -moz-control-character-visibility,
>    // from preferences.
>    static uint8_t ControlCharVisibilityDefault();
>  
> +  enum class FlushUserFontSet {

Nit: While we're here, perhaps make this uint8_t to save some memory?

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8929987 [details]
Bug 1418899 - Part 2: Move some font-related static methods out of nsRuleNode.

https://reviewboard.mozilla.org/r/201170/#review206284

> Nit: While we're here, perhaps make this uint8_t to save some memory?

It probably doesn't make much difference, since we only use this as an argument value, so we'll be pushing a whole word on the stack anyway.

Comment 10

[u459114]

Comment on attachment 8929988 [details]
Bug 1418899 - Part 3: Use a single image layer array filling function.

https://reviewboard.mozilla.org/r/201172/#review206290

Comment 11

[u459114]

Comment on attachment 8929990 [details]
Bug 1418899 - Part 4: Move image layer filling function out of nsRuleNode.

https://reviewboard.mozilla.org/r/201176/#review206292

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8929986 [details]
Bug 1418899 - Part 1: Remove some unused method declarations on nsRuleNode.

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

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8929987 [details]
Bug 1418899 - Part 2: Move some font-related static methods out of nsRuleNode.

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

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8929988 [details]
Bug 1418899 - Part 3: Use a single image layer array filling function.

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

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8929990 [details]
Bug 1418899 - Part 4: Move image layer filling function out of nsRuleNode.

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

Comment 16

[Code Review Bot [:reviewbot]]

Comment on attachment 8929987 [details]
Bug 1418899 - Part 2: Move some font-related static methods out of nsRuleNode.

https://reviewboard.mozilla.org/r/201170/#review206666


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: layout/base/nsLayoutUtils.cpp:10167
(Diff revision 2)
> +  float devPerCSS =
> +    (float)nsPresContext::AppUnitsPerCSSPixel() /
> +    aPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom();
> +  nsAutoString systemFontName;
> +  if (LookAndFeel::GetFont(aFontID, systemFontName, fontStyle, devPerCSS)) {
> +    systemFontName.Trim("\"'");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    systemFontName.Trim("\"'");
                        ^
                        R"("')"

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59639cc071fd
Part 1: Remove some unused method declarations on nsRuleNode. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/58773b3c8b85
Part 2: Move some font-related static methods out of nsRuleNode. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/2d6409b7bbfd
Part 3: Use a single image layer array filling function. r=cjku
https://hg.mozilla.org/integration/autoland/rev/3f7025b5d57e
Part 4: Move image layer filling function out of nsRuleNode. r=cjku

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71b0dd5f14d3
Followup speculative Windows build fix. (CLOSED TREE)

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/59639cc071fd
https://hg.mozilla.org/mozilla-central/rev/58773b3c8b85
https://hg.mozilla.org/mozilla-central/rev/2d6409b7bbfd
https://hg.mozilla.org/mozilla-central/rev/3f7025b5d57e
https://hg.mozilla.org/mozilla-central/rev/71b0dd5f14d3