Last Comment Bug 660525 - Useless uses of PromiseFlatString
: Useless uses of PromiseFlatString
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
-- trivial (vote)
: mozilla7
Assigned To:
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2011-05-29 06:06 PDT by
Modified: 2011-09-01 21:23 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed patch (5.67 KB, patch)
2011-05-29 06:07 PDT,
joe: review+
Details | Diff | Splinter Review
Updated for bitrot (4.70 KB, patch)
2011-06-09 15:57 PDT,
neil: review+
Details | Diff | Splinter Review

Description User image 2011-05-29 06:06:49 PDT
There's no point calling PromiseFlatString on a known flat string.
There's also little point calling it on a known substring, you might as well assign it directly into an nsAutoString.
Comment 1 User image 2011-05-29 06:07:42 PDT
Created attachment 535924 [details] [diff] [review]
Proposed patch
Comment 2 User image Joe Drew (not getting mail) 2011-06-09 11:21:12 PDT
Comment on attachment 535924 [details] [diff] [review]
Proposed patch

Review of attachment 535924 [details] [diff] [review]:

::: gfx/thebes/gfxGDIFontList.cpp
@@ +550,5 @@
>      LOGFONTW logFont;
>      memset(&logFont, 0, sizeof(LOGFONTW));
>      logFont.lfCharSet = DEFAULT_CHARSET;
>      logFont.lfPitchAndFamily = 0;
> +    PRUint32 l = PR_MIN(mName.Length() + 1, LF_FACESIZE);

If you can, can you split this out into a separate patch and get John Daggett to review it? It's not directly related to the nsPromiseFlatString work and I'm not confident in what its effects will be.
Comment 3 User image 2011-06-09 13:32:39 PDT
Pushed changeset 322d3d456f5b to mozilla-central with the change as requested.
Comment 4 User image :Ehsan Akhgari 2011-06-09 14:04:22 PDT
Backed out because of red on Windows desktop mobile build:
Comment 5 User image 2011-06-09 14:12:24 PDT
Not worth my while checking things in any more, some other loser can do it.
Comment 6 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-06-09 14:27:43 PDT
(In reply to comment #5)
> Not worth my while checking things in any more, some other loser can do it.

If I understand correctly, "some other loser" will hit the exact same problem when they check this in.  The red in comment 4 was due to an issue with this patch.

The failure was:
> gfx/thebes/gfxGDIFontList.cpp(438) : error C2039: 'get' : is not a member of 'nsAString_internal'
...which is a compile error on a line touched by this bug's patch. (the memcpy one, I believe)
Comment 7 User image 2011-06-09 15:57:41 PDT
Created attachment 538372 [details] [diff] [review]
Updated for bitrot

Conveniently the contentious hunk was the one that bitrotted anyway.
Comment 8 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-06-13 19:00:18 PDT
Fixed newly-introduced bitrot, added committer & commit message metadata (please make sure to include these when using checkin-needed), & pushed:
Comment 9 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-06-13 19:05:57 PDT
(In reply to comment #8)
> Fixed newly-introduced bitrot

odd -- I definitely qrefreshed after fixing that bitrot (I diffed the new patch against the patch posted here as a sanity-check), but somehow when I pushed, the pushed cset (in comment 8) ended up being empty.

I pushed again, and this one caught the code changes:
Comment 11 User image Mihaela Velimiroviciu (:mihaelav) 2011-08-30 01:02:15 PDT
Verified the files 


were updated in mozilla-central repository.
Is this enough to verify the fix?

Thank you
Comment 12 User image 2011-08-30 02:01:17 PDT
It's possible that the compiler can optimise the old code away anyway so there wouldn't actually be any other difference that you can check.
Comment 13 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-09-01 21:23:23 PDT
(In other words: Yes, comment 11 is sufficient for marking as verified. --> Marking as such.)

Note You need to log in before you can comment on or make changes to this bug.