Bug 75535 - [Qt] Implement HTTP authentication QML API
Summary: [Qt] Implement HTTP authentication QML API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexander Færøy
URL:
Keywords:
Depends on:
Blocks: 76347
  Show dependency treegraph
 
Reported: 2012-01-04 03:36 PST by Alexander Færøy
Modified: 2012-01-17 05:18 PST (History)
4 users (show)

See Also:


Attachments
Patch (20.13 KB, patch)
2012-01-14 17:09 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff
Patch (20.14 KB, patch)
2012-01-14 17:36 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff
Patch (20.28 KB, patch)
2012-01-16 08:10 PST, Alexander Færøy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Færøy 2012-01-04 03:36:02 PST
SSIA :-)

I will start hacking on this.
Comment 1 Alexander Færøy 2012-01-14 17:09:57 PST
Created attachment 122558 [details]
Patch
Comment 2 Alexander Færøy 2012-01-14 17:10:29 PST
Adding Simon and Kenneth for review.
Comment 3 WebKit Review Bot 2012-01-14 17:10:52 PST
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.
Comment 4 Alexander Færøy 2012-01-14 17:13:16 PST
(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.
Comment 5 Alexander Færøy 2012-01-14 17:36:24 PST
Created attachment 122559 [details]
Patch
Comment 6 Kenneth Rohde Christiansen 2012-01-16 03:03:54 PST
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...
Comment 7 Kenneth Rohde Christiansen 2012-01-16 03:04:28 PST
Is there any work on making this not sync?
Comment 8 Alexander Færøy 2012-01-16 05:34:20 PST
(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.
Comment 9 Alexander Færøy 2012-01-16 08:10:34 PST
Created attachment 122639 [details]
Patch
Comment 10 Kenneth Rohde Christiansen 2012-01-16 08:43:11 PST
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?
Comment 11 Alexander Færøy 2012-01-16 10:10:51 PST
(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?
Comment 12 Kenneth Rohde Christiansen 2012-01-16 13:39:40 PST
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.
Comment 13 Alexander Færøy 2012-01-17 03:54:05 PST
Comment on attachment 122639 [details]
Patch

CQ?
Comment 14 WebKit Review Bot 2012-01-17 05:18:07 PST
Comment on attachment 122639 [details]
Patch

Clearing flags on attachment: 122639

Committed r105146: <http://trac.webkit.org/changeset/105146>
Comment 15 WebKit Review Bot 2012-01-17 05:18:12 PST
All reviewed patches have been landed.  Closing bug.