Bug 99478 – Init timeout flag in ResourceErrorMac

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dominik Röttsches (drott)]

The timeout value is not initialized from NSError immediately. Since errorCode is in lazyInit, we can initialize the timeout flag there too.

[Dominik Röttsches (drott)]

Created attachment 168974 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 168974 [details]
Patch

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

r-, because this breaks setIsTimeout().

> Source/WebCore/platform/network/ResourceErrorBase.h:50
>      bool isCancellation() const { return m_isCancellation; }

Notably, we don't need lazyInit() here.

> Source/WebCore/platform/network/ResourceErrorBase.h:53
>      void setIsTimeout(bool isTimeout) { m_isTimeout = isTimeout; }
> -    bool isTimeout() const { return m_isTimeout; }
> +    bool isTimeout() const { lazyInit(); return m_isTimeout; }

This means that even after a setIsTimeout(true) call, isTimeout() might return false. That's not an intuitively correct behavior.

[Dominik Röttsches (drott)]

Created attachment 169007 [details]
Patch

[Dominik Röttsches (drott)]

(In reply to comment #2)
> (From update of attachment 168974 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168974&action=review
> 
> r-, because this breaks setIsTimeout().

Thanks for the quick review.

> > Source/WebCore/platform/network/ResourceErrorBase.h:50
> >      bool isCancellation() const { return m_isCancellation; }
> 
> Notably, we don't need lazyInit() here.

Yes, the difference with isTimeout/setIsTimeout is that the true value can also come from the network side, not only from the webcore side. 

So how about an immediate initialization in the constructor?

[Alexey Proskuryakov]

Comment on attachment 169007 [details]
Patch

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

OK. There are still weird cases remaining (like what happens when one calls setIsTimeout() and then round-trips the error object via NSError or WebKit2 IPC), but that's true of many things in ResourceError class.

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:99
> +    if (!m_isNull)
> +        m_isTimeout = [m_platformError.get() code] == NSURLErrorTimedOut;

The null check is not strictly necessary - sending messages to null receivers generally works in Objective-C (unconditionally works when returned type is an integer).

[WebKit Review Bot]

Comment on attachment 169007 [details]
Patch

Clearing flags on attachment: 169007

Committed r131499: <http://trac.webkit.org/changeset/131499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.