[Webkit-unassigned] [Bug 50550] [Qt] QWebPage::createWindow does not recieve the proper window type...

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50550





--- Comment #9 from Dawit A. <adawit at kde.org>  2010-12-07 15:18:26 PST ---
(In reply to comment #7)
> (From update of attachment 75780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75780&action=review
> 
> >> WebCore/ChangeLog:8
> >> +        No new tests. (OOPS!)
> > 
> > You have an auto test. Either delete this line OR mention why it can not be tested.
> 
> I am in favor of extending the controller to have a layout test. It is too important to be limited to Qt autotests only.
> 
> > WebCore/page/WindowFeatures.cpp:159
> > +    else if (keyString == "dialog")
> > +        dialog = value;
> 
> In FrameLoader, there is an assert against this:
>    ASSERT(!features.dialog || request.frameName().isEmpty());
> This could never been hit before, could you check if it is still the case?

hmm... unfortunately that will be hit in debug mode. :( I am unsure why that check is there. Perhaps it assumes "dialog=yes" means a "modal dialog" ? Not sure. Anyhow, the qtwebkit implementation does not make sense either. Not only does the type property never get set to WebModalDialog, but also the suggestion in QWebPage::createWindow's documentation to set the WindowModality to ApplicationModal is wrong. Allowing any arbitrary untrusted script to set a modal dialog is a recipe for disaster. See the section on the "modal" property at https://developer.mozilla.org/en/DOM/window.open. That flag instead should have simply been labeled WebDialog instead...

> > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:359
> > +    virtual QWebPage* createWindow(WebWindowType type)
> > +    {
> >          QWebPage* page = new TestPage(this);
> >          createdWindows.append(page);
> > +        windowType = type;
> >          return page;
> 
> This is missleading in my opinion. You create a new page, but set the windowType of the current page, not the page returned by the call.
> 
> > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:@
> >  void tst_QWebPage::acceptNavigationRequestWithNewWindow()
> 
> I would prefer a new test.

I will do a new test once I finish implementing this correctly.

After much testing and playing around with other browsers, I have come to the conclusion that the patch I provided needs more work. Specifically, as already mentioned above the would be qtwebkit implementation for a dialog like window, though currently non-functional, has security risks associated with it. Further more, both of the other browsers I tested seem to create a new window if any property is present, even a non-valid one!! For example "window.open("about:blank", "", "foobar");" will result in a new window instead of a new tab. Only when no properties are specified do they resort to creating a tab or rather a browser window...

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.