Bug 75535 – [Qt] Implement HTTP authentication QML API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Alexander Færøy]

SSIA :-)

I will start hacking on this.

[Alexander Færøy]

Created attachment 122558 [details]
Patch

[Alexander Færøy]

Adding Simon and Kenneth for review.

[WebKit Review Bot]

Attachment 122558 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:72:  Failed to find complete declaration of class AuthenticationDialogContextObject  [build/class] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Færøy]

(In reply to comment #3)
> Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:72:  Failed to find complete declaration of class AuthenticationDialogContextObject  [build/class] [5]
> Total errors found: 1 in 14 files

I assumed this could safely be ignored, since we do the same for DialogContextObject in the surrounding code.

[Alexander Færøy]

Created attachment 122559 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 122559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122559&action=review

> Source/WebKit2/ChangeLog:13
> +        Based in part upon patch by Peter Hartmann.

a patch*

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:444
> +void QQuickWebViewPrivate::authenticationRequired(const QString& hostname, const QString& realm, const QString& oldUsername, QString& username, QString& password)

previousUsername? or is it UserName

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:451
> +    if (!dialogRunner.initForAuthentication(authenticationDialog, q, hostname, realm, oldUsername))

WebKit uses initialize

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:99
> +    void authenticationRequired(const QString& hostname, const QString& realm, const QString& oldUsername, QString& username, QString& password);

maybe s/oldUser.../prefilledUsername/ ? I think that is what it is

> Source/WebKit2/UIProcess/qt/QtPageClient.h:57
> +    virtual void authenticationRequired(const String& hostname, const String& realm, const String& oldUsername, String& username, String& password);

why not handleAuthenticationRequest? any reason?

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:86
> +void QtNetworkAccessManager::onAuthenticationRequired(QNetworkReply *reply, QAuthenticator *authenticator)

style

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:91
> +    // FIXME: This check can go away once our Qt version is up-to-date. See: QTBUG-23512.
> +    if (originatingObject) {

I would use if !originating...

[Kenneth Rohde Christiansen]

Is there any work on making this not sync?

[Alexander Færøy]

(In reply to comment #6)
> WebKit uses initialize

In that case, I think that should be fixed in a separate patch. We already have initForAlert(), initForConfirm(), initForPrompt() and now initForAuthentication() in QtDialogRunner.
 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:99
> > +    void authenticationRequired(const QString& hostname, const QString& realm, const QString& oldUsername, QString& username, QString& password);
> 
> maybe s/oldUser.../prefilledUsername/ ? I think that is what it is

Thinking about it, changing it to just username makes pretty good sense. This is also what the QAuthenticator API uses, which is where this comes from. I don't think prefilledUsername adds any additional information really.

> > Source/WebKit2/UIProcess/qt/QtPageClient.h:57
> > +    virtual void authenticationRequired(const String& hostname, const String& realm, const String& oldUsername, String& username, String& password);
> 
> why not handleAuthenticationRequest? any reason?

Good idea. Sounds better than "authenticationRequired" :-)
 
> > Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:91
> > +    // FIXME: This check can go away once our Qt version is up-to-date. See: QTBUG-23512.
> > +    if (originatingObject) {
> 
> I would use if !originating...

Yup, changing that locally. Waiting for the patch in bug #76342 to land firstly though.

[Alexander Færøy]

Created attachment 122639 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 122639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122639&action=review

> Source/WebKit2/ChangeLog:10
> +        and the UIProcess which is called when the authenticationRequired

need to update the changelog

> Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:97
> +signals:
> +    void accepted(const QString& username, const QString& password);
> +    void rejected();

why are these needed?

[Alexander Færøy]

(In reply to comment #10)
> > Source/WebKit2/ChangeLog:10
> > +        and the UIProcess which is called when the authenticationRequired
> 
> need to update the changelog

Not sure why? The QNAM signal is still called authenticationRequired.

> > Source/WebKit2/UIProcess/qt/QtDialogRunner.cpp:97
> > +signals:
> > +    void accepted(const QString& username, const QString& password);
> > +    void rejected();
> 
> why are these needed?

The other option is to pass a reference/pointer to the QtDialogRunner in the constructor of the AuthenticationDialogContextObject class and expose a setUsername() and setPassword() method in QtDialogRunner. What would you prefer?

[Kenneth Rohde Christiansen]

Comment on attachment 122639 [details]
Patch

Ok got the point. This seems good enough to go in. We can discuss the other issues f2f at some point.

[Alexander Færøy]

Comment on attachment 122639 [details]
Patch

CQ?

[WebKit Review Bot]

Comment on attachment 122639 [details]
Patch

Clearing flags on attachment: 122639

Committed r105146: <http://trac.webkit.org/changeset/105146>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.