Bug 75433 – [GTK] Rename webkit_web_view_load_alternate_html as webkit_web_view_replace_content in WebKit2 GTK+

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

As discussed during the WebKitGTK+ hackfest the method load_alternate_content is a bit confusing. replace_content was suggested as a better name.

[Carlos Garcia Campos]

Created attachment 120872 [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

[Martin Robinson]

Comment on attachment 120872 [details]
Patch

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

Looks fine to me, but I'm holding off on r+ until I understand the change in Source/WebKit2/UIProcess/WebPageProxy.cpp.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:685
> + * Replace the content of @web_view with @content using @content_uri as page URI.
> + * This allows clients to display page-loading errors in the #WebKitWebView itself.
> + * This is typically called from
> + * #WebKitWebLoaderClient::provisional-load-failed or #WebKitWebLoaderClient::load-failed
> + * signals.
> + * The URI passed in @base_uri has to be an absolute URI.

I think it could be useful here to explicitly mention that the difference between this and load_content is that this does not trigger load signals.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:623
>      if (!m_pendingAPIRequestURL.isNull())
>          return m_pendingAPIRequestURL;
>  
> -    // FIXME: What do we do in the case of the unreachable URL?
> +    if (!m_mainFrame->unreachableURL().isEmpty())
> +        return m_mainFrame->unreachableURL();
>  
>      switch (m_mainFrame->loadState()) {

Was this added accidentally?

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 120872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120872&action=review
> 
> Looks fine to me, but I'm holding off on r+ until I understand the change in Source/WebKit2/UIProcess/WebPageProxy.cpp.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:685
> > + * Replace the content of @web_view with @content using @content_uri as page URI.
> > + * This allows clients to display page-loading errors in the #WebKitWebView itself.
> > + * This is typically called from
> > + * #WebKitWebLoaderClient::provisional-load-failed or #WebKitWebLoaderClient::load-failed
> > + * signals.
> > + * The URI passed in @base_uri has to be an absolute URI.
> 
> I think it could be useful here to explicitly mention that the difference between this and load_content is that this does not trigger load signals.

Yes, I plan to do that in a new patch as soon as new loader client patch lands, because in this moment this method do actually trigger load signals. So, once we have the new loader client, I'll fix it to not trigger load signals when replacing content and I'll comment it here too.

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:623
> >      if (!m_pendingAPIRequestURL.isNull())
> >          return m_pendingAPIRequestURL;
> >  
> > -    // FIXME: What do we do in the case of the unreachable URL?
> > +    if (!m_mainFrame->unreachableURL().isEmpty())
> > +        return m_mainFrame->unreachableURL();
> >  
> >      switch (m_mainFrame->loadState()) {
> 
> Was this added accidentally?

No, when using loadAlternateHTML the active uri is actually the unreachableURL, since it's the uri you want to use for the new page contents. Othrewise this returns about:blank.

[Martin Robinson]

> > > Source/WebKit2/UIProcess/WebPageProxy.cpp:623
> > >      if (!m_pendingAPIRequestURL.isNull())
> > >          return m_pendingAPIRequestURL;
> > >  
> > > -    // FIXME: What do we do in the case of the unreachable URL?
> > > +    if (!m_mainFrame->unreachableURL().isEmpty())
> > > +        return m_mainFrame->unreachableURL();
> > >  
> > >      switch (m_mainFrame->loadState()) {
> > 
> > Was this added accidentally?
> 
> No, when using loadAlternateHTML the active uri is actually the unreachableURL, since it's the uri you want to use for the new page contents. Othrewise this returns about:blank.

It seems like this should be in another bug, since it isn't strictly renaming the method. Another reason I say this is that I'm confident enough to review the rename, but not familiar enough with this cross-platform code to review it. I'm not sure Anders and Sam are interested in reviewing the API change either. It seems like it should be split off.

[Carlos Garcia Campos]

(In reply to comment #5)
> > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:623
> > > >      if (!m_pendingAPIRequestURL.isNull())
> > > >          return m_pendingAPIRequestURL;
> > > >  
> > > > -    // FIXME: What do we do in the case of the unreachable URL?
> > > > +    if (!m_mainFrame->unreachableURL().isEmpty())
> > > > +        return m_mainFrame->unreachableURL();
> > > >  
> > > >      switch (m_mainFrame->loadState()) {
> > > 
> > > Was this added accidentally?
> > 
> > No, when using loadAlternateHTML the active uri is actually the unreachableURL, since it's the uri you want to use for the new page contents. Othrewise this returns about:blank.
> 
> It seems like this should be in another bug, since it isn't strictly renaming the method. Another reason I say this is that I'm confident enough to review the rename, but not familiar enough with this cross-platform code to review it. I'm not sure Anders and Sam are interested in reviewing the API change either. It seems like it should be split off.

You are right, I added it because it breaks the unit test, but I'll file a new bug report for it blocking this one.

[Carlos Garcia Campos]

Created attachment 120919 [details]
Patch without the WebPageProxy change

Note that with this patch unit tests don't pass because of bug #75465

[Carlos Garcia Campos]

Created attachment 121088 [details]
Updated patch

Updated to apply on current git master and added checks to make usre load signals are not emitted when replacing content, now that new loader client patch landed.

[Martin Robinson]

Comment on attachment 121088 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:782
> + * Replace the content of @web_view with @content using @content_uri as page URI.
> + * This allows clients to display page-loading errors in the #WebKitWebView itself.
> + * This is typically called from #WebKitWebView::load-failed signal. The URI passed in
> + * @base_uri has to be an absolute URI.
> + * Signals #WebKitWebView::load-changed and #WebKitWebView::load-failed are not emitted
> + * when replacing content of a #WebKitWebView using this method.

Oh, there is one more thing I forgot to mention here. The documentation should also state that the content type will be HTML. This is one way this method differs from load_content.

[Carlos Garcia Campos]

Ok, I'll land it with a workaround for the unit tests until #75465 is fixed.

[Carlos Garcia Campos]

Committed r104129: <http://trac.webkit.org/changeset/104129>