660525 - Useless uses of PromiseFlatString

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[[e-mail]]

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

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Joe Drew (not getting mail)]

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

[[e-mail]]

Pushed changeset 322d3d456f5b to mozilla-central with the change as requested.

Comment 4

[(no longer active)]

Backed out because of red on Windows desktop mobile build:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1307650903.1307652099.19955.gz

Comment 5

[[e-mail]]

Not worth my while checking things in any more, some other loser can do it.

Comment 6

[Daniel Holbert [:dholbert]]

(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)

Comment 7

[[e-mail]]

Attachment: Updated for bitrot

Conveniently the contentious hunk was the one that bitrotted anyway.

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Daniel Holbert [:dholbert]]

(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

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b65e22bc0c28
http://hg.mozilla.org/mozilla-central/rev/078113c73963

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

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

Comment 12

[[e-mail]]

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]]

(In other words: Yes, comment 11 is sufficient for marking as verified. --> Marking as such.)