[WK2] WebFrameLoaderClient::shouldFallback() should use a port-specific implementation

[Jesus Sanchez-Palencia]

In WebKit 1 each port had its own implementation of FrameLoaderClient::shouldFallback since there was a port specific implementation of FrameLoaderClient itself. For instance, Qt and EFL use cancelledError and interruptedForPolicyChangeError to decide if they should fallback or not, while Mac only uses cancelledError and PlugInWillHandleLoad. GTK uses all of them.

However, in WebKit 2 all ports are currently sharing a single WebFrameLoaderClient implementation and, therefore, the same shouldFallback implementation. As I'm not aware of the fallback decisions for all ports, I believe this function should have a port specific implementation and it could sit on Source/WebKit2/WebProcess/WebCoreSupport/WebErrors.h.

[Jesus Sanchez-Palencia]

Created attachment 110909 [details]
Patch

[Luiz Agostini]

Comment on attachment 110909 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-985
> -    DEFINE_STATIC_LOCAL(const ResourceError, cancelledError, (this->cancelledError(ResourceRequest())));
> -    DEFINE_STATIC_LOCAL(const ResourceError, pluginWillHandleLoadError, (this->pluginWillHandleLoadError(ResourceResponse())));

r- because you ignored static and const in all the implementations of shouldFallBack that you provided.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebErrorsMac.mm:93
> +    ResourceError errorCancelled = cancelledError(ResourceRequest());
> +    ResourceError errorPluginWillHandleLoad = pluginWillHandleLoadError(ResourceResponse());

Ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebErrorsQt.cpp:91
> +    ResourceError errorCancelled = cancelledError(ResourceRequest());
> +    ResourceError errorPluginWillHandleLoad = pluginWillHandleLoadError(ResourceResponse());
> +    ResourceError errorInterruptedForPolicyChange = interruptedForPolicyChangeError(ResourceRequest());

Ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebErrorsWin.cpp:84
> +    ResourceError errorCancelled = cancelledError(ResourceRequest());
> +    ResourceError errorPluginWillHandleLoad = pluginWillHandleLoadError(ResourceResponse());

Ditto.

[Jesus Sanchez-Palencia]

Created attachment 111011 [details]
Patch

[Jesus Sanchez-Palencia]

(In reply to comment #2)
> r- because you ignored static and const in all the implementations of shouldFallBack that you provided.

Thanks for the review, Luis. I've uploaded a new patch here.

[Luiz Agostini]

Comment on attachment 111011 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebErrorsQt.cpp:91
> +    static const ResourceError errorCancelled = cancelledError(ResourceRequest());
> +    static const ResourceError errorPluginWillHandleLoad = pluginWillHandleLoadError(ResourceResponse());
> +    static const ResourceError errorInterruptedForPolicyChange = interruptedForPolicyChangeError(ResourceRequest());

Static local variables like these have indeterminated order of construction and destruction. They may cause really hard to find bugs.
These variables should probably leak (you should be using pointers).

Do you have strong reasons for not using the DEFINE_STATIC_LOCAL macro?

[Collabora GTK+ EWS bot]

Comment on attachment 111011 [details]
Patch

Attachment 111011 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10054607

[Gustavo Noronha (kov)]

Comment on attachment 111011 [details]
Patch

Attachment 111011 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10068412

[Jesus Sanchez-Palencia]

Created attachment 111093 [details]
Patch

[Luiz Agostini]

Comment on attachment 111093 [details]
Patch

This patch causes behavior changes in EFL and GTK ports. The guys that are responsible for those ports should be notified.

Looking at ResourceErrorBase.h, I think that ResourceError::isCancellation is not trivially equivalent to error.errorCode() == cancelledError.errorCode(). It may cause unwanted behavior changes in Mac and Win.

I would prefer to make sure that there will be behavior changes only in Qt port. You could file bugs to other ports if you think that they need changes as well.

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> (From update of attachment 111093 [details])
> This patch causes behavior changes in EFL and GTK ports. The guys that are responsible for those ports should be notified.

My idea was to keep the behavior they had on WebKit1 because I believe the behavior change was caused by WebFrameLoaderClient implementation itself and they weren't even aware of it. Of course I can be wrong and I'm already contacting people from both ports in order to gather their opinion.

> 
> Looking at ResourceErrorBase.h, I think that ResourceError::isCancellation is not trivially equivalent to error.errorCode() == cancelledError.errorCode(). It may cause unwanted behavior changes in Mac and Win.

You are right, there is no way to make sure people are handling this as we are in Qt, so I will modify the patch to use isCancellation() or the errorCode() accordingly to each port's previous implementation.

[Luiz Agostini]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 111093 [details] [details])
> > This patch causes behavior changes in EFL and GTK ports. The guys that are responsible for those ports should be notified.
> 
> My idea was to keep the behavior they had on WebKit1 because I believe the behavior change was caused by WebFrameLoaderClient implementation itself and they weren't even aware of it. Of course I can be wrong and I'm already contacting people from both ports in order to gather their opinion.
> 

Just let me know if they are aware of the changes and if they are ok with them.

> > 
> > Looking at ResourceErrorBase.h, I think that ResourceError::isCancellation is not trivially equivalent to error.errorCode() == cancelledError.errorCode(). It may cause unwanted behavior changes in Mac and Win.
> 
> You are right, there is no way to make sure people are handling this as we are in Qt, so I will modify the patch to use isCancellation() or the errorCode() accordingly to each port's previous implementation.

Nice.

Otherwise it looks good.

[Gustavo Noronha (kov)]

Comment on attachment 111093 [details]
Patch

I consider the code duplication to be unnecessary. For GTK+ I believe the current cross-platform shouldFallback is being enough. We might review our needs when we review all our APIs later this year. For the moment I would say an #ifdef inside shouldFallback adding/removing the checks as necessary for the ports that need it is the best way to go. If it turns out that most ports need customization that they can't share with others, than I would see this change as reasonable.

[Raphael Kubo da Costa (:rakuco)]

CC'ing the Samsung guys who probably know better about EFL's port of WebKit2. At first glance, we're fine with keeping the implementation unified, even if that changes behavior.

[Jesus Sanchez-Palencia]

(In reply to comment #12) and (In reply to comment #13):

Ok, guys, thanks for all your feedback. I was talking to Luiz on irc and following the comments we decided to add just a #ifdef for Qt for the time being. This way we don't change anything for any port but ours and if it ever gets to a point where we have reached an "ifdef mayhem" there, then we can move to something more specific like the proposed patch.

Just for the sake of curiosity, Luiz also agrees that a FrameLoaderClient impl per port on WebKit2 might not be an option.

I will upload a patch later with the changes for Qt. Thanks again for the feedback!

[Jesus Sanchez-Palencia]

Created attachment 111866 [details]
Patch

[WebKit Review Bot]

Comment on attachment 111866 [details]
Patch

Clearing flags on attachment: 111866

Committed r98138: <http://trac.webkit.org/changeset/98138>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.