596200 - gfxQPainterSurface.h should only be included if we are enabling qt builds

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jory A. Pratt [:anarchy]]

Attachment: check that moz_widget_qt is defined before including gfxQPainterSurface.h

../../staticlib/libthebes.a(gfxASurface.o): In function
`gfxASurface::Wrap(_cairo_surface*)':
gfxASurface.cpp:(.text+0x1d1): undefined reference to
`gfxQPainterSurface::gfxQPainterSurface(_cairo_surface*)'
collect2: ld returned 1 exit status

Comment 1

[Jory A. Pratt [:anarchy]]

Attachment: check_for_moz_widget_qt_before_including_gfxQPainterSurface-v0.1

Sorry for the double work here roc, but I screwed up the patch. It needed to be #ifdef defined(CAIRO_HAS_QT_SURFACE) && defined(MOZ_WIDGET_QT) 

#ifdef doesn't support complex expressions and doesn't evaluate the defined(MOZ_WIDGET_QT) check.

Comment 2

[Jory A. Pratt [:anarchy]]

Attachment: check_for_moz_widget_qt_before_including_gfxQPainterSurface-v0.2

review carried over small change from ifdef to if as we are checking two defines.

Comment 3

[Jory A. Pratt [:anarchy]]

Attachment: check_for_moz_widget_qt_before_including_gfxQPainterSurface-v0.3

Once again I missed a single change of ifdef to if for the defines to be used correctly.

Comment 4

[Jory A. Pratt [:anarchy]]

(In reply to comment #3)
> Created attachment 476444 [details] [diff] [review]
> check_for_moz_widget_qt_before_including_gfxQPainterSurface-v0.3
> 
> Once again I missed a single change of ifdef to if for the defines to be used
> correctly.

I should have noted without this change we would have broken qt builds.

Comment 5

[Oleg Romashin (:romaxa)]

I guess you should ask approval before checkin-needed

Comment 6

[Oleg Romashin (:romaxa)]

I think this is safe to push

Comment 7

[Mounir Lamouri (:mounir)]

Roc, can you approve this patch? (considering you reviewed it)

Comment 8

[Jory A. Pratt [:anarchy]]

The more I look at this, the more I do not believe this is best fix, it seems we should fix configure.in, so it respects the default-toolkit which it obviously is not doing. Based of what I see in configure.in --default-toolkit=cairo-qt needs to be passed in order to enable MOZ_WIDGET_TOOLKIT=qt which then runs 

     if test "$MOZ_WIDGET_TOOLKIT" = "qt"; then
         QT_SURFACE_FEATURE="#define CAIRO_HAS_QT_SURFACE 1"
     fi

which should only be possible like I said before if default-toolkit=qt was passed to configure.

Comment 9

[Oleg Romashin (:romaxa)]

Can you attach new  patch?

Comment 10

[Mounir Lamouri (:mounir)]

Pushed as requested by Jory:
http://hg.mozilla.org/mozilla-central/rev/f8854fb6b63f

Open another bug if there is a better way to fix it.

Comment 11

[Mounir Lamouri (:mounir)]

For some reasons the attached patch didn't had the fixes from comment 2 and comment 3.

Re-pushed:
http://hg.mozilla.org/mozilla-central/rev/dd06023bddcd