Fix nsComputedDOMStyle.cpp's AppendEscapedCSSString calls to prefer local nsAutoString instead of local nsString for output parameter

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: wizard_A, Mentored)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
nsStyleUtil::AppendEscapedCSSString does what it says -- it appends to a given string.

nsComputedDOMStyle.cpp has a bunch of places where we call this function, and we pass in a local "nsString" variable as the outparam. This is sub-optimal, because nsString will need to do a heap allocation once you start writing to it (which takes some time).  We should use nsAutoString instead of nsString for these local variables instead.  (nsAutoString is like nsString, but it brings along a small stack-allocated buffer that it can write to quickly.)

So: I'm filing this mentored bug on going through the calls to  nsStyleUtil::AppendEscapedCSSString in nsComputedDOMStyle.cpp [1], and for any that use a local nsString as their result, change that variable's type to nsAutoString instead.

[1] http://mxr.mozilla.org/mozilla-central/search?string=AppendEscapedCSSString&find=nsComputedDOMStyle.cpp
(Reporter)

Updated

2 years ago
Whiteboard: [lang=c++]
(Reporter)

Updated

2 years ago
Summary: Fix nsComputedDOMStyle.cpp's AppendEscapedCSSString calls to prefer nsAutoString for output parameter → Fix nsComputedDOMStyle.cpp's AppendEscapedCSSString calls to prefer local nsAutoString instead of local nsString for output parameter
(Assignee)

Comment 1

2 years ago
Hi Daniel,

I would like to work on this bug.
Just to confirm I only need touch the initialization of the local variables being passed in the AppendCSSString() (11 times) in the nsComputedDOMStyle.cpp file.

Am I interpreting the bug in the right way??

Thanks,
Amit.
Flags: needinfo?(dholbert)
(Reporter)

Comment 2

2 years ago
Hi Amit -- thanks!

Yes, the local variable that's passed to those AppendCSSString calls should change to have type nsAutoString instead of nsString. (In some cases it's already nsAutoString. And in some cases it might not be a local variable -- I'm not sure.  So not all 11 calls will necessarily need a change.)

If you haven't already done so, you'll want to compile a local build of Firefox following the instructions here:
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

...and you'll want to generate a patch file with your changes, as described here:
 https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

(and post it as an attachment on this bug)

If you have any questions or get stuck, you can ask for help in irc.mozilla.org #introduction or #developers, or feel free to ask questions here as well.
Assignee: nobody → amitforfriends_dns
Flags: needinfo?(dholbert)
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8740116 [details] [diff] [review]
Bug-1262646_changed_nsString_to_nsAutoString.patch

Hi Daniel,

Requesting your review for the changes made.

Thanks,
Amit.
Attachment #8740116 - Flags: review?(amitforfriends_dns)
(Reporter)

Comment 4

2 years ago
Comment on attachment 8740116 [details] [diff] [review]
Bug-1262646_changed_nsString_to_nsAutoString.patch

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

Thanks for the patch! This looks mostly good. r- for now, but I'll grant r+ with the following addressed.

Firstly: you should add "unified=8" to the [diff] section of your .hgrc file, so that you'll be generating patches with 8 lines of context instead of the default 3.  That makes it a bit easier to see what's going on.  (This is included in the sample .hgrc file at https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F )

Secondly, a few changes for your commit message:
> Bug 1262646: Changed the type of the local vars being passed to nsStyleUtil::AppendEscapedCSSString from nsString to nsAutoString. r = Daniel Holbert

 (1) s/Changed/Change/  (We tend to use the present tense in commit messages)
 (2) s/local vars being passed to/outparams passed to/  (The point here is that we're changing the variables that this function writes to -- its outparams)
 (3) End it with r=dholbert  (no spaces, and just using abbreviated IRC nickname)

Finally, there are two code-changes here that don't belong:

::: layout/style/nsComputedDOMStyle.cpp
@@ +3435,5 @@
>      nsString type;
>      StyleList()->GetListStyleType(type);
>      nsStyleUtil::AppendEscapedCSSIdent(type, tmp);
>    } else if (anonymous->IsSingleString()) {
> +    const nsTArray<nsAutoString>& symbols = anonymous->GetSymbols();

The strings inside of this array don't fit the pattern of the things I want to change as part of this bug.  (They're not the thing that's being appended to in AppendEscapedCSSString -- they're providing source material.)

So -- please revert this change.

(I'd expect this change would cause a compile error, actually, because the type of this variable doesn't match what GetSymbols returns.  Did you check that a build with your patch compiles successfully?  Please give that a try before re-posting the final version.)

@@ +3455,4 @@
>        tmp.Append(' ');
>      }
>  
> +    const nsTArray<nsAutoString>& symbols = anonymous->GetSymbols();

(Above comment applies here as well -- please revert this change.)
Attachment #8740116 - Flags: review?(amitforfriends_dns) → review-
(Assignee)

Comment 5

2 years ago
Created attachment 8740365 [details] [diff] [review]
Bug-1262646_change_nsString_to_nsAutoString.patch

Hi Daniel,

Thanks a lot for those comments. Have tried to fix all of them.

Actually, regarding the string within the array, you're right.. the build fails..

Please review the patch and let me know if anything else is missing.

Thanks,
Amit.
Attachment #8740116 - Attachment is obsolete: true
Attachment #8740365 - Flags: review?(dholbert)
(Reporter)

Comment 6

2 years ago
Comment on attachment 8740365 [details] [diff] [review]
Bug-1262646_change_nsString_to_nsAutoString.patch

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

Looks great -- thanks! r=me

I've taken the liberty of submitting this to our TryServer, to get a sanity-check that it builds & doesn't break any tests in our mochitest suite. (We have other testsuites as well, but mochitests should be sufficient to exercise all of the code that you're modifying in this bug). Results will appear here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ab061f49f6

If everything's green* there when the run completes in a few hours, feel free to add the "checkin-needed" patch to this bug, and someone will land the patch on your behalf.

* (there might be a few orange test-results from known low-probability intermittent test failures. Those are easy to identify after a little practice -- #developers or #introduction can help with that.)

Thanks for the patch!
Attachment #8740365 - Flags: review?(dholbert) → review+
(Reporter)

Comment 7

2 years ago
More information on the Try Server here:
 https://wiki.mozilla.org/ReleaseEngineering/TryServer
(including how to get access, which you'll want to do after you've fixed a few bugs)
(Assignee)

Comment 8

2 years ago
Thank you Daniel for your guidance through my very first source code contribution to open source.

Looking forward for fixing some more bugs with your help and guidance.

Thanks a lot.... :)
Amit.
(Reporter)

Comment 9

2 years ago
You're very welcome! We're happy to have you as a contributor.

The Try run looks good, so I'll set this as "checkin-needed" -- and then it should get checked in within the next day or so.
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Reporter)

Comment 11

2 years ago
The patch applies correctly, with fuzz.

I'll just go ahead and land this to save us a few steps here.
(Reporter)

Comment 12

2 years ago
er, can't land because the tree's closed. I'll repost Amit's patch (rebased on current mozilla-inbound) and add checkin-needed back.
(Reporter)

Comment 13

2 years ago
Created attachment 8740667 [details] [diff] [review]
amit's patch, rebased to apply on mozilla-inbound [r=dholbert]
Attachment #8740365 - Attachment is obsolete: true
Attachment #8740667 - Flags: review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1d120b8dbb2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.