Bug 29678 – Conditionally guard cursor code with !QT_NO_CURSOR

• Component: WebKit API
• Status: RESOLVED FIXED

[Norbert Leser]

Conditionally guard cursor code with !QT_NO_CURSOR. Otherwise, it is inconsistent with class declaration of QCursor.

[Norbert Leser]

Created attachment 39991 [details]
Patch for QT_NO_CURSOR guard

[Simon Hausmann]

Comment on attachment 39991 [details]
Patch for QT_NO_CURSOR guard


> Index: WebKit/qt/Api/qwebgraphicsitem.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebgraphicsitem.cpp	(revision 48670)
> +++ WebKit/qt/Api/qwebgraphicsitem.cpp	(working copy)
> @@ -95,12 +95,16 @@ void QWebGraphicsItemPrivate::update(con
>  
>  QCursor QWebGraphicsItemPrivate::cursor() const
>  {
> +#ifndef QT_NO_CURSOR
>      return q->cursor();
> +#endif
>  }
>  
>  void QWebGraphicsItemPrivate::updateCursor(const QCursor& cursor)
>  {
> +#ifndef QT_NO_CURSOR
>      q->setCursor(cursor);
> +#endif
>  }

The patch looks good to me, expect for these two hunks. Instead of placing the #ifdef into the function it should be placed
around the definition/declaration. In other words: If QT_NO_CURSOR is defined, then those functions not not be declared
in the first place, similar to the guards around QWidget::setCursor()/cursor() in qwidget.h.

[Norbert Leser]

(In reply to comment #2)
> (From update of attachment 39991 [details])
> 
> > Index: WebKit/qt/Api/qwebgraphicsitem.cpp
> > ===================================================================
> > --- WebKit/qt/Api/qwebgraphicsitem.cpp	(revision 48670)
> > +++ WebKit/qt/Api/qwebgraphicsitem.cpp	(working copy)
> > @@ -95,12 +95,16 @@ void QWebGraphicsItemPrivate::update(con
> >  
> >  QCursor QWebGraphicsItemPrivate::cursor() const
> >  {
> > +#ifndef QT_NO_CURSOR
> >      return q->cursor();
> > +#endif
> >  }
> >  
> >  void QWebGraphicsItemPrivate::updateCursor(const QCursor& cursor)
> >  {
> > +#ifndef QT_NO_CURSOR
> >      q->setCursor(cursor);
> > +#endif
> >  }
> 
> The patch looks good to me, expect for these two hunks. Instead of placing the
> #ifdef into the function it should be placed
> around the definition/declaration. In other words: If QT_NO_CURSOR is defined,
> then those functions not not be declared
> in the first place, similar to the guards around QWidget::setCursor()/cursor()
> in qwidget.h.

That is your call, or whoever implemented the classes/functions. In the case of conditionally guarding the functions itself, you may end up with a multitude of conditional statements in the client code. Otherwise, it would be transparent (though, perhaps unexpected behavior - i.e., updateCursor etc would do nothing, unless there is another default implementation).

Anyway, could you or the implementer of QWebGraphicsItemPrivate, QWebViewPrivate, and QWebPageClient take it from here? - Thanks.

[Janne Koskinen]

I have to agree with Simon here.
Even if you would return empty QCursor (which you don't), client still needs to be aware of that cursor is not enabled.

I'm saying this even if we are in huge trouble with ordinal exports and conditional exporting in Symbian.

[Norbert Leser]

Created attachment 40513 [details]
updated patch for bug 29678

Patch updated according to Simon's comments (enclosed whole functions within conditional clause.
This should not affect the API since these are functions of a private class.

[Janne Koskinen]

(In reply to comment #5)
> This should not affect the API since these are functions of a private class.

Right, I've been way too much in QtCore and QtGui where private classes are exported.

[Simon Hausmann]

Comment on attachment 40513 [details]
updated patch for bug 29678

r=me. Thanks!

[Laszlo Gombos]

Comment on attachment 40513 [details]
updated patch for bug 29678

Landed as http://trac.webkit.org/changeset/49023