Bug 90974 – [BlackBerry] Some small changes in network code

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

[Mary Wu]

Add some change in Network, from RIM PR# 171555.
1. Set status when NetworkJob closed so that its wrapped stream can also query the result after NetworkJob handling redirect/authenticate etc. 
2. pass download attribute to NetworkRequest.

[Mary Wu]

Created attachment 151684 [details]
Patch

[Joe Mason]

Comment on attachment 151684 [details]
Patch

What's the rationale behind overwriting m_status with a less-specific value on close?  Why couldn't the wrapped stream query the existing status?

[Mary Wu]

(In reply to comment #2)
> (From update of attachment 151684 [details])
> What's the rationale behind overwriting m_status with a less-specific value on close? 

Right now nobody would use the status() except download client who only cares if it's success or network error.

> Why couldn't the wrapped stream query the existing status?

There's some cases like StatusTooManyRedirects, wrapped stream would be unable to get the status.

[Rob Buis]

Comment on attachment 151684 [details]
Patch

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

Looks good.

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:483
> +                m_status = isError(m_extendedStatusCode) ? BlackBerry::Platform::FilterStream::StatusNetworkError: BlackBerry::Platform::FilterStream::StatusSuccess;

Style nit, please add a space before ':'

[Joe Mason]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 151684 [details] [details])
> > What's the rationale behind overwriting m_status with a less-specific value on close? 
> 
> Right now nobody would use the status() except download client who only cares if it's success or network error.

In that case can you add a separate "closeStatus" that the download client could check?  (I'm not sure what the best name is.)

The idea is that the status provided to notifyClose says whether the underlying stream was actually able to connect to its endpoint or not (so if it closes without ever connecting, you get notifyClose with an error code immediately) and the status in notifyStatusReceived is sent by the endpoint.  Whenever possible we put a detailed failure status in notifyStatusReceived as well as in notifyClose, but sometimes it's not possible so notifyClose is the fallback.

For example, if making a request to an http:// url, if the connection fails we may get notifyStatusReceived with a network error and then notifyClose with a network error, or just notifyClose (actually I forget the exact circumstance where notifyClose can happen without notifyStatusReceived, but it's theoretically possible).  If the connection succeeds but the server returns a 404, we get notifyStatusReceived(404) followed by notifyClose(Success).

I'd prefer to have a separate method that the download client can call which returns exactly the pass/fail status the download client is looking for rather than changing the behaviour of status.

[Lyon Chen]

(In reply to comment #5)
> In that case can you add a separate "closeStatus" that the download client could check?  (I'm not sure what the best name is.)
> 
> The idea is that the status provided to notifyClose says whether the underlying stream was actually able to connect to its endpoint or not (so if it closes without ever connecting, you get notifyClose with an error code immediately) and the status in notifyStatusReceived is sent by the endpoint.  Whenever possible we put a detailed failure status in notifyStatusReceived as well as in notifyClose, but sometimes it's not possible so notifyClose is the fallback.
> 
> For example, if making a request to an http:// url, if the connection fails we may get notifyStatusReceived with a network error and then notifyClose with a network error, or just notifyClose (actually I forget the exact circumstance where notifyClose can happen without notifyStatusReceived, but it's theoretically possible).  If the connection succeeds but the server returns a 404, we get notifyStatusReceived(404) followed by notifyClose(Success).
> 
> I'd prefer to have a separate method that the download client can call which returns exactly the pass/fail status the download client is looking for rather than changing the behaviour of status.

I remember there should be only one status, that is the http status, plus some internal error (which are negative to differentiated from normal http status). And this status is from notifyStatusReceived().

And the when merging WebSocket stream, we get separate status, notifyStatusReceived is now all about the connection establishment. And notifyClose() now comes a new status to tell status after the connection is established, like why it is closed, maybe due to some error (like lost network connection, the other end is down, etc).

So I would suggest we still treat them all as one status:

* It defaults to Success;
* When notifyStatusReceived() comes, it changes to what is in it;
* When notifyClose() comes, if it is Success, leave the status untouched, but if it is anything other than Success, then switch to new status.
* when NetworkJob::status() is called, we always return what is saved in m_status.

[Joe Mason]

(In reply to comment #6)
> I remember there should be only one status, that is the http status, plus some internal error (which are negative to differentiated from normal http status). And this status is from notifyStatusReceived().
> 
> And the when merging WebSocket stream, we get separate status, notifyStatusReceived is now all about the connection establishment. And notifyClose() now comes a new status to tell status after the connection is established, like why it is closed, maybe due to some error (like lost network connection, the other end is down, etc).

You're correct!  I was remembering the close status completely wrong.  Ignore my last comment.

> So I would suggest we still treat them all as one status:
> 
> * It defaults to Success;
> * When notifyStatusReceived() comes, it changes to what is in it;
> * When notifyClose() comes, if it is Success, leave the status untouched, but if it is anything other than Success, then switch to new status.
> * when NetworkJob::status() is called, we always return what is saved in m_status.

So that would just change line 483 in the patch to:

m_status = isError(status) ? status : m_status;

Sounds good to me!  I believe that would fix the StatusTooManyRedirects case: m_extendedStatus gets set right before that line, and then gets overwritten with another error if one happens (which is fine, the close error is more important.)

Except - wait a minute.  How does this patch even apply?  There is not "m_status" in NetworkJob.cpp, only m_extendedStatus.

[Lyon Chen]

(In reply to comment #7)
> 
> Except - wait a minute.  How does this patch even apply?  There is not "m_status" in NetworkJob.cpp, only m_extendedStatus.

m_status is a FilterStream protected member variable added in platform patch, so it should be fine.

Thought personally I would prefer not adding m_status to FilterStream, I think it should be just added to stream that maintain a status, like NetworkJob, DownloadFilterStream, or whatever stream that wants to maintain a status for the whole stream chain.

[Mary Wu]

> So that would just change line 483 in the patch to:

> m_status = isError(status) ? status : m_status;

Thanks for review, Joe. So with this change the m_status will possible be HTTP code or PlatformStatus.

[Mary Wu]

Created attachment 152204 [details]
Patch

[Joe Mason]

Comment on attachment 152204 [details]
Patch

Internally reviewed by myself and Lyon.

[Rob Buis]

Comment on attachment 152204 [details]
Patch

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

Looks good.

> Source/WebCore/platform/network/blackberry/NetworkJob.h:83
> +    virtual int status() { return m_extendedStatusCode; }

Could be const.

> Source/WebCore/platform/network/blackberry/SocketStreamHandle.h:60
> +    virtual int status() { return m_status; }

Could be const.

[Joe Mason]

Created attachment 152264 [details]
patch with const

Good catch. I'll go ahead and make that change since Mary's in Beijing and there's no sense in waiting another 12 hours.

[Rob Buis]

Comment on attachment 152264 [details]
patch with const

Thanks Joe, looks fine now.

[WebKit Review Bot]

Comment on attachment 152264 [details]
patch with const

Clearing flags on attachment: 152264

Committed r122604: <http://trac.webkit.org/changeset/122604>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.