Last Comment Bug 752380 - [Azure][Skia] Refactor gfxFont out of Azure
: [Azure][Skia] Refactor gfxFont out of Azure
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on: 895431
Blocks: 751431
  Show dependency treegraph
 
Reported: 2012-05-06 17:11 PDT by Nick Cameron [:nrc]
Modified: 2013-07-18 09:08 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.99 KB, patch)
2012-05-06 17:16 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
updated patch (14.09 KB, patch)
2012-05-10 21:24 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
updated some more (14.55 KB, patch)
2012-05-14 14:26 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
updated patch (14.32 KB, patch)
2012-05-17 02:25 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Splinter Review
use mozilla::gfx namespace instead of directly using an enum value (1.97 KB, patch)
2012-05-24 00:35 PDT, Landry Breuil (:gaston)
bas: review+
ncameron: feedback+
Details | Diff | Splinter Review
correct the scope of enums (1.99 KB, patch)
2012-05-27 13:02 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-05-06 17:11:08 PDT
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
Comment 1 Nick Cameron [:nrc] 2012-05-06 17:16:07 PDT
Created attachment 621482 [details] [diff] [review]
patch
Comment 2 Bas Schouten (:bas.schouten) 2012-05-07 05:52:38 PDT
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.
Comment 3 Nick Cameron [:nrc] 2012-05-07 15:51:18 PDT
(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.
Comment 4 Nick Cameron [:nrc] 2012-05-07 17:32:16 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=b2b3dbd3193d
Comment 5 Nick Cameron [:nrc] 2012-05-10 21:24:20 PDT
Created attachment 623030 [details] [diff] [review]
updated patch

Made requested changes
Comment 6 Bas Schouten (:bas.schouten) 2012-05-10 22:39:30 PDT
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.
Comment 7 Nick Cameron [:nrc] 2012-05-14 14:26:30 PDT
Created attachment 623813 [details] [diff] [review]
updated some more

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.
Comment 8 Nick Cameron [:nrc] 2012-05-17 02:24:44 PDT
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
Comment 9 Nick Cameron [:nrc] 2012-05-17 02:25:33 PDT
Created attachment 624681 [details] [diff] [review]
updated patch
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-22 19:26:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/303684497400
Comment 11 Ed Morley [:emorley] 2012-05-23 04:51:02 PDT
https://hg.mozilla.org/mozilla-central/rev/303684497400
Comment 12 Landry Breuil (:gaston) 2012-05-23 23:27:08 PDT
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 ?
Comment 13 Landry Breuil (:gaston) 2012-05-24 00:35:38 PDT
Created attachment 626718 [details] [diff] [review]
use mozilla::gfx namespace instead of directly using an enum value

And apparently directly calling enum values from a distant namespace is a c++11 feature. Adding 'using namespace mozilla::gfx;' fixes the issue here.
Comment 14 Nick Cameron [:nrc] 2012-05-24 03:36:20 PDT
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 :-)
Comment 15 Landry Breuil (:gaston) 2012-05-24 08:48:23 PDT
(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 :)
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-05-24 15:53:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6747b994ba00
Comment 17 Ed Morley [:emorley] 2012-05-25 08:25:55 PDT
https://hg.mozilla.org/mozilla-central/rev/6747b994ba00
Comment 18 neil@parkwaycc.co.uk 2012-05-26 05:38:54 PDT
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.
Comment 19 neil@parkwaycc.co.uk 2012-05-26 06:00:33 PDT
(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.
Comment 20 Nick Cameron [:nrc] 2012-05-27 13:02:08 PDT
Created attachment 627587 [details] [diff] [review]
correct the scope of enums
Comment 21 Nick Cameron [:nrc] 2012-05-27 13:12:56 PDT
(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.
Comment 22 Nick Cameron [:nrc] 2012-05-27 17:39:23 PDT
Try push (build only): https://tbpl.mozilla.org/?tree=Try&rev=8bbf0303a1fe
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-29 20:06:07 PDT
https://hg.mozilla.org/mozilla-central/rev/4b57d0e84255

Note You need to log in before you can comment on or make changes to this bug.