Bug 103277 – [GTK] [WebKit2] Embed the HTTP authentication dialog into the WebView

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Instead of popping up a separate dialog, we can embed the dialog directly into the WebView like Chromium.

[Martin Robinson]

Created attachment 176382 [details]
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 176382 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.h"
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:197:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:201:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:202:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:206:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:209:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:210:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 10 files


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

[Carlos Garcia Campos]

Comment on attachment 176382 [details]
Patch

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

This is awesome! the dialog looks great embedded in the web view. But I think the implementation is a bit complex because we are trying to share the dialog code but we don't want a dialog anymore in webkit2. So, I think we could move the dialog specific code to WebKit1 and create a widget in webcore with the auth logic that can be added to a dialog or any other widget. The API could be very simple:

WebKitAuth* webkitAuthNew(const AuthenticationChallenge& challenge);
void webkitAuthAuthenticate(WebKitAuth*);
void webkitAuthCancel(WebKitAuth*);

The dialog would simply add the widget to its content area, and call authentiate or cancel in the response callback depending on the button clicked. In WebKit2 we create a simple widget with the WebKitAuth and ok/cancel buttons.

> Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.cpp:54
> +    m_dialog = gtk_event_box_new();

Instead of an event box we could use a custom widget an override realize to crerate a GdkWindow.

> Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.cpp:65
> +    gtk_style_context_add_class(m_styleContext.get(), GTK_STYLE_CLASS_BACKGROUND);
> +    GtkWidgetPath* path = gtk_widget_path_new();
> +    gtk_widget_path_append_type(path, GTK_TYPE_WINDOW);
> +    gtk_style_context_set_path(m_styleContext.get(), path);
> +    gtk_widget_path_free(path);

With a custom widget, we add the GtkWindow path in the init method and then we can use our own style context in draw instead of creating a new one.

> Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.cpp:85
> +    webkitWebViewBaseAddAuthenticationDialog(WEBKIT_WEB_VIEW_BASE(m_webView), this);
> +    GtkAuthenticationDialog::show();

It's a bit strange that the dialog adds itself to the view.

> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:121
> +    (new WebKit2GtkAuthenticationDialog(WEBKIT_WEB_VIEW(clientInfo), toImpl(authenticationChallenge)))->show();

I think this should be handled by the WebView, here we call something like webkitWebViewAuthenticate (WEBKIT_WEB_VIEW(clientInfo), toImpl(authenticationChallenge));

The view then creates the auth widget and adds it to itself. In load-changed, we can check if there's an auth widget and destroy it when a new load starts, instead of connecting to load-changed from the auth widget. When we implement the auth api this will only be done if the user doesn't hander el authenticate signal.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:109
> +    WebKit::WebKit2GtkAuthenticationDialog* authenticationDialog;

I think there's a using namespace WebKit.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:355
>  void webkitWebViewBaseChildMoveResize(WebKitWebViewBase* webView, GtkWidget* child, const IntRect& childRect)
>  {
> -    const IntRect& geometry = webView->priv->children.get(child);
> +    if (webkitWebViewChildIsInternalWidget(webView, child))
> +        return;
>  
> +    const IntRect& geometry = webView->priv->children.get(child);

This is only called when a plugin geometry changes, this is already supposed to be called only for non internal children.

[Martin Robinson]

(In reply to comment #4)
> (From update of attachment 176382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176382&action=review
> 
> This is awesome! the dialog looks great embedded in the web view. But I think the implementation is a bit complex because we are trying to share the dialog code but we don't want a dialog anymore in webkit2. So, I think we could move the dialog specific code to WebKit1 and create a widget in webcore with the auth logic that can be added to a dialog or any other widget. The API could be very simple:
> 
> WebKitAuth* webkitAuthNew(const AuthenticationChallenge& challenge);
> void webkitAuthAuthenticate(WebKitAuth*);
> void webkitAuthCancel(WebKitAuth*);

First, thanks for the suggestions. You have some interesting ideas. I see two main suggestions for redesign in your comments here: the creation of a new widget subclass and the movement of the dialog specific code to WebKit1. I'll address each of these points separately:

1. Creating a widget subclass

I'm somewhat reluctant to do this for several reasons. The first is that I based this implementation on the Chromium implementation, so I'm more confidant that it can work with GTK+ 2.0. This is important because I'm considering writing another patch to extend the embedded dialog for WebKit1. During the implementation I talked about this approach with Benjamin Otte (indeed he suggested overriding the draw signal, as I've done), so I feel fairly confidant that it isn't a complete hack. Creating a widget subclass would also be many more lines of code than what I use here. The bits responsible for drawing the widget background must surely be less than 10 lines.

2. Moving the dialog specific code to WebKit1

You make a good point about the class knowledge being convoluted. It's weird that the superclass needs to expose a method to create the dialog guts. I think that I'll wait to fix this when I create the patch for WebKit1 though. Here is my proposed class hierarchy:

GtkAuthenticationWidget (abstract base class)
   - GtkAuthenticationDialog - GtkDialog implementation for use with webkitsoupauthenticationdialog
   - EmbeddedGtkAuthenticationWidget - Embedded version
   - WebKit2EmbeddedGtkAuthenticationWidget - WebKit2 version of the embedded widget (that uses UIProcess classes for authentication).

In this approach GtkAuthenticationDialog would move to WebKit1, while 
EmbeddableGtkAuthenticationWidget would be in WebCore.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.cpp:85
> > +    webkitWebViewBaseAddAuthenticationDialog(WEBKIT_WEB_VIEW_BASE(m_webView), this);
> > +    GtkAuthenticationDialog::show();
> 
> It's a bit strange that the dialog adds itself to the view.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:121
> > +    (new WebKit2GtkAuthenticationDialog(WEBKIT_WEB_VIEW(clientInfo), toImpl(authenticationChallenge)))->show();
> 
> I think this should be handled by the WebView, here we call something like webkitWebViewAuthenticate (WEBKIT_WEB_VIEW(clientInfo), toImpl(authenticationChallenge));
> 
> The view then creates the auth widget and adds it to itself. In load-changed, we can check if there's an auth widget and destroy it when a new load starts, instead of connecting to load-changed from the auth widget. When we implement the auth api this will only be done if the user doesn't hander el authenticate signal.

Believe it or not I started with a design like this one. :) The WebView needs to track the destruction of the dialog to disconnect the signal handler. The issue is that the WebKitWebView does not have any notification when the dialog is destroyed and the use of a weak pointer was causing issues for me. I can try this again though.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:109
> > +    WebKit::WebKit2GtkAuthenticationDialog* authenticationDialog;
> 
> I think there's a using namespace WebKit.

Okay. I'll fix this.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:355
> >  void webkitWebViewBaseChildMoveResize(WebKitWebViewBase* webView, GtkWidget* child, const IntRect& childRect)
> >  {
> > -    const IntRect& geometry = webView->priv->children.get(child);
> > +    if (webkitWebViewChildIsInternalWidget(webView, child))
> > +        return;
> >  
> > +    const IntRect& geometry = webView->priv->children.get(child);
> 
> This is only called when a plugin geometry changes, this is already supposed to be called only for non internal children.

I'll try removing it.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 176382 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=176382&action=review
> > 
> > This is awesome! the dialog looks great embedded in the web view. But I think the implementation is a bit complex because we are trying to share the dialog code but we don't want a dialog anymore in webkit2. So, I think we could move the dialog specific code to WebKit1 and create a widget in webcore with the auth logic that can be added to a dialog or any other widget. The API could be very simple:
> > 
> > WebKitAuth* webkitAuthNew(const AuthenticationChallenge& challenge);
> > void webkitAuthAuthenticate(WebKitAuth*);
> > void webkitAuthCancel(WebKitAuth*);
> 
> First, thanks for the suggestions. You have some interesting ideas. I see two main suggestions for redesign in your comments here: the creation of a new widget subclass and the movement of the dialog specific code to WebKit1. I'll address each of these points separately:
> 
> 1. Creating a widget subclass
> 
> I'm somewhat reluctant to do this for several reasons. The first is that I based this implementation on the Chromium implementation, so I'm more confidant that it can work with GTK+ 2.0. 

Creating a widget subclass won't prevent it to work with GTK+ 2.

> This is important because I'm considering writing another patch to extend the embedded dialog for WebKit1.

Not sure it's worth it, considering wk1 is somehow deprecated now.

> During the implementation I talked about this approach with Benjamin Otte (indeed he suggested overriding the draw signal, as I've done), so I feel fairly confidant that it isn't a complete hack.

Using an event box and creating a different style context just to draw its background sounds like a hack to me.

> Creating a widget subclass would also be many more lines of code than what I use here. The bits responsible for drawing the widget background must surely be less than 10 lines.

Yes, it's a bit more boilerplate for the widget implementation, but the code that uses the widget will be a lot more simple. I can do it in a follow up patch if you don't want to write GObject boilerplate. 

> 
> 2. Moving the dialog specific code to WebKit1
> 
> You make a good point about the class knowledge being convoluted. It's weird that the superclass needs to expose a method to create the dialog guts. I think that I'll wait to fix this when I create the patch for WebKit1 though. Here is my proposed class hierarchy:
> 
> GtkAuthenticationWidget (abstract base class)
>    - GtkAuthenticationDialog - GtkDialog implementation for use with webkitsoupauthenticationdialog
>    - EmbeddedGtkAuthenticationWidget - Embedded version
>    - WebKit2EmbeddedGtkAuthenticationWidget - WebKit2 version of the embedded widget (that uses UIProcess classes for authentication).
> 
> In this approach GtkAuthenticationDialog would move to WebKit1, while 
> EmbeddableGtkAuthenticationWidget would be in WebCore.

Why keeping a base C++ class? what we want is a common widget that can be added to the dialog and the embedded dialog, which makes its use simpler, beause you just need to create the widget and add it to the container. 

> > > Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.cpp:85
> > > +    webkitWebViewBaseAddAuthenticationDialog(WEBKIT_WEB_VIEW_BASE(m_webView), this);
> > > +    GtkAuthenticationDialog::show();
> > 
> > It's a bit strange that the dialog adds itself to the view.
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:121
> > > +    (new WebKit2GtkAuthenticationDialog(WEBKIT_WEB_VIEW(clientInfo), toImpl(authenticationChallenge)))->show();
> > 
> > I think this should be handled by the WebView, here we call something like webkitWebViewAuthenticate (WEBKIT_WEB_VIEW(clientInfo), toImpl(authenticationChallenge));
> > 
> > The view then creates the auth widget and adds it to itself. In load-changed, we can check if there's an auth widget and destroy it when a new load starts, instead of connecting to load-changed from the auth widget. When we implement the auth api this will only be done if the user doesn't hander el authenticate signal.
> 
> Believe it or not I started with a design like this one. :) The WebView needs to track the destruction of the dialog to disconnect the signal handler. The issue is that the WebKitWebView does not have any notification when the dialog is destroyed and the use of a weak pointer was causing issues for me. I can try this again though.

You don't need to track the destruction, because you don't need to use a signal. The WebView keeps already a pointer to the dialog, so when a new load starts, you can check if there's a dialog active and destroy it before emitting the load-started signal. When the widget is destroyed it's automatically removed from the container, and you are already setting the pointer to NULL in webkitWebViewBaseContainerRemove.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:109
> > > +    WebKit::WebKit2GtkAuthenticationDialog* authenticationDialog;
> > 
> > I think there's a using namespace WebKit.
> 
> Okay. I'll fix this.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:355
> > >  void webkitWebViewBaseChildMoveResize(WebKitWebViewBase* webView, GtkWidget* child, const IntRect& childRect)
> > >  {
> > > -    const IntRect& geometry = webView->priv->children.get(child);
> > > +    if (webkitWebViewChildIsInternalWidget(webView, child))
> > > +        return;
> > >  
> > > +    const IntRect& geometry = webView->priv->children.get(child);
> > 
> > This is only called when a plugin geometry changes, this is already supposed to be called only for non internal children.
> 
> I'll try removing it.

[Martin Robinson]

(In reply to comment #6)


> > This is important because I'm considering writing another patch to extend the embedded dialog for WebKit1.
> 
> Not sure it's worth it, considering wk1 is somehow deprecated now.

It isn't unfortunately and doesn't look like it will be for some time. We just aren't working on it very much these days. As much as I'd like to deprecate it and GTK+ 2.0, it looks like we should ship them both for at least one more cycle.

> > During the implementation I talked about this approach with Benjamin Otte (indeed he suggested overriding the draw signal, as I've done), so I feel fairly confidant that it isn't a complete hack.
> 
> Using an event box and creating a different style context just to draw its background sounds like a hack to me.

I guess that's your opinion? If you consider it for a moment, we have to do the exact same thing for a custom widget (add the  background style class and implement a custom draw signal).  It would make more sense to me if we needed to implement many custom behaviors. I just need an EventBox with a background.

Carlos, I really appreciate your reviews, I truly do. They are some of the best I receive. In this case though, I feel like you are suggesting changes without expressing why they are important for both future readers of the code and for the functionality of the library. Is there a reason to add many more lines of code other than your personal aesthetic preferences?

> > You make a good point about the class knowledge being convoluted. It's weird that the superclass needs to expose a method to create the dialog guts. I think that I'll wait to fix this when I create the patch for WebKit1 though. Here is my proposed class hierarchy:
> > 
> > GtkAuthenticationWidget (abstract base class)
> >    - GtkAuthenticationDialog - GtkDialog implementation for use with webkitsoupauthenticationdialog
> >    - EmbeddedGtkAuthenticationWidget - Embedded version
> >    - WebKit2EmbeddedGtkAuthenticationWidget - WebKit2 version of the embedded widget (that uses UIProcess classes for authentication).
> > 
> > In this approach GtkAuthenticationDialog would move to WebKit1, while 
> > EmbeddableGtkAuthenticationWidget would be in WebCore.
> 
> Why keeping a base C++ class? what we want is a common widget that can be added to the dialog and the embedded dialog, which makes its use simpler, beause you just need to create the widget and add it to the container. 

The reason is that with the GtkDialog case it doesn't need to containing GtkEventBox and GtkFrame that the embedded dialog contains.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> 
> 
> > > This is important because I'm considering writing another patch to extend the embedded dialog for WebKit1.
> > 
> > Not sure it's worth it, considering wk1 is somehow deprecated now.
> 
> It isn't unfortunately and doesn't look like it will be for some time. We just aren't working on it very much these days. As much as I'd like to deprecate it and GTK+ 2.0, it looks like we should ship them both for at least one more cycle.

Agree.

> > > During the implementation I talked about this approach with Benjamin Otte (indeed he suggested overriding the draw signal, as I've done), so I feel fairly confidant that it isn't a complete hack.
> > 
> > Using an event box and creating a different style context just to draw its background sounds like a hack to me.
> 
> I guess that's your opinion?

Of course, I said it looks like a hack to *me*

> If you consider it for a moment, we have to do the exact same thing for a custom widget (add the  background style class and implement a custom draw signal).

Having our own widget we could use the style context of the widget, instead of a different one, I think you would agree with me if the implementation didn't require writing GObjects, try to think of it as C++ classes, the natural thing would be to inherit and override the methods you want.

>  It would make more sense to me if we needed to implement many custom behaviors. I just need an EventBox with a background.

> Carlos, I really appreciate your reviews, I truly do. They are some of the best I receive. In this case though, I feel like you are suggesting changes without expressing why they are important for both future readers of the code and for the functionality of the library.

Using widget looks very natural to me in this case, and it simplifies the code, even if it requires to write some GObject boilerplate, the code that uses the widgets is simpler and easier to maintain.

> Is there a reason to add many more lines of code other than your personal aesthetic preferences?

No, I've commented based on my personal preferences, that's I haven't set r-, I never reject good patches that work just because I don't like them. You are free to simply ignore my suggestions. 

> > > You make a good point about the class knowledge being convoluted. It's weird that the superclass needs to expose a method to create the dialog guts. I think that I'll wait to fix this when I create the patch for WebKit1 though. Here is my proposed class hierarchy:
> > > 
> > > GtkAuthenticationWidget (abstract base class)
> > >    - GtkAuthenticationDialog - GtkDialog implementation for use with webkitsoupauthenticationdialog
> > >    - EmbeddedGtkAuthenticationWidget - Embedded version
> > >    - WebKit2EmbeddedGtkAuthenticationWidget - WebKit2 version of the embedded widget (that uses UIProcess classes for authentication).
> > > 
> > > In this approach GtkAuthenticationDialog would move to WebKit1, while 
> > > EmbeddableGtkAuthenticationWidget would be in WebCore.
> > 
> > Why keeping a base C++ class? what we want is a common widget that can be added to the dialog and the embedded dialog, which makes its use simpler, beause you just need to create the widget and add it to the container. 
> 
> The reason is that with the GtkDialog case it doesn't need to containing GtkEventBox and GtkFrame that the embedded dialog contains.

I'm not sure I understand this sentence.

[Carlos Garcia Campos]

I've written a patch to split GtkAuthDialog, I've just moved the code, so that you can add your improvements on top of it, let me know id that makes sense to you, see:

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

[Martin Robinson]

(In reply to comment #9)
> I've written a patch to split GtkAuthDialog, I've just moved the code, so that you can add your improvements on top of it, let me know id that makes sense to you, see:
> 
> https://bugs.webkit.org/show_bug.cgi?id=103644

Maybe you'd be willing to rewrite your patch on top of mine? I'd really like to land this and keep working on the other things I need to finish.

[Carlos Garcia Campos]

Comment on attachment 176382 [details]
Patch

Ok, let's land this for now, I don't want to give you more work and this works pretty well, and I'll try to find time to implement my suggestion afterwards. Please check the build before landing, because the EWS failed.

[Martin Robinson]

(In reply to comment #11)
> (From update of attachment 176382 [details])
> Ok, let's land this for now, I don't want to give you more work and this works pretty well, and I'll try to find time to implement my suggestion afterwards. Please check the build before landing, because the EWS failed.

I appreciate your understanding here. Something that I can do is that if I do this embedding for WebKit1, when I design the class hierarchy, I can make it so that it's easy to swap out the C++ class for a GtkWidget.

[Martin Robinson]

Created attachment 176766 [details]
Patch for landing

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.h"
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:197:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:201:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:202:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:206:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:209:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:210:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 14 files


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

[WebKit Review Bot]

Comment on attachment 176766 [details]
Patch for landing

Clearing flags on attachment: 176766

Committed r136152: <http://trac.webkit.org/changeset/136152>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.