Useless uses of PromiseFlatString

VERIFIED FIXED in mozilla7

Status

()

Core
Graphics
--
trivial
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.70 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 535924 [details] [diff] [review]
Proposed patch
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+
(Assignee)

Comment 3

6 years ago
Pushed changeset 322d3d456f5b to mozilla-central with the change as requested.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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 → ---
(Assignee)

Comment 5

6 years ago
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
(Assignee)

Comment 7

6 years ago
Created attachment 538372 [details] [diff] [review]
Updated for bitrot

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
Last Resolved: 6 years ago6 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
(Assignee)

Comment 12

6 years ago
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.