Closed
Bug 1234773
Opened 9 years ago
Closed 9 years ago
build and parse preference style sheet as a single string
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(1 file)
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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).
Attachment #8701374 -
Flags: review?(dbaron)
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8701374 -
Flags: review?(dbaron) → review?(dholbert)
Comment 2•9 years ago
|
||
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
Attachment #8701374 -
Flags: review?(dholbert) → review+
Comment 5•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7513ac7a797e
https://hg.mozilla.org/mozilla-central/rev/0efc06a65e6a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•