Bug 91482 – [GTK] No main resource in WebView on load committed when page has been loaded from history cache

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This is similar to bug #91478, but we were not only assuming we have a main resource with a response when page is loaded, but also when load-committed is emitted. Again, this is true for normal loads and reloads, but not for pages loaded from the history cache. See also bug #91220 that fixed a crash because of this assumption.

[Carlos Garcia Campos]

Created attachment 152725 [details]
Patch

[Martin Robinson]

Comment on attachment 152725 [details]
Patch

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

Looks good overall. I wonder if it would make sense to fix this behavior in WebCore or WebKit2 instead though. Regardless, I have a few suggestions below.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1148
> +        // Finish a possible previous load waiting for main resource.
> +        webkitWebViewEmitDelayedLoadEvents(webView);

What sort of situation does WEBKIT_LOAD_STARTED happen without the main resource response happening? It might be good to specify this situation directly in this comment. Right now it answers the "What?" but not the "Why?"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1155
> +        setCertificateToMainResourceOrWait(webView);

I think it might be good to move

priv->waitingForMainResource = true;

to here and get rid of setCertificateToMainResourceOrWait. Right now it feels like it's a side-effect of setCertificateToMainResourceOrWait, while it affects more than just certificate handling (also emitting delayed loading events), so it's somewhat "hidden" away.

Also above that line I believe a comment explaining why we sometimes wait for the resource to load -- explaining the situation of back and forward with cached resources.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 152725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152725&action=review
> 
> Looks good overall. I wonder if it would make sense to fix this behavior in WebCore or WebKit2 instead though. Regardless, I have a few suggestions below.

It's not necessarily a bug in WebCore, it has always worked that way. This also happens in wk1 indeed, but in wk1 you can get the main resource directly from the loader, so you don't need to wait until load resource signals are emitted.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1148
> > +        // Finish a possible previous load waiting for main resource.
> > +        webkitWebViewEmitDelayedLoadEvents(webView);
> 
> What sort of situation does WEBKIT_LOAD_STARTED happen without the main resource response happening? It might be good to specify this situation directly in this comment. Right now it answers the "What?" but not the "Why?"

I think this was a paranoid check, I'm not sure that situation can happen.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1155
> > +        setCertificateToMainResourceOrWait(webView);
> 
> I think it might be good to move
> 
> priv->waitingForMainResource = true;
> 
> to here and get rid of setCertificateToMainResourceOrWait. Right now it feels like it's a side-effect of setCertificateToMainResourceOrWait, while it affects more than just certificate handling (also emitting delayed loading events), so it's somewhat "hidden" away.

So, are you proposing to move the contents of the helper function here?

> Also above that line I believe a comment explaining why we sometimes wait for the resource to load -- explaining the situation of back and forward with cached resources.

Fair enough.

[Martin Robinson]

Comment on attachment 152725 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1155
>>> +        setCertificateToMainResourceOrWait(webView);
>> 
>> I think it might be good to move
>> 
>> priv->waitingForMainResource = true;
>> 
>> to here and get rid of setCertificateToMainResourceOrWait. Right now it feels like it's a side-effect of setCertificateToMainResourceOrWait, while it affects more than just certificate handling (also emitting delayed loading events), so it's somewhat "hidden" away.
>> 
>> Also above that line I believe a comment explaining why we sometimes wait for the resource to load -- explaining the situation of back and forward with cached resources.
> 
> So, are you proposing to move the contents of the helper function here?

Sure, I think it would be sufficient just to move the contents of setCertificateToMainResourceOrWait here and to keep setCertificateToMainResource. It's mostly just the name, in fact, setCertificateToMainResourceOrWait is also important in situations when there is no certificate.

[Carlos Garcia Campos]

Created attachment 155796 [details]
Updated 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

[Carlos Garcia Campos]

Committed r124456: <http://trac.webkit.org/changeset/124456>