One for the common auth logic and another one with the dialog specific things.
Created attachment 176741 [details] Patch
Created attachment 177218 [details] New patch
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
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.
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.
Adding owners to CC, since the patch includes changes in WebKit2
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.
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.
Comment on attachment 189975 [details] Updated patch Ah, leave the r+ alone since it touches stuff in WebKit2 (even if it's only GTK).
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.
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?
(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.
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 :(
(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.
See also Bug 112248 - POTFILES.in needs to be updated!
(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.
Created attachment 193134 [details] Updated patch Updated for the POTFILE changes in trunk
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.
ping owners, this is just a refactoring
Comment on attachment 193134 [details] Updated patch I sign off on this.
Comment on attachment 193134 [details] Updated patch I assume Benjamin's comment means this is ok for the owners.
(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.
Committed r148294: <http://trac.webkit.org/changeset/148294>