Closed Bug 660525 Opened 9 years ago Closed 9 years ago

Useless uses of PromiseFlatString

Categories

(Core :: Graphics, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #535924 - Flags: review?(joe)
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.
Attachment #535924 - Flags: review?(joe) → review+
Pushed changeset 322d3d456f5b to mozilla-central with the change as requested.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out because of red on Windows desktop mobile build:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1307650903.1307652099.19955.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Not worth my while checking things in any more, some other loser can do it.
Keywords: checkin-needed
(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:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1307650903.1307652792.22182.gz
> 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)
Keywords: checkin-needed
Conveniently the contentious hunk was the one that bitrotted anyway.
Attachment #535924 - Attachment is obsolete: true
Attachment #538372 - Flags: review+
Fixed newly-introduced bitrot, added committer & commit message metadata (please make sure to include these when using checkin-needed), & pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b65e22bc0c28
Whiteboard: [inbound]
(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:
http://hg.mozilla.org/integration/mozilla-inbound/rev/078113c73963
http://hg.mozilla.org/mozilla-central/rev/b65e22bc0c28
http://hg.mozilla.org/mozilla-central/rev/078113c73963
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Verified the files 

gfx/layers/opengl/LayerManagerOGLProgram.h
gfx/thebes/GLContext.cpp
gfx/thebes/gfxFontUtils.cpp
gfx/thebes/gfxGDIFontList.cpp

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

Thank you
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.
(In other words: Yes, comment 11 is sufficient for marking as verified. --> Marking as such.)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.