[Webkit-unassigned] [Bug 136692] Race condition with WebKitWebView:is-loading after starting page load

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237878|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-09-10 02:03:16 PST ---
(From update of attachment 237878)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.