Bug 103644 – [GTK] Split GtkAuthenticationDialog in two widgets

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

One for the common auth logic and another one with the dialog specific things.

[Carlos Garcia Campos]

Created attachment 176741 [details]
Patch

[Carlos Garcia Campos]

Created attachment 177218 [details]
New patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h"
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:165:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:169:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:170:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:175:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:178:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:179:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 19 files


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

[Carlos Garcia Campos]

Created attachment 189975 [details]
Updated patch

Updated to apply on current git master. This patch also fixes a problem in both WebKit1 and WebKit2 when the web view is destroyed with an active auth dialog, in WebKit1 the dialog remains open while in WebKit2 the UI process crashes.

[Carlos Garcia Campos]

Adding owners to CC, since the patch includes changes in WebKit2

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp', u'Source/WebCore/platform/gtk/GtkAuthenticationDialog.h', u'Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp', u'Source/WebCore/platform/gtk/WebKitAuthenticationWidget.h', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Source/WebKit/gtk/po/ChangeLog', u'Source/WebKit/gtk/po/POTFILES.in', u'Source/WebKit/gtk/webkit/webkitauthenticationdialog.cpp', u'Source/WebKit/gtk/webkit/webkitauthenticationdialog.h', u'Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h', u'Source/autotools/Versions.m4', u'Tools/ChangeLog', u'Tools/gtk/generate-gtkdoc']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h"
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:166:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:170:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:171:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:176:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:179:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:180:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 23 files


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

[Xan Lopez]

Comment on attachment 189975 [details]
Updated patch

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

This looks great to me, I think a common core and two small widgets for wk1 and wk2 makes this more readable. I just have two or three very minor comments, but r+ as far as I'm concerned.

> Source/WebKit/gtk/webkit/webkitauthenticationdialog.h:21
> +

I think it's probably not needed here but I'd just add the G_BEGIN_DECLS stuff for consistency's sake.

> Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:27
> +#include <wtf/text/CString.h>

This extra include seems odd?

> Source/autotools/Versions.m4:-18
> -m4_define([gtk2_required_version], [2.10])

If it's not a pain in the ass I'd just try to not bump this, since the gtk2 stuff is on its way out and asking people to bump it for a very minor feature seems odd. But no strong feelings about it.

[Xan Lopez]

Comment on attachment 189975 [details]
Updated patch

Ah, leave the r+ alone since it touches stuff in WebKit2 (even if it's only GTK).

[Carlos Garcia Campos]

Comment on attachment 189975 [details]
Updated patch

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

Thanks for the review!

>> Source/WebKit/gtk/webkit/webkitauthenticationdialog.h:21
>> +
> 
> I think it's probably not needed here but I'd just add the G_BEGIN_DECLS stuff for consistency's sake.

We don't use those macros in private headers, I think it doesn't even compile, since this is not a C header but a C++ one.

>> Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp:27
>> +#include <wtf/text/CString.h>
> 
> This extra include seems odd?

GtkAuthenticationDialog.h included CString, and it's used in this file.

>> Source/autotools/Versions.m4:-18
>> -m4_define([gtk2_required_version], [2.10])
> 
> If it's not a pain in the ass I'd just try to not bump this, since the gtk2 stuff is on its way out and asking people to bump it for a very minor feature seems odd. But no strong feelings about it.

I don't think anybody will have to bump gtk version, 2.16 is already too old. I'm not even sure current wk builds with gtk 2.10.

[Benjamin Poulain]

Comment on attachment 189975 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:30
> +    RefPtr<AuthenticationChallengeProxy> authenticationChallenge;

There is a proper "public" API for this: WKAuthenticationChallenge (already used by Mac and EFL). Why can't it be used here?

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 189975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189975&action=review

Thanks for the review.

> > Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:30
> > +    RefPtr<AuthenticationChallengeProxy> authenticationChallenge;
> 
> There is a proper "public" API for this: WKAuthenticationChallenge (already used by Mac and EFL). Why can't it be used here?

It can be used. We switched long time ago to use only the internal API, because the code is a lot simpler, to avoid unnecessary casts and memory allocations in some cases. We didn't know this was a problem for Apple, I've always considered the C API as public for wk users not for the ports, and we indeed expose both APIs, so that WebKitGTK+ users can use the GTK+ high level API or the C API directly. Switching back to use the C API now would be a lot of work and we are currently at the end of our release cycle.

[Benjamin Poulain]

I don't see anything that could hurt the project. I sign off on this for WebKit2.

> It can be used. We switched long time ago to use only the internal API, because the code is a lot simpler, to avoid unnecessary casts and memory allocations in some cases. We didn't know this was a problem for Apple, I've always considered the C API as public for wk users not for the ports, and we indeed expose both APIs, so that WebKitGTK+ users can use the GTK+ high level API or the C API directly. Switching back to use the C API now would be a lot of work and we are currently at the end of our release cycle.

I don't have a strong opinion regarding using the C API or not for all ports.

What I care about is:
1) correctness and testing.
2) hackability.

The nice thing I see about the recent move from everyone to the C API, is we have a common ground for testing: the WK C API. Anything exposed there can/should be tested by solid API tests.

Ports can have extra layers of tests on top (and I think all of them do now), but the common base is covered by common tests.

That being said, I dislike the C API as the interfacing layer. I wish WebKit2 did not grow into this giant mess such that the C API is considered a good idea :(

[Carlos Garcia Campos]

(In reply to comment #13)
> I don't see anything that could hurt the project. I sign off on this for WebKit2.
> 
> > It can be used. We switched long time ago to use only the internal API, because the code is a lot simpler, to avoid unnecessary casts and memory allocations in some cases. We didn't know this was a problem for Apple, I've always considered the C API as public for wk users not for the ports, and we indeed expose both APIs, so that WebKitGTK+ users can use the GTK+ high level API or the C API directly. Switching back to use the C API now would be a lot of work and we are currently at the end of our release cycle.
> 
> I don't have a strong opinion regarding using the C API or not for all ports.
> 
> What I care about is:
> 1) correctness and testing.
> 2) hackability.
> 
> The nice thing I see about the recent move from everyone to the C API, is we have a common ground for testing: the WK C API. Anything exposed there can/should be tested by solid API tests.

Well, in most of the cases the C API methods are just wrappers around an internal method, without any other logic, so it's not that relevant for testing purposes. We use the C API clients to receive the events. The C API is also fully tested by WTR and the TestWebKitAPI tests.

> Ports can have extra layers of tests on top (and I think all of them do now), but the common base is covered by common tests.

Indeed, unit tests are very important for us, and we don't accept patches adding new API without a solid unit test exercising the API. 

> That being said, I dislike the C API as the interfacing layer. I wish WebKit2 did not grow into this giant mess such that the C API is considered a good idea :(

Agree, we decided to switch also because in the last contributors meeting it was said that the C API was going to be removed or replaced by a C++ API at some point.

[Philippe Normand]

See also Bug 112248 - POTFILES.in needs to be updated!

[Carlos Garcia Campos]

(In reply to comment #15)
> See also Bug 112248 - POTFILES.in needs to be updated!

hmm, the patch actually includes those changes, the problem is that the POTFILES.in patch has changed in trunk since I wrote the patch, so it's not patched when applied.

[Carlos Garcia Campos]

Created attachment 193134 [details]
Updated patch

Updated for the POTFILE changes in trunk

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp', u'Source/WebCore/platform/gtk/GtkAuthenticationDialog.h', u'Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp', u'Source/WebCore/platform/gtk/WebKitAuthenticationWidget.h', u'Source/WebCore/platform/gtk/po/POTFILES.in', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Source/WebKit/gtk/webkit/webkitauthenticationdialog.cpp', u'Source/WebKit/gtk/webkit/webkitauthenticationdialog.h', u'Source/WebKit/gtk/webkit/webkitsoupauthdialog.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h', u'Source/autotools/Versions.m4', u'Tools/ChangeLog', u'Tools/gtk/generate-gtkdoc']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitAuthenticationDialog.h"
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:166:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:170:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:171:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:176:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:179:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/WebKitAuthenticationWidget.cpp:180:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 22 files


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

[Carlos Garcia Campos]

ping owners, this is just a refactoring

[Benjamin Poulain]

Comment on attachment 193134 [details]
Updated patch

I sign off on this.

[Xan Lopez]

Comment on attachment 193134 [details]
Updated patch

I assume Benjamin's comment means this is ok for the owners.

[Benjamin Poulain]

(In reply to comment #21)
> (From update of attachment 193134 [details])
> I assume Benjamin's comment means this is ok for the owners.

Yep. :)

I had expressed my concerns before about using the C API, but that should not be blocking.

[Carlos Garcia Campos]

Committed r148294: <http://trac.webkit.org/changeset/148294>