1234773 - build and parse preference style sheet as a single string

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: patch

Currently we build the preference style sheet by parsing each rule in the sheet and calling InsertRuleInternal for each.  Parsing the entire sheet as a string will help when we have other style system backends (where we might not yet have insertRule implemented on style sheet objects).

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f4fd8cfb6e0

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8701374 [details] [diff] [review]
patch

Review of attachment 8701374 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a valid conversion. r=me, nits below:

::: layout/style/nsLayoutStylesheetCache.cpp
@@ +830,5 @@
>    aSheet->SetURIs(uri, uri, uri);
>    aSheet->SetComplete();
>  
> +  nsString sheetText;
> +  sheetText.SetCapacity(1024);

Hmm, 1024 is kind of a magic number here.  It looks like it's just an optimization, to preallocate "~just enough" to avoid reallocing over and over as you append -- correct?  And the text is pretty much known ahead of time, so we can make a good guess at its length here, I guess.

Anyway -- could you define this as a constant, e.g.
   static const uint32_t kPreallocSize = 1024;
...and after we've finished building the string, add a non-fatal NS_ASSERTION (or at least a warning) to check that the final string-length is <= this preallocated size. (If the string ends up larger, then that means we've been potentially causing some memory churn on the last few appends, and we'd benefit from bumping up the preallocated size.)

@@ +857,5 @@
> +    aPresContext->GetCachedBoolPref(kPresContext_UnderlineLinks);
> +  sheetText.AppendPrintf(
> +      "*|*:-moz-any-link%s { text-decoration: %s; }\n",
> +      underlineLinks ? (const char*) ":not(svg|a)" : (const char*) "",
> +      underlineLinks ? (const char*) "underline" : (const char*) "none");

As noted in bug 1234758, I'm not sure why you'd need these (const char*) casts here & elsewhere in this patch.

I found at least one place in our tree where we call AppendPrintf with "%s" and with a string-literal without any casts:
http://hg.mozilla.org/mozilla-central/annotate/22f51211915b/layout/base/DisplayItemScrollClip.cpp#l34
...so if that hasn't broken the build, I don't see why they'd be needed here.

(I can imagine cases where a ternary expression might force us to cast its two different-looking branches to have the same type in order to be valid, but here, both branches of the ternary expression are already the same type...)

Anyway - please enlighten me on why they're needed, or drop them if they're not. :)

@@ +912,2 @@
>    }
> +  

drop whitespace on blank line

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7513ac7a797e

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0efc06a65e6a

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7513ac7a797e
https://hg.mozilla.org/mozilla-central/rev/0efc06a65e6a