Bug 90974 - [BlackBerry] Some small changes in network code
Summary: [BlackBerry] Some small changes in network code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mary Wu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-11 04:49 PDT by Mary Wu
Modified: 2012-07-13 10:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.59 KB, patch)
2012-07-11 04:59 PDT, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2012-07-13 03:14 PDT, Mary Wu
rwlbuis: review+
Details | Formatted Diff | Diff
patch with const (5.85 KB, patch)
2012-07-13 08:04 PDT, Joe Mason
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mary Wu 2012-07-11 04:49:44 PDT
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.
Comment 1 Mary Wu 2012-07-11 04:59:45 PDT
Created attachment 151684 [details]
Patch
Comment 2 Joe Mason 2012-07-11 09:39:48 PDT
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?
Comment 3 Mary Wu 2012-07-11 18:43:51 PDT
(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.
Comment 4 Rob Buis 2012-07-12 07:48:33 PDT
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 ':'
Comment 5 Joe Mason 2012-07-12 07:57:43 PDT
(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.
Comment 6 Lyon Chen 2012-07-12 08:20:03 PDT
(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.
Comment 7 Joe Mason 2012-07-12 13:38:44 PDT
(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.
Comment 8 Lyon Chen 2012-07-12 13:43:41 PDT
(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.
Comment 9 Mary Wu 2012-07-13 02:23:20 PDT
> 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.
Comment 10 Mary Wu 2012-07-13 03:14:21 PDT
Created attachment 152204 [details]
Patch
Comment 11 Joe Mason 2012-07-13 07:27:38 PDT
Comment on attachment 152204 [details]
Patch

Internally reviewed by myself and Lyon.
Comment 12 Rob Buis 2012-07-13 07:44:56 PDT
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.
Comment 13 Joe Mason 2012-07-13 08:04:47 PDT
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.
Comment 14 Rob Buis 2012-07-13 08:08:08 PDT
Comment on attachment 152264 [details]
patch with const

Thanks Joe, looks fine now.
Comment 15 WebKit Review Bot 2012-07-13 10:40:13 PDT
Comment on attachment 152264 [details]
patch with const

Clearing flags on attachment: 152264

Committed r122604: <http://trac.webkit.org/changeset/122604>
Comment 16 WebKit Review Bot 2012-07-13 10:40:18 PDT
All reviewed patches have been landed.  Closing bug.