Nick Cameron [:nrc] Assignee
	Description • 12 years ago

Remove gfxFont (a Thebes class) from Azure.

We will do this by using a struct which describes the properties of a font only. This is kind of a temporary solution, hopefully it will be replaced by Bug 738014

	Bas Schouten (:bas.schouten)
	Comment 2 • 12 years ago

Comment on attachment 621482 [details] [diff] [review]
patch

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

Why can't the gfxFont stuff just go into gfxFT2Fonts? And we can then get rid of a -lot- of these #ifdefs.

::: gfx/2d/2D.h
@@ +505,5 @@
> + * Describes a font
> + * Used to pass the key informatin from a gfxFont into Azure
> + * XXX Should be replaced by a more long term solution, perhaps Bug 738014
> + */
> +struct GFX2D_API FontOptions

No need for GFX2D_API here I think, we're not exposing any symbols.

::: gfx/2d/Types.h
@@ +110,5 @@
> +  FONT_STYLE_ITALIC,
> +  FONT_STYLE_BOLD,
> +  FONT_STYLE_BOLD_ITALIC
> +};
> +#endif

I don't think we need an #ifdef here.

	Nick Cameron [:nrc] Assignee
	Comment 3 • 12 years ago

(In reply to Bas Schouten (:bas) from comment #2)
> Comment on attachment 621482 [details] [diff] [review]
> patch
> 
> Review of attachment 621482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why can't the gfxFont stuff just go into gfxFT2Fonts? And we can then get
> rid of a -lot- of these #ifdefs.
> 
I thought the whole idea was to get rid of dependencies on Thebes, if we put this lot in gfxFT2Fonts, then don't we need a dependency from Azure to gfxFT2Fonts?

> ::: gfx/2d/2D.h
> @@ +505,5 @@
> > + * Describes a font
> > + * Used to pass the key informatin from a gfxFont into Azure
> > + * XXX Should be replaced by a more long term solution, perhaps Bug 738014
> > + */
> > +struct GFX2D_API FontOptions
> 
> No need for GFX2D_API here I think, we're not exposing any symbols.
> 
Will remove it.

> ::: gfx/2d/Types.h
> @@ +110,5 @@
> > +  FONT_STYLE_ITALIC,
> > +  FONT_STYLE_BOLD,
> > +  FONT_STYLE_BOLD_ITALIC
> > +};
> > +#endif
> 
> I don't think we need an #ifdef here.

We don't need to, but this is only used by stuff which is also ifdefed, so I thought I should ifdef it too, otherwise it gets built but not ever used.

	Nick Cameron [:nrc] Assignee
	Comment 4 • 12 years ago

Try run: https://tbpl.mozilla.org/?tree=Try&rev=b2b3dbd3193d

	Nick Cameron [:nrc] Assignee
	Comment 5 • 12 years ago

Made requested changes

	Bas Schouten (:bas.schouten)
	Comment 6 • 12 years ago

Comment on attachment 623030 [details] [diff] [review]
updated patch

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

I've been thinking, if we require Freetype support on GTK and Android. Let's just for GTK2 and Android define MOZ_ENABLE_FREETYPE with a DEFINES addition inside gfx/2d/Makefile.in and only build the file there, and save ourselves clogging up the build system with MOZ_ENABLE_FREETYPE. As far as I can tell the #ifdefs inside gfxAndroidPlatform and gfxPlatformGtk can just go since that is an unsupported build configuration anyway if I understand the errors correctly.

	Nick Cameron [:nrc] Assignee
	Comment 7 • 12 years ago

I think this is what you want. The gfxPlatform classes are already only built when Freetype is on, so no changes to the build is needed, just removed the ifdefs. Also had to shuffle a few things around to handle Pango fonts. We still need MOZ_ENABLE_FREETYPE for 2d.h, to avoid exposing string stuff on mac.

	Nick Cameron [:nrc] Assignee
	Comment 8 • 12 years ago

Try run for the build: https://tbpl.mozilla.org/?tree=Try&rev=d74ecdf282f4 and another with a slightly different build, but the same code to pass the tests: https://tbpl.mozilla.org/?tree=Try&rev=ee154a754a80

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/303684497400

	Ed Morley [:emorley]
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/303684497400

	Landry Breuil (:gaston)
	Comment 12 • 12 years ago

It seems that broke the build on OpenBSD/gcc 4.2.1, see http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/401/steps/build/logs/stdio.

gfx/thebes/gfxFT2FontBase.cpp:234: error: 'mozilla::gfx::FontStyle' is not a class or namespace

i'm pretty sure adding 'using namespace mozilla::gfx;' before using the enum will fix it, but not sure it's the best way to workaround that. Thoughts ?

	Landry Breuil (:gaston)
	Comment 13 • 12 years ago

And apparently directly calling enum values from a distant namespace is a c++11 feature. Adding 'using namespace mozilla::gfx;' fixes the issue here.

	Nick Cameron [:nrc] Assignee
	Comment 14 • 12 years ago

Comment on attachment 626718 [details] [diff] [review]
use mozilla::gfx namespace instead of directly using an enum value

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

I'm pretty sure this is fine, but I don't have enough experience with the code-base to be comfortable r+ing it, sorry, so I've asked Bas to have a quick look at it too.

Thanks for fixing this! And I did not know that was a C++11 feature, I've learned something :-)

	Landry Breuil (:gaston)
	Comment 15 • 12 years ago

(In reply to Nick Cameron [:nrc] from comment #14)

> Thanks for fixing this! And I did not know that was a C++11 feature, I've
> learned something :-)

I didn't either :)

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6747b994ba00

	Ed Morley [:emorley]
	Comment 17 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/6747b994ba00

	neil@parkwaycc.co.uk
	Comment 18 • 12 years ago

Comment on attachment 624681 [details] [diff] [review]
updated patch

> RefPtr<ScaledFont>
> gfxPlatformGtk::GetScaledFontForFont(gfxFont *aFont)
> {
>-  NativeFont nativeFont;
>-  nativeFont.mType = NATIVE_FONT_SKIA_FONT_FACE;
>-  nativeFont.mFont = aFont;
>-  RefPtr<ScaledFont> scaledFont =
>-    Factory::CreateScaledFontForNativeFont(nativeFont, aFont->GetAdjustedSize());
>+    NS_ASSERTION(aFont->GetType() == gfxFont::FontType::FONT_TYPE_FT2, "Expecting Freetype font");
>+    NativeFont nativeFont;
>+    nativeFont.mType = NATIVE_FONT_SKIA_FONT_FACE;
>+    nativeFont.mFont = static_cast<gfxFT2FontBase*>(aFont)->GetFontOptions();
>+    RefPtr<ScaledFont> scaledFont =
>+      Factory::CreateScaledFontForNativeFont(nativeFont, aFont->GetAdjustedSize());
> 
>-  return scaledFont;
>+    return scaledFont;
> }
I don't know how this ever worked before, but gfxPlatformGtk doesn't actually include gfxFont.h, which means that the assertion fails to compile if you build with freetype instead of pango.

	neil@parkwaycc.co.uk
	Comment 19 • 12 years ago

(In reply to comment #18)
> (From update of attachment 624681 [details] [diff] [review])
> >+    NS_ASSERTION(aFont->GetType() == gfxFont::FontType::FONT_TYPE_FT2, "Expecting Freetype font");
Actually I misread the compiler error message. There is no such value as gfxFont::FontType::FONT_TYPE_FT2, because FontType is an enum, and therefore the constants actually exist on gfxFont, i.e. gfxFont::FONT_TYPE_FT2.

	Nick Cameron [:nrc] Assignee
	Comment 21 • 12 years ago

(In reply to neil@parkwaycc.co.uk from comment #19)
> (In reply to comment #18)
> > (From update of attachment 624681 [details] [diff] [review])
> > >+    NS_ASSERTION(aFont->GetType() == gfxFont::FontType::FONT_TYPE_FT2, "Expecting Freetype font");
> Actually I misread the compiler error message. There is no such value as
> gfxFont::FontType::FONT_TYPE_FT2, because FontType is an enum, and therefore
> the constants actually exist on gfxFont, i.e. gfxFont::FONT_TYPE_FT2.

Patch should fix this. Most compilers seem to accept the long version, but not sure if it is correct c++, the only references I could find specify the short version, but none forbid the long.

	Nick Cameron [:nrc] Assignee
	Comment 22 • 12 years ago

Try push (build only): https://tbpl.mozilla.org/?tree=Try&rev=8bbf0303a1fe

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 23 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4b57d0e84255