Closed
Bug 1262646
Opened 8 years ago
Closed 8 years ago
Fix nsComputedDOMStyle.cpp's AppendEscapedCSSString calls to prefer local nsAutoString instead of local nsString for output parameter
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: dholbert, Assigned: wizard_A, Mentored)
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 2 obsolete files)
4.96 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Whiteboard: [lang=c++]
Reporter | ||
Updated•8 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•8 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•8 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•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
Hi Daniel, Requesting your review for the changes made. Thanks, Amit.
Attachment #8740116 -
Flags: review?(amitforfriends_dns)
Reporter | ||
Comment 4•8 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•8 years ago
|
||
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•8 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•8 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•8 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•8 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
Reporter | ||
Comment 11•8 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•8 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•8 years ago
|
||
Attachment #8740365 -
Attachment is obsolete: true
Attachment #8740667 -
Flags: review+
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d120b8dbb2
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d1d120b8dbb2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•