Closed Bug 1418899 Opened 3 years ago Closed 3 years ago

move some static methods out of nsRuleNode

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(4 files, 1 obsolete file)

We need to keep calling these methods after removing the old style system.
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 on attachment 8929986 [details]
Bug 1418899 - Part 1: Remove some unused method declarations on nsRuleNode.

https://reviewboard.mozilla.org/r/201168/#review206280
Attachment #8929986 - Flags: review?(tlin) → review+
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?
Attachment #8929987 - Flags: review?(tlin) → review+
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 on attachment 8929988 [details]
Bug 1418899 - Part 3: Use a single image layer array filling function.

https://reviewboard.mozilla.org/r/201172/#review206290
Attachment #8929988 - Flags: review?(cku) → review+
Comment on attachment 8929990 [details]
Bug 1418899 - Part 4: Move image layer filling function out of nsRuleNode.

https://reviewboard.mozilla.org/r/201176/#review206292
Attachment #8929990 - Flags: review?(cku) → review+
Attachment #8929989 - Flags: review?(cku) → review+
Attachment #8929989 - Attachment is obsolete: true
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"("')"
Pushed by cmccormack@mozilla.com:
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
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71b0dd5f14d3
Followup speculative Windows build fix. (CLOSED TREE)
You need to log in before you can comment on or make changes to this bug.