891709 - gfx/2d/Factory.cpp wrong include logic？

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130706191431

Steps to reproduce:

http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Factory.cpp#22 
http://mxr.mozilla.org/mozilla-central/source/gfx/2d/Factory.cpp#288

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Can you explain what is wrong?

Comment 2

[[e-mail]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Can you explain what is wrong?

skia can be disabled, then ScaledFontWin.h will not be included, but it's required by line 291.

Comment 3

[[e-mail]]

Attachment: fix_disable-skia

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8416882 [details] [diff] [review]
fix_disable-skia

Review of attachment 8416882 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a peer of gfx. As harmless as this looks, I'm redirecting.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8416882 [details] [diff] [review]
fix_disable-skia

Review of attachment 8416882 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with this as well, but to be 100% sure we can use this outside of using Skia, redirecting.

Comment 6

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8416882 [details] [diff] [review]
fix_disable-skia

Review of attachment 8416882 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. All the Skia-specific code in ScaledFontWin is already guarded so we should be fine here.

Comment 7

[George Wright (:gw280) (needinfo me!)]

(That's not to say things will work, but it's better than the current situation where it potentially won't compile) :)

Comment 8

[George Wright (:gw280) (needinfo me!)]

Ugh, I should probably be more clear. I was replying to Bas's question about whether we can use ScaledFontWin objects with a backend that's not Skia (or at least, that's how I interpreted it?). I think we can, with the Cairo backend, as would be implied by Factory::CreateScaledFontForNativeFont(), but I can't say for sure as I'm not familiar with Cairo.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8539a2bf06

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b8539a2bf06