[Webkit-unassigned] [Bug 71554] [Qt] 4 Layout test fail due to network error constant values clash with WebkitError enum values in FrameloaderclientQt

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Deepak Sherveghar <bpwv64 at motorola.com>  2011-11-04 11:51:01 PST ---
(In reply to comment #4)
> However... why do the "clash" matter? The error domain is all different. When we put a QNetworkReply error in the ResourceError, we use a "QtWebKit" domain. When we put the http status code it in, the domain is "HTTP". And similarly when we put one of those errors in there, the domain is something else.
> 
> Do you know how the layout test failure relates to that? Where is the error() used and what is it compared to?

In my patch for bug:62108, I added a new condition in the method shouldFallBack
bool FrameLoaderClientQt::shouldFallBack(const WebCore::ResourceError& error)
{
 - return !(error.isCancellation() || (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange));
 + return !(error.isCancellation() || (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange) || (error.errorCode() == WebKitErrorPluginWillHandleLoad));
}

The layout test fail because of this new condition.

shouldFallBack() tells the DOM if it should attempt to render the next nested fallback content if its parent fails to load.
In the failing layout test, we have an <object> tag with invalid url, expecting the fallback content to be loaded and rendered.
Now the object url fails to load, QNetworkReplyHandler sets the QNetworkReply::NetworkError as 203 (ContentNotFoundError).
Webkit calls shouldFallBack to check if fallback content can be loaded. Now since in shouldFallBack() we only check for error code and not for domains(which we actually should), error code 203 matches with WebKitErrorPluginWillHandleLoad enum value and returns false.

Hence the test case fail because fallback contents are not rendered.

Just did a quick check with the codebase and found that chromium port and webkit2 are the only port that check for domain along with error code.
I think as explained by you, we should also add domain check along with error code in shouldFallBack().
With domain check in place, enum constant value changes would not be required.

I was thinking somewhere along the lines of webkit2, Something like below for shouldFallBack() 

bool FrameLoaderClientQt::shouldFallBack(const WebCore::ResourceError& error)
{
   ResourceError cancelledError = cancelledError(ResourceRequest());

   if (error.errorCode() == cancelledError.errorCode() && error.domain() == cancelledError.domain())
        return false;

   if (error.errorCode() == WebKitErrorFrameLoadInterruptedByPolicyChange  && error.domain() == String("WebKitErrorDomain"))
        return false;

   if (error.errorCode() == WebKitErrorPlugInWillHandleLoad  && error.domain() == String("WebKit"))
        return false;

   return true;
}

Does that seem reasonable ???

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