Bug 30336 - [Qt] Make context menu to work in QGraphicsWebView
Summary: [Qt] Make context menu to work in QGraphicsWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt
: 29180 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-10-13 09:24 PDT by Antonio Gomes
Modified: 2009-10-21 20:10 PDT (History)
1 user (show)

See Also:


Attachments
patch 0.1 (2.27 KB, patch)
2009-10-13 09:27 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 0.2 - same as 0.1, but not leaky (2.26 KB, patch)
2009-10-13 09:44 PDT, Antonio Gomes
hausmann: review-
Details | Formatted Diff | Diff
landed in r49618 - patch 0.3 (2.18 KB, patch)
2009-10-13 18:30 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2009-10-13 09:24:43 PDT
currently csm are not working in qgwv ...
Comment 1 Antonio Gomes 2009-10-13 09:27:59 PDT
Created attachment 41106 [details]
patch 0.1
Comment 2 Antonio Gomes 2009-10-13 09:44:59 PDT
Created attachment 41110 [details]
patch 0.2 - same as 0.1, but not leaky
Comment 3 Simon Hausmann 2009-10-13 15:19:38 PDT
Comment on attachment 41110 [details]
patch 0.2 - same as 0.1, but not leaky


Argh, I'm sorry Antonio, I overlooked two details about the patch :-/

> +            if (QGraphicsSceneContextMenuEvent* ev = static_cast<QGraphicsSceneContextMenuEvent*>(event)) {

This if (and scope) isn't necessary, as the cast can't fail and as event is guaranteed to be non-zero.

> +                QContextMenuEvent fakeEvent(QContextMenuEvent(QContextMenuEvent::Mouse, ev->pos().toPoint()));

It looks like there's a redundant nesting :), i.e. the could should probably read

QContextMenuEvent fakeEvent(reason, ev->pos().toPoint());

instead of QContextMenuEvent fakeEvent(QContextMenuEvent(...));

I also think we should map ev->reason() to the QContextMenuEvent::Reason.

Sorry :-/, I should've spotted this on the first review.
Comment 4 Antonio Gomes 2009-10-13 18:30:59 PDT
Created attachment 41144 [details]
landed in r49618 - patch 0.3

> Argh, I'm sorry Antonio, I overlooked two details about the patch :-/
> I should've spotted this on the first review.

thx again for re-reviewing, simon.

> > +            if (QGraphicsSceneContextMenuEvent* ev = static_cast<QGraphicsSceneContextMenuEvent*>(event)) {
> This if (and scope) isn't necessary, as the cast can't fail and as event is
> guaranteed to be non-zero.

totally agree. done ...

> > +                QContextMenuEvent fakeEvent(QContextMenuEvent(QContextMenuEvent::Mouse, ev->pos().toPoint()));
> It looks like there's a redundant nesting :), i.e. the could should probably
> read

errr ... likely copy and paste error. fixed.

> I also think we should map ev->reason() to the QContextMenuEvent::Reason.

true. also done.
Comment 5 Simon Hausmann 2009-10-13 22:05:13 PDT
Comment on attachment 41144 [details]
landed in r49618 - patch 0.3

Thanks!
Comment 6 Antonio Gomes 2009-10-15 05:43:00 PDT
landed in r49618
Comment 7 Antonio Gomes 2009-10-15 10:17:59 PDT
Comment on attachment 41144 [details]
landed in r49618 - patch 0.3

clearing r+ flag since patch has landed.
Comment 8 Antonio Gomes 2009-10-21 20:10:32 PDT
*** Bug 29180 has been marked as a duplicate of this bug. ***