Bug 75433

Summary: [GTK] Rename webkit_web_view_load_alternate_html as webkit_web_view_replace_content in WebKit2 GTK+
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, mrobinson, pnormand, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 75465    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch without the WebPageProxy change
none
Updated patch mrobinson: review+

Description Carlos Garcia Campos 2012-01-02 05:25:45 PST
As discussed during the WebKitGTK+ hackfest the method load_alternate_content is a bit confusing. replace_content was suggested as a better name.
Comment 1 Carlos Garcia Campos 2012-01-02 05:30:21 PST
Created attachment 120872 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-02 05:33:01 PST
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
Comment 3 Martin Robinson 2012-01-02 08:20:19 PST
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?
Comment 4 Carlos Garcia Campos 2012-01-02 08:25:17 PST
(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.
Comment 5 Martin Robinson 2012-01-02 08:30:20 PST
> > > 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.
Comment 6 Carlos Garcia Campos 2012-01-02 08:33:53 PST
(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.
Comment 7 Carlos Garcia Campos 2012-01-03 01:32:42 PST
Created attachment 120919 [details]
Patch without the WebPageProxy change

Note that with this patch unit tests don't pass because of bug #75465
Comment 8 Carlos Garcia Campos 2012-01-04 02:17:22 PST
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.
Comment 9 Martin Robinson 2012-01-04 09:07:36 PST
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.
Comment 10 Carlos Garcia Campos 2012-01-05 01:03:17 PST
Ok, I'll land it with a workaround for the unit tests until #75465 is fixed.
Comment 11 Carlos Garcia Campos 2012-01-05 02:08:26 PST
Committed r104129: <http://trac.webkit.org/changeset/104129>