Bug 29678 - Conditionally guard cursor code with !QT_NO_CURSOR
Summary: Conditionally guard cursor code with !QT_NO_CURSOR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-09-23 05:37 PDT by Norbert Leser
Modified: 2009-10-02 08:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch for QT_NO_CURSOR guard (3.40 KB, patch)
2009-09-23 05:38 PDT, Norbert Leser
hausmann: review-
Details | Formatted Diff | Diff
updated patch for bug 29678 (3.77 KB, patch)
2009-10-02 06:46 PDT, Norbert Leser
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Norbert Leser 2009-09-23 05:37:45 PDT
Conditionally guard cursor code with !QT_NO_CURSOR. Otherwise, it is inconsistent with class declaration of QCursor.
Comment 1 Norbert Leser 2009-09-23 05:38:46 PDT
Created attachment 39991 [details]
Patch for QT_NO_CURSOR guard
Comment 2 Simon Hausmann 2009-09-23 06:15:31 PDT
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.
Comment 3 Norbert Leser 2009-09-23 06:48:45 PDT
(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.
Comment 4 Janne Koskinen 2009-09-24 01:15:06 PDT
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.
Comment 5 Norbert Leser 2009-10-02 06:46:12 PDT
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.
Comment 6 Janne Koskinen 2009-10-02 07:46:40 PDT
(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.
Comment 7 Simon Hausmann 2009-10-02 07:54:40 PDT
Comment on attachment 40513 [details]
updated patch for bug 29678

r=me. Thanks!
Comment 8 Laszlo Gombos 2009-10-02 08:50:40 PDT
Comment on attachment 40513 [details]
updated patch for bug 29678

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