The default bug view has changed. See this FAQ.

[Azure][Skia] Refactor gfxFont out of Azure

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

unspecified
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 751431
(Assignee)

Comment 1

5 years ago
Created attachment 621482 [details] [diff] [review]
patch
Attachment #621482 - Flags: review?(bas.schouten)
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.
(Assignee)

Comment 3

5 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.
(Assignee)

Comment 4

5 years ago
Try run: https://tbpl.mozilla.org/?tree=Try&rev=b2b3dbd3193d
(Assignee)

Comment 5

5 years ago
Created attachment 623030 [details] [diff] [review]
updated patch

Made requested changes
Attachment #621482 - Attachment is obsolete: true
Attachment #621482 - Flags: review?(bas.schouten)
Attachment #623030 - Flags: review?(bas.schouten)
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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #623030 - Attachment is obsolete: true
Attachment #623030 - Flags: review?(bas.schouten)
Attachment #623813 - Flags: review?(bas.schouten)
(Assignee)

Comment 8

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

Comment 9

5 years ago
Created attachment 624681 [details] [diff] [review]
updated patch
Attachment #623813 - Attachment is obsolete: true
Attachment #623813 - Flags: review?(bas.schouten)
Attachment #624681 - Flags: review?(bas.schouten)
Attachment #624681 - Flags: review?(bas.schouten) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/303684497400
https://hg.mozilla.org/mozilla-central/rev/303684497400
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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 ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #626718 - Flags: review?(ncameron)
(Assignee)

Comment 14

5 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 :-)
Attachment #626718 - Flags: review?(ncameron)
Attachment #626718 - Flags: review?(bas.schouten)
Attachment #626718 - Flags: feedback+
Attachment #626718 - Flags: review?(bas.schouten) → review+
(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 :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6747b994ba00
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6747b994ba00
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
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.
(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.
(Assignee)

Comment 20

5 years ago
Created attachment 627587 [details] [diff] [review]
correct the scope of enums
Attachment #627587 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #627587 - Flags: review? → review?(bas.schouten)
(Assignee)

Comment 21

5 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.
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

5 years ago
Try push (build only): https://tbpl.mozilla.org/?tree=Try&rev=8bbf0303a1fe
Status: REOPENED → NEW
Target Milestone: mozilla15 → ---
Attachment #627587 - Flags: review?(bas.schouten) → review+
https://hg.mozilla.org/mozilla-central/rev/4b57d0e84255
Status: NEW → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 895431
You need to log in before you can comment on or make changes to this bug.