URLValueData.mString should just be a nsString

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

The |mString| member of |URLValueData| should probably just be a |nsString| rather than a |RefPtr<nsStringBuffer>|. This would avoid unnecessary conversions between nsStringBuffer and nsString. Additionally we can speed up anything that is going to want to know the length of the string such as equality tests and string building.

AFAICT there is no usage where we construct a URLValueData from an existing nsStringBuffer, we always convert from an nsString.

[1] http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/layout/style/nsCSSValue.h#164
Bobby, I can see this being relevant to perf so you might want to be in the loop.
Xidorn, is there any reason we did things this way?
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(xidorn+moz) → needinfo?(cam)
Are those conversions really expensive?  It seems like we would get the same amount of string buffer sharing whether we are using nsString or manually using nsStringBuffer.  But I think it would be fine to switch to nsString.
Flags: needinfo?(cam)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> Xidorn, is there any reason we did things this way?

I think it's just a historical quirk of CSS that we're using a string buffer.

(In reply to Cameron McCormack (:heycam) from comment #3)
> Are those conversions really expensive?  It seems like we would get the same
> amount of string buffer sharing whether we are using nsString or manually
> using nsStringBuffer.  But I think it would be fine to switch to nsString.

The problem is you have to calculate the string length when using a string buffer. Regardless, I have a patch ready.
Created attachment 8857741 [details] [diff] [review]
Part 1: Just use nsString in URLValueData

This switches over from using nsStringBuffer to nsString for URLValueData's
|mString| member. This avoids various tedious conversions and can provide
potential performance improvements by avoiding length calculations.

MozReview-Commit-ID: 5eRifUZrAso
Attachment #8857741 - Flags: review?(cam)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8857741 [details] [diff] [review]
Part 1: Just use nsString in URLValueData

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

Looks good, thanks.

::: layout/style/nsCSSParser.cpp
@@ +8195,1 @@
>    // Note: urlVal retains its own reference to |buffer|.

This comment can be removed.

::: layout/style/nsCSSValue.cpp
@@ +2840,5 @@
>  bool
>  css::URLValueData::DefinitelyEqualURIs(const URLValueData& aOther) const
>  {
>    return mExtraData->BaseURI() == aOther.mExtraData->BaseURI() &&
> +         (mString == aOther.mString);

Nit: no parens needed.
Attachment #8857741 - Flags: review?(cam) → review+

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f790984c97f
https://hg.mozilla.org/mozilla-central/rev/ca8c662c97fc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.