[webkit-reviews] review denied: [Bug 136692] Race condition with WebKitWebView:is-loading after starting page load : [Attachment 237878] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 136692: Race condition with WebKitWebView:is-loading after starting page
load
https://bugs.webkit.org/show_bug.cgi?id=136692

Attachment 237878: Patch
https://bugs.webkit.org/attachment.cgi?id=237878&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237878&action=review


I also started with this, I have a wip patch in my local repo, but it's blocked
by other differences between our API and the internal one regarding the active
URL.

> Source/WebKit2/PlatformGTK.cmake:169
> +    UIProcess/API/gtk/WebKitPageLoadObserver.cpp
> +    UIProcess/API/gtk/WebKitPageLoadObserver.h

I'm not sure we need a new file for this, I would add the class in
WebKitWebView.cpp, since it's a private class an only used by WebKitWebView.

> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:40
> -    webkitWebViewLoadChanged(WEBKIT_WEB_VIEW(clientInfo),
WEBKIT_LOAD_STARTED);
> +    ASSERT_UNUSED(clientInfo,
webkit_web_view_is_loading(WEBKIT_WEB_VIEW(clientInfo)));

I don't think this is correct, we should really call webkitWebViewLoadChanged
here.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:24
> +#include <WebCore/platform/NotImplemented.h>

This should be #include <WebCore/NotImplemented.h> but I don't think we need
this here.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:30
> +    , m_pageLoadState(pageLoadState)

I would leave the caller use PageLoadState to add/remove the observer

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:43
> +PassRefPtr<WebKitPageLoadObserver>
WebKitPageLoadObserver::create(WebKitWebView& webView, PageLoadState&
pageLoadState)
> +{
> +    return adoptRef(new WebKitPageLoadObserver(webView, pageLoadState));
> +}

I don't this this class nedds to be ref counted, it will not be shared, it
should be created by the web view in the constructor and deleted on dispose.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:47
> +    notImplemented();

Don't need to use notImplemented in this case, I think, since it's not
something that will not work because this is not implemented. We can use the
will* methods to freeze the gobject property notify.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.cpp:53
> +    if (m_pageLoadState.isLoading())
> +	   webkitWebViewLoadChanged(&m_webView, WEBKIT_LOAD_STARTED);

I don't think this is correct, we should only emit load-started when the frame
has transition to provisional state, and this is happening before. I agree on
changing the is-loading before as well as making sure that get_uri() right
after load_uri() doesn't return null but the requested uri, but for that we
should also implement the didChangeActiveURL.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.h:29
> +class WebKitPageLoadObserver : public RefCounted<WebKitPageLoadObserver>,
public PageLoadState::Observer {

This class could be marked as final, and again I don't think it needs to be
refcounted.

> Source/WebKit2/UIProcess/API/gtk/WebKitPageLoadObserver.h:62
> +    WebKitWebView& m_webView;

I don't use references for GObjects, use a pointer instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:102
> +    RefPtr<WebKitPageLoadObserver> pageLoadObserver;

I don't think this should be in WebKitWebViewBase, but in WebKitWebView.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1011
> +    priv->pageLoadObserver =
WebKitPageLoadObserver::create(*WEBKIT_WEB_VIEW(webkitWebViewBase),
priv->pageProxy->pageLoadState());

It's not correct to cast the webkitWebViewBase as WebKitWebView here.