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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file)

Attached patch patch (deleted) — 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)
Attachment #8701374 - Flags: review?(dbaron) → review?(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
Attachment #8701374 - Flags: review?(dholbert) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: stylo
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: