Bug 99478 - Init timeout flag in ResourceErrorMac
: Init timeout flag in ResourceErrorMac
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Dominik Röttsches (drott)
:
Depends on:
Blocks: 74802
  Show dependency treegraph
 
Reported: 2012-10-16 10:42 PDT by Dominik Röttsches (drott)
Modified: 2012-10-16 14:05 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.48 KB, patch)
2012-10-16 10:53 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Patch (1.86 KB, patch)
2012-10-16 13:07 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-10-16 10:42:49 PDT
The timeout value is not initialized from NSError immediately. Since errorCode is in lazyInit, we can initialize the timeout flag there too.
Comment 1 Dominik Röttsches (drott) 2012-10-16 10:53:58 PDT
Created attachment 168974 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-10-16 11:01:45 PDT
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.
Comment 3 Dominik Röttsches (drott) 2012-10-16 13:07:37 PDT
Created attachment 169007 [details]
Patch
Comment 4 Dominik Röttsches (drott) 2012-10-16 13:11:29 PDT
(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?
Comment 5 Alexey Proskuryakov 2012-10-16 13:22:58 PDT
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).
Comment 6 WebKit Review Bot 2012-10-16 14:05:20 PDT
Comment on attachment 169007 [details]
Patch

Clearing flags on attachment: 169007

Committed r131499: <http://trac.webkit.org/changeset/131499>
Comment 7 WebKit Review Bot 2012-10-16 14:05:24 PDT
All reviewed patches have been landed.  Closing bug.