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 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 2011-05-29 06:07:42 PDT
Created attachment 535924 [details] [diff] [review]
Proposed patch
Comment 2 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 2011-06-09 13:32:39 PDT
Pushed changeset 322d3d456f5b to mozilla-central with the change as requested.
Comment 4 :Ehsan Akhgari 2011-06-09 14:04:22 PDT
Backed out because of red on Windows desktop mobile build:
Comment 5 2011-06-09 14:12:24 PDT
Not worth my while checking things in any more, some other loser can do it.
Comment 6 Daniel Holbert [:dholbert] 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 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 Daniel Holbert [:dholbert] 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 Daniel Holbert [:dholbert] 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 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 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 Daniel Holbert [:dholbert] 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.