Bug 30771 - [Qt] Make qwebpage's createWindow not qwebview dependent
Summary: [Qt] Make qwebpage's createWindow not qwebview dependent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-10-26 06:13 PDT by Antonio Gomes
Modified: 2009-11-11 13:52 PST (History)
6 users (show)

See Also:


Attachments
patch 0.1 (9.57 KB, patch)
2009-10-26 06:18 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-26 06:13:12 PDT
Currently, qwebpage's createWindow virtual is totatly dependent on qwebview, which is wrong by design, now that we have PageClient.

QWebPage *QWebPage::createWindow(WebWindowType type)
{
  QWebView *webView = qobject_cast<QWebView *>(view());
  if (webView) {
    QWebView *newView = webView->createWindow(type);
    if (newView)
      return newView->page();
  }
  return 0;
}

Attached patch (coming) changes that for qwebpage+qwebview and implements it for qwebpage+qgraphicswebview createWindow.
Comment 1 Antonio Gomes 2009-10-26 06:18:37 PDT
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)
Comment 2 Kenneth Rohde Christiansen 2009-10-26 10:55:01 PDT
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.
Comment 3 Antonio Gomes 2009-10-26 11:05:33 PDT
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.
Comment 4 Kenneth Rohde Christiansen 2009-10-26 11:13:22 PDT
> 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.
Comment 5 Antonio Gomes 2009-10-26 11:44:24 PDT
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 :)
Comment 6 Antonio Gomes 2009-10-26 11:52:10 PDT
> 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
Comment 7 Kenneth Rohde Christiansen 2009-10-26 11:58:46 PDT
(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.
Comment 8 Kenneth Rohde Christiansen 2009-10-26 11:59:24 PDT
(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.
Comment 9 Kenneth Rohde Christiansen 2009-10-26 12:43:21 PDT
Btw, with your change the documentation for QWebPage::createWindow is wrong. The patch needs to update that as well :-)
Comment 10 Antonio Gomes 2009-10-26 12:54:32 PDT
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?
Comment 11 Kenneth Rohde Christiansen 2009-10-26 12:59:32 PDT
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!
Comment 12 Kenneth Rohde Christiansen 2009-10-26 13:04:12 PDT
> 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.
Comment 13 Antonio Gomes 2009-10-26 13:45:40 PDT
(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 14 Antonio Gomes 2009-10-26 21:46:05 PDT
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.
Comment 15 Benjamin Poulain 2009-11-02 16:20:19 PST
Are you planning to make the patch to update the documentation?
This task is still blocking #29799, it would be nice to close it.
Comment 16 Kenneth Rohde Christiansen 2009-11-09 11:08:06 PST
(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
Comment 17 Simon Hausmann 2009-11-10 06:50:02 PST
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.
Comment 18 Simon Hausmann 2009-11-10 06:54:21 PST
Removed from the API block bug, as it's not an API issue anymore but a documentation issue.
Comment 19 Kenneth Rohde Christiansen 2009-11-10 11:01:08 PST
(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.
Comment 20 Simon Hausmann 2009-11-10 13:00:31 PST
(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 :)
Comment 21 Simon Hausmann 2009-11-11 10:44:13 PST
Landed in svn and cherry-picked into qtwebkit-4.6. Closing this bug
Comment 22 Antonio Gomes 2009-11-11 11:16:11 PST
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.
Comment 23 Simon Hausmann 2009-11-11 13:30:27 PST
(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?"
Comment 24 Kenneth Rohde Christiansen 2009-11-11 13:36:41 PST
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?"
Comment 25 Antonio Gomes 2009-11-11 13:52:19 PST
> 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.