Summary: | PopupMenu::show() assumes FrameView::hostWindow()->platformPageClient() is non-null | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Seigo <aseigo> | ||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED WORKSFORME | ||||||
Severity: | Normal | CC: | benjamin, hausmann, jesus, kenneth, kent.hansen, luiz | ||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | 37866 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Aaron Seigo
2010-02-17 12:58:33 PST
Could you provide a test case? I am not sure to understand why you don't use QGraphicsWebView in Plasma, but that is another story. Created attachment 50694 [details]
Testcase
(In reply to comment #1) > I am not sure to understand why you don't use QGraphicsWebView in Plasma, but > that is another story. Originally we didn't use QGraphicsWebView because QGraphicsWebView didn't exist. In mainline/trunk we do use QGraphicsWebView (well, KGraphicsWebView, which is QGraphicsWebview with all the KDE integration bits registered by default). It is still wrapped in libplasma to meet the libplasma scripting API requirements. Plasma moves very fast towards solutions, often faster than Qt can provide them. We also adapt quickly to solutions when they become available. :) More on topic, thanks, Kent for the test case. This looks related to bug #32670, btw. The patch attached to that report will likely fail for Plasma, however, as we often have multiple views on the canvas and the view may change (e.g. when moving an item from a desktop panel to the desktop layer itself, the item on the canvas, in this case a webview, will change where it is in the QGraphicsScene and the view will also change; ditto when adding it to a small panel and then later making the panel large enough to contain the webview in whole.) Seems to be the same bug as #36436, where I left a comment. The pageClient is indeed not supposed to be null in order for WebKit to work properly. (In reply to comment #4) > Seems to be the same bug as #36436, where I left a comment. > > The pageClient is indeed not supposed to be null in order for WebKit to work > properly. Yes, looks like a duplicate. Aaron, the patches being landed on https://bugs.webkit.org/show_bug.cgi?id=36436 solve this? I just want confirm so we can close both bugs when this is fixed. (In reply to comment #5) > (In reply to comment #4) > > Seems to be the same bug as #36436, where I left a comment. > > > > The pageClient is indeed not supposed to be null in order for WebKit to work > > properly. > > Yes, looks like a duplicate. they are related, yes. > Aaron, the patches being landed on > https://bugs.webkit.org/show_bug.cgi?id=36436 solve this? i haven't tested them, but from reading the patches i'm note sure they do. to get a pageClient(), it's still required to call setView with a QWidget. this will not work for apps with more than one view on the QGraphicsScene (either simultaneously or over time) for reasons already mentioned above. and now in QtFallbackWebPopup::show() it just returns quietly if there is no view set, which looks like (again, without testing, so mea culpa if i'm reading it wrong) we will get no popups without a view being set. imho, this really ought to be fetching the widget that the mouse event comes from; QGraphicsWidget knows this via QGraphicsSceneEvent::widget(), so the information does exist. and hopefully there are no other places in the code that assumes pageClient() is not-null (since we'll get the same kind of crash all over again there :) > i haven't tested them, but from reading the patches i'm note sure they do.
They won't entirely as that would need some more work. Aaron is not using the QGraphicsWebView but using the QWebPage together with a QGraphicsItem and not a QWidget.
(In reply to comment #7) > > i haven't tested them, but from reading the patches i'm note sure they do. > > They won't entirely as that would need some more work. Aaron is not using the > QGraphicsWebView but using the QWebPage together with a QGraphicsItem and not a > QWidget. That is a use-case we _could_ make work, just like it works with QWidget and QWebPage. (In reply to comment #8) > (In reply to comment #7) > > > i haven't tested them, but from reading the patches i'm note sure they do. > > > > They won't entirely as that would need some more work. Aaron is not using the > > QGraphicsWebView but using the QWebPage together with a QGraphicsItem and not a > > QWidget. > > That is a use-case we _could_ make work, just like it works with QWidget and > QWebPage. Actually, Jesus has some patches that _makes_ this work :-) basically name this QGraphicsWebView private not inherit the page client and make it similar to that of the QWebView, by installing the page client on demand. I looked at his patches today and gave some feedback, but I'm sure he will soon have patches up for review. (In reply to comment #9) > Actually, Jesus has some patches that _makes_ this work :-) basically name this > QGraphicsWebView private not inherit the page client and make it similar to > that of the QWebView, by installing the page client on demand. I looked at his > patches today and gave some feedback, but I'm sure he will soon have patches up > for review. And it is just waiting for review! https://bugs.webkit.org/show_bug.cgi?id=37858 was just a refactoring for the QWidget PageClient and it is landed. Now I'm just waiting for review on https://bugs.webkit.org/show_bug.cgi?id=37866, which might solve this. We have a patch for this use case but we are discussing if this should go into QtWebKit 2.0 or 2.1. Removing the blockers for now and adding the PageClient patch dependency. Can we close this bug? (In reply to comment #12) > Can we close this bug? Yep, looks like it. |