Closed Bug 1390317 Opened 2 years ago Closed 2 years ago

gfx/thebes/gfxFont.cpp:846:5: error: 'cairo_scaled_font_get_hint_metrics' was not declared in this scope

Categories

(Core :: Graphics: Text, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: jbeich, Assigned: jfkthame)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

$ pkg info -x cairo
cairo-1.14.10,2
py27-cairo-1.10.0_2

$ echo "ac_add_options --enable-system-cairo" >>.mozconfig
$ ./mach build
[...]
In file included from objdir/gfx/thebes/Unified_cpp_gfx_thebes0.cpp:65:
gfx/thebes/gfxFont.cpp:846:5: error: use of undeclared identifier
      'cairo_scaled_font_get_hint_metrics'; did you mean 'cairo_scaled_font_get_font_face'?
    cairo_scaled_font_get_hint_metrics(scaled_font);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    cairo_scaled_font_get_font_face
/usr/local/include/cairo/cairo.h:1628:1: note: 'cairo_scaled_font_get_font_face' declared here
cairo_scaled_font_get_font_face (cairo_scaled_font_t *scaled_font);
^
In file included from objdir/gfx/thebes/Unified_cpp_gfx_thebes0.cpp:65:
gfx/thebes/gfxFont.cpp:845:24: error: cannot initialize a variable of type
      'cairo_hint_metrics_t' (aka '_cairo_hint_metrics') with an rvalue of type 'cairo_font_face_t *'
      (aka '_cairo_font_face *')
  cairo_hint_metrics_t hint_metrics =
                       ^
2 errors generated.
Hmm, I didn't know we still supported building with --enable-system-cairo. OK, for that situation we can't use the new mozilla-specific cairo api that was added in bug 1377257, so I guess we can just fall back on the old code here.
Attachment #8897179 - Flags: review?(jmuizelaar)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8897179 [details] [diff] [review]
Don't try to use the mozilla-specific cairo extension from bug 1377257 if we're building with system cairo

Thanks. With the patch applied --enable-system-cairo builds fine and runs as before.
Attachment #8897179 - Flags: feedback+
Building with system cairo isn't really supported anymore. Without a really compelling reason I think it would probably be better to just remove the --enable-system-cairo flag.
Jan, do you have "a really compelling reason" (see comment 3) to use --enable-system-cairo? Or can we just remove it?
Flags: needinfo?(jbeich)
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Jan, do you have "a really compelling reason" (see comment 3) to use
> --enable-system-cairo? Or can we just remove it?

No. What about Gentoo?
Flags: needinfo?(jbeich) → needinfo?(anarchy)
We do not support building with system-cairo.
Flags: needinfo?(anarchy)
Priority: -- → P3
Whiteboard: [gfx-noted]
Can you either land the fix or remove the option? Leaving --enable-system-cairo half-broken on FF57 is going to confuse users. Maybe add AC_MSG_ERROR to old-configure.in if you're not ready to remove the ifdefs e.g., need to compare old vs. new Cairo sometimes.
Flags: needinfo?(jfkthame)
FWIW, I've used --enable-system-cairo for years without issues (except circa FF40 due to OMTC) and didn't notice it was broken. e10s was disabled by extensions.
Same here. Apart from the very rare breakage, I've been compiling with --enable-system-cairo since firefox 2.0 days.
I use cairo 1.15.8 at the moment.
Jeff: while I have no particular attachment to --enable-system-cairo, some people are accustomed to using it, and removing it altogether looks like a lot more upheaval than the patch here. Given that this is such a simple fix (and doesn't affect functionality at all, it's just disabling a minor optimization), I'd be inclined to take this patch for now.

Removing build support for --enable-system-cairo (and the associated MOZ_TREE_CAIRO #ifdefs, etc) should be a separate followup, IMO, that we can consider at leisure.
Flags: needinfo?(jfkthame) → needinfo?(jmuizelaar)
We've also been using --with-system-cairo since more or less forever (1.14.10 right now), when it was possible. It wasnt possible for a while in the 40-something days, but since the switch to Gtk3 i reenabled it (my notes point to bug 983843 for the actual reason; memory is fuzzy).

If it's officially unsupported, then someone has to take the responsability to remove it.
Attachment #8897179 - Flags: review?(jmuizelaar) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5a050f7eb7
Don't try to use the mozilla-specific cairo extension from bug 1377257 if we're building with system cairo. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/ff5a050f7eb7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Can you request merge to Firefox 57 to avoid regression there as well?
Flags: needinfo?(jfkthame)
Comment on attachment 8897179 [details] [diff] [review]
Don't try to use the mozilla-specific cairo extension from bug 1377257 if we're building with system cairo

We can request merging to 57, but I don't know if it will be accepted; the bar for patches to land there is pretty strict, I believe. But as this has no effect on mozilla builds, maybe it'll be OK.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1377257
[User impact if declined]: build failure for downstream consumers using --enable-system-cairo
[Is this code covered by automated tests?]: n/a, #ifdef'd code that is relevant only to non-mozilla --enable-system-cairo builds
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: no impact on mozilla builds, #ifdef'd alternative just reverts to previous code (dropping a micro-optimization) for downstream builds
[String changes made/needed]: none
Flags: needinfo?(jfkthame)
Attachment #8897179 - Flags: approval-mozilla-beta?
Flags: needinfo?(jmuizelaar)
Comment on attachment 8897179 [details] [diff] [review]
Don't try to use the mozilla-specific cairo extension from bug 1377257 if we're building with system cairo

NPOTB, Beta57+
Attachment #8897179 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.