Bug 30771

Created attachment 41862 [details] patch 0.1 Patch does: 1) makes qwebpage::createWindow widget independent (pageClient used now). 2) adds a pure virtual createWindow method to qwebpageclient. 3) fixes some minor nits ("*" position and such)

Generally, I dont like much the original method. First of all, the method is called by webkit, but the API user can call it as well (not very well explained in the documentation). It returns a reference to QWebView and now you introduced a verson returning QGraphicsWebView. That seems very limiting to me. Maybe I want to return a more real widget (like a BrowerWindow) or even a QWebView from a QGraphicsWebView implementation. Thus, shouldn't we make the new QGraphicsWebView version return a QObject instead? +QGraphicsWebView *QGraphicsWebView::createWindow(QWebPage::WebWindowType type) +{ + Q_UNUSED(type) + return 0; the Q_UNUSED is not needed, just leave out "type". Is it possible to make this work without adding these specialized methods to QWebPageClient. We need to consider very much what we add there, and whether it really makes sense.

thx for commenting. (In reply to comment #2) > Generally, I dont like much the original method. First of all, the method is > called by webkit, but the API user can call it as well (not very well explained in the documentation). hum, it is protected, so it should not be totally callable, unless you are re-implementing the class (and then knows what you are doing). > It returns a reference to QWebView and now you > introduced a verson returning QGraphicsWebView. That seems very limiting to me. > Maybe I want to return a more real widget (like a BrowerWindow) or even a > QWebView from a QGraphicsWebView implementation. Thus, shouldn't we make the > new QGraphicsWebView version return a QObject instead? i am particularly ok w/ that. > +QGraphicsWebView *QGraphicsWebView::createWindow(QWebPage::WebWindowType type) > +{ > + Q_UNUSED(type) > + return 0; > > the Q_UNUSED is not needed, just leave out "type". will do. as per ariya's request, i will check some autotests for it. > Is it possible to make this work without adding these specialized methods to > QWebPageClient. We need to consider very much what we add there, and whether it > really makes sense. pageClient is meant to be an interface to add widget specific methods, or to abstract the widget (either qwebview or qgraphicswebview for now) from the internal classes. that is why i extended it.

> hum, it is protected, so it should not be totally callable, unless you are > re-implementing the class (and then knows what you are doing). Then there is not much sense is having it return something, is there? What is the use-case? > pageClient is meant to be an interface to add widget specific methods, or to > abstract the widget (either qwebview or qgraphicswebview for now) from the > internal classes. that is why i extended it. I know what it is for, as I actually implemented it. What I'm trying to say here is that we should only add very generic and needed things, as I don't want this to end up as a dumping ground.

thx for commenting again. > > hum, it is protected, so it should not be totally callable, unless you are > > re-implementing the class (and then knows what you are doing). > > Then there is not much sense is having it return something, is there? What is > the use-case? not sure if i get you correct here. For new window we do not need a new widget (e.g. qwebview or qgraphicswebview) ? I think we do and usecases are simple: popups, JS open-in-another-window, etc. again, method createWindow is already there for qwebview. logic backtrace: 1) ChromeClientQt::createWindow (WebCore needs a Page object) - calls 2) QWebPage::createWindow - calls pageClient's createWindow, abstracting the widget. 3) QXXXWebView::createWindow - creates a new widget, and retuns *internally* to the application whom maybe want to attach the widget to TAB or what ever ( again it is protected +- internal) 4) qXXXwebview has a qwebpage that has a Page , which is needed to WebCore for a new window. > What I'm trying to say > here is that we should only add very generic and needed things, as I don't want > this to end up as a dumping ground. do you think is it not generic enough to be there? i am open to suggestions. > I know what it is for, as I actually implemented it. i know, i know ... great contrib btw :)

> Then there is not much sense is having it return something, is there? What is > the use-case? maybe i am seeing a problem: when user implements qwebpage::createWindow but not qXXXwebview::createWindow ... checking

(In reply to comment #5) > thx for commenting again. > > > > hum, it is protected, so it should not be totally callable, unless you are > > > re-implementing the class (and then knows what you are doing). > > > > Then there is not much sense is having it return something, is there? What > 3) QXXXWebView::createWindow - creates a new widget, and retuns *internally* to > the application whom maybe want to attach the widget to TAB or what ever ( > again it is protected +- internal) The reimplemented can easily do this internally without needing it to return anything. I guess this was only done for being able to return the page (QWebPage) which is needed by WebCore. I guess QWebPage* createWindow() might make more sense.

(In reply to comment #5) > thx for commenting again. > > > > hum, it is protected, so it should not be totally callable, unless you are > > > re-implementing the class (and then knows what you are doing). > > > > Then there is not much sense is having it return something, is there? What > 3) QXXXWebView::createWindow - creates a new widget, and retuns *internally* to > the application whom maybe want to attach the widget to TAB or what ever ( > again it is protected +- internal) The reimplemented can easily do this internally without needing it to return anything. I guess this was only done for being able to return the page (QWebPage) which is needed by WebCore. I guess QWebPage* createWindow() might make more sense for QGraphicsWebView.

Btw, with your change the documentation for QWebPage::createWindow is wrong. The patch needs to update that as well :-)

ok, all confuse bits come from the fact that both qwebview and qwebpage have this virtual createWindow method to be re-implemented. kenneth and I agree on IRC that if there is not a good reason for also duplicate into QGraphicsWebView then it should be avoided. thoughts?

The problem is that I can reimplement the createWindow in QWebView and in the associated QWebPage. If it is reimplemented in the latter, the version reimplemented in QWebView derived class, will never be used. This is not detailed in the documentation and is outright confusing :-) Also, a worse problem: If I use a special page class (derived from QWebPage) together with QWebView and do not reimplement QWebPage::createWindow, then all new windows will be created by QWebView::createWindow and use the normal QWebPage class and not my derived one! Ups!

> If I use a special page class (derived from QWebPage) together with QWebView > and do not reimplement QWebPage::createWindow, then all new windows will be > created by QWebView::createWindow and use the normal QWebPage class and not my > derived one! Ups! or put differently you need to make sure that your QWebView::createWindow creates pages of the right kind. If doing anything like a web browser you need to reimplement the QWebPage anyway, so I'm not sure that the Q(GraphicsView)WebView::createWindow helps much, except creating all these confusing situations. So, I would say, let's fix the docs to explain the current behaviour with QWebView::createWindow and let's think a bit more before adding this method to QGraphicsWebView.

(In reply to comment #12) > > If I use a special page class (derived from QWebPage) together with QWebView > > and do not reimplement QWebPage::createWindow, then all new windows will be > > created by QWebView::createWindow and use the normal QWebPage class and not my > > derived one! Ups! > > or put differently you need to make sure that your QWebView::createWindow > creates pages of the right kind. > > If doing anything like a web browser you need to reimplement the QWebPage > anyway, so I'm not sure that the Q(GraphicsView)WebView::createWindow helps > much, except creating all these confusing situations. > > So, I would say, let's fix the docs to explain the current behaviour with > QWebView::createWindow and let's think a bit more before adding this method to > QGraphicsWebView. hum, another problem w/ current patch: suppose we have one qgraphicswebview and two qwebpages swapped as apropriated. if i swap page_1 to page_2, page_1's client will '0' then in the default implementation a page wont be able to create a window if it is not the one attached to the current qgraphicswebview. QWebPage *QWebPage::createWindow(WebWindowType type) { if (d->client) //<------- this will only work for current viewed pages. return d->client->createWindow(type); return 0; }

Comment on attachment 41862 [details] patch 0.1 i am clearing r? flag for now, as the wanted behavior is yet to be defined here.

Are you planning to make the patch to update the documentation? This task is still blocking #29799, it would be nice to close it.

(In reply to comment #15) > Are you planning to make the patch to update the documentation? > This task is still blocking #29799, it would be nice to close it. I can change the documentation tomorrow. I will add it to my TODO

I agree with the earlier comments not _not_ have a createWindow implementation QGWV. Initially I liked the idea, but what convinced me was the point that adding it is only going to _create_ confusion, apart from the convenience it adds. So I'm all for not adding it.

Removed from the API block bug, as it's not an API issue anymore but a documentation issue.

(In reply to comment #18) > Removed from the API block bug, as it's not an API issue anymore but a > documentation issue. OK, I have updated the documentation of QWebView::createWindow.

(In reply to comment #19) > (In reply to comment #18) > > Removed from the API block bug, as it's not an API issue anymore but a > > documentation issue. > > OK, I have updated the documentation of QWebView::createWindow. Thanks! Now we just need to cherry-pick the fixes into the qtwebkit-4.6 branch :)

Landed in svn and cherry-picked into qtwebkit-4.6. Closing this bug

simon, kenneth, although I agree that documentation is improved for createWindow, the original problem is not fixed. see comment #1. it is just not possible for browsers using qgraphicswebview to open new window. I will reopen and make it a not 4.6 blocker.

(In reply to comment #22) > simon, kenneth, although I agree that documentation is improved for > createWindow, the original problem is not fixed. see comment #1. > > it is just not possible for browsers using qgraphicswebview to open new window. > > I will reopen and make it a not 4.6 blocker. I don't understand why it's not possible for browsers using QGraphicsWebView to handle the creation of new windows. Comment #1 doesn't explain that either and the bug description merely says that the current default implementation in QWebPage is tailored towards QWebView. That is correct. However given that QWebPage::createWindow is virtual shouldn't browsers be able to use their QWebPage subclass together with QGraphicsWebView and intercept the window creation? The main argument I see in favour of _not_ adding it to QGraphicsWebView is that adding it adds to the API confusion. Two virtual functions in related classes with the same name make it easy for developers to shake their head and wonder: "Which one do I have to deal with now?"

I fully agree and think we should close this bug (In reply to comment #23) > (In reply to comment #22) > > simon, kenneth, although I agree that documentation is improved for > > createWindow, the original problem is not fixed. see comment #1. > > > > it is just not possible for browsers using qgraphicswebview to open new window. > > > > I will reopen and make it a not 4.6 blocker. > > I don't understand why it's not possible for browsers using QGraphicsWebView to > handle the creation of new windows. Comment #1 doesn't explain that either and > the bug description merely says that the current default implementation in > QWebPage is tailored towards QWebView. That is correct. > > However given that QWebPage::createWindow is virtual shouldn't browsers be able > to use their QWebPage subclass together with QGraphicsWebView and intercept the > window creation? > > > The main argument I see in favour of _not_ adding it to QGraphicsWebView is > that adding it adds to the API confusion. Two virtual functions in related > classes with the same name make it easy for developers to shake their head and > wonder: "Which one do I have to deal with now?"

> However given that QWebPage::createWindow is virtual shouldn't browsers be able to use their QWebPage subclass together with QGraphicsWebView and intercept the window creation? Yes, re-implementing it makes it possible to create windows. However the initial goal of this bug was to make the default implementation of qwebpage::createWindow to not depend on qwebview at all, but instead qwebpageclient, which i still think is a good think to do. > The main argument I see in favour of _not_ adding it to QGraphicsWebView is > that adding it adds to the API confusion. Two virtual functions in related > classes with the same name make it easy for developers to shake their head and > wonder: "Which one do I have to deal with now?" right, the original API is not good imho, and that is the problem. But i agree we have to keep ABI compatible. ok, lets close it then.