Enabled actual transfer size in chromium

Enabled actual transfer size in chromium


BUG=40502
TEST=Open DevTools, open site having gzip/chunked encoding, ensure transfer size is correct.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80965

[pfeldman, 2011-04-01 08:48:05]

LGTM with nits. Overall looks good. Please fix style nits.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
File chrome/renderer/security_filter_peer.cc (right):

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:82: void
SecurityFilterPeer::OnReceivedData(const char* data, int data_length,
ditto

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:141: void
BufferedPeer::OnReceivedData(const char* data, int data_length,
diffo

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:190: void
ReplaceContentPeer::OnReceivedData(const char* data, int data_length,
ditto

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:206: static_cast<int>(data_.size()),
-1);
ditto

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
File chrome/renderer/security_filter_peer.h (right):

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:41: virtual void OnReceivedData(const
char* data, int data_length,
nit: one parameter per line.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:70: virtual void OnReceivedData(const
char* data, int data_length,
ditto

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:85: 
Remove this new line.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:112: virtual void OnReceivedData(const
char* data, int data_length,
ditto

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
File content/common/resource_dispatcher_unittest.cc (right):

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:59: virtual void
OnReceivedData(const char* data, int data_length,
nit: one parameter per line.

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:73: bool complete() const {
define in the header.

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:79: int lengthReceived() const {
either GetLengthReceived or length_received defined in .h (latter is better.

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:282: virtual void
OnReceivedData(const char* data, int data_length,
ditto, ditto, ditto...

[vsevik, 2011-04-01 09:50:59]

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
File chrome/renderer/security_filter_peer.cc (right):

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:82: void
SecurityFilterPeer::OnReceivedData(const char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:141: void
BufferedPeer::OnReceivedData(const char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> diffo

Done.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:190: void
ReplaceContentPeer::OnReceivedData(const char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.cc:206: static_cast<int>(data_.size()),
-1);
On 2011/04/01 08:48:06, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
File chrome/renderer/security_filter_peer.h (right):

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:41: virtual void OnReceivedData(const
char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> nit: one parameter per line.

Done.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:70: virtual void OnReceivedData(const
char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:85: 
On 2011/04/01 08:48:06, pfeldman wrote:
> Remove this new line.

Done.

http://codereview.chromium.org/6771043/diff/1/chrome/renderer/security_filter...
chrome/renderer/security_filter_peer.h:112: virtual void OnReceivedData(const
char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
File content/common/resource_dispatcher_unittest.cc (right):

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:59: virtual void
OnReceivedData(const char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> nit: one parameter per line.

Done.

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:73: bool complete() const {
On 2011/04/01 08:48:06, pfeldman wrote:
> define in the header.
It's a unit test without a header.

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:79: int lengthReceived() const {
On 2011/04/01 08:48:06, pfeldman wrote:
> either GetLengthReceived or length_received defined in .h (latter is better.
Renamed to length_received

http://codereview.chromium.org/6771043/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher_unittest.cc:282: virtual void
OnReceivedData(const char* data, int data_length,
On 2011/04/01 08:48:06, pfeldman wrote:
> ditto, ditto, ditto...

Done.

[vsevik, 2011-04-01 11:34:21]

Could you please review my changes?

[vsevik, 2011-04-01 12:22:40]

Hi jam,

Could you please review this?

[darin (slow to review), 2011-04-05 19:02:17]

some comments so far...  (interrupted, will return to reviewing)

http://codereview.chromium.org/6771043/diff/2003/content/browser/renderer_hos...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/6771043/diff/2003/content/browser/renderer_hos...
content/browser/renderer_host/async_resource_handler.cc:221: int bytes_received
= DevToolsNetLogObserver::GetAndResetTransferSize(request);
nit: it feels like there is too much different terminology going on here.

we have "Get*TransferSize", "bytes_received", and at the WebKit API
"lengthReceived".  these all mean the same thing.  one term would be better than
three :)

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
content/common/resource_dispatcher.cc:215: response->content_length =
result.content_length;
is this change related?

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
File content/common/resource_dispatcher_unittest.cc (right):

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
content/common/resource_dispatcher_unittest.cc:64: length_received_ +=
length_received;
here length_received_ means the accumulation of chunk sizes, but length_received
means the size of an individual chunk.  using the same term for two different
things is way confusing!

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
content/common/resource_dispatcher_unittest.cc:80: int length_received() const {
maybe using the prefix "total_" could help here?
total_raw_bytes_received is more clear.

[vsevik, 2011-04-07 16:32:49]

http://codereview.chromium.org/6771043/diff/2003/content/browser/renderer_hos...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/6771043/diff/2003/content/browser/renderer_hos...
content/browser/renderer_host/async_resource_handler.cc:221: int bytes_received
= DevToolsNetLogObserver::GetAndResetTransferSize(request);
As discussed, everything with this meaning is renamed to raw_data_length.

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
content/common/resource_dispatcher.cc:215: response->content_length =
result.content_length;
On 2011/04/05 19:02:18, darin wrote:
> is this change related?
Yes, but another field should have been used.
I added raw_data_length to ResourceResponseInfo for sync requests.

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
File content/common/resource_dispatcher_unittest.cc (right):

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
content/common/resource_dispatcher_unittest.cc:64: length_received_ +=
length_received;
Renamed to raw_data_length/total_raw_data_length.

http://codereview.chromium.org/6771043/diff/2003/content/common/resource_disp...
content/common/resource_dispatcher_unittest.cc:80: int length_received() const {
On 2011/04/05 19:02:18, darin wrote:
> maybe using the prefix "total_" could help here?
> total_raw_bytes_received is more clear.
Renamed to raw_data_length/total_raw_data_length.

[jam, 2011-04-07 16:46:34]

sorry for the delay, just saw this (in the future, please feel free to IM me
after a few hours of not responding to a code review).

[jam, 2011-04-07 16:48:25]

change lgtm on a high level, will defer to darin and others for details

[pfeldman, 2011-04-07 16:59:07]

I'd like to clarify whether single owner's LGTM is sufficient to land a change
like this. Or do we need all of them to gather together, consider the upsides
and downsides of a patch and come up with the recommendations. Note that this
change is trivial and was uploaded a week ago.

[darin (slow to review), 2011-04-08 15:31:12]

Single owner's LGTM is sufficient.  You just need to have LGTM coverage across
all of the files you are modifying.

[darin (slow to review), 2011-04-08 15:47:18]

LGTM w/ some nits/concerns:

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/multipart_respo...
File webkit/glue/multipart_response_delegate.cc (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/multipart_respo...
webkit/glue/multipart_response_delegate.cc:196: raw_data_length_ = 0;
it can sometimes be dangerous to assume that |this| is still valid after calling
out to a client interface.  given that other calls to didReceiveData are already
followed by dereferencing |this|, it is probably safe to assume that it is safe
in this case too, but are you sure?  are you sure that the
MultipartResponseDelegate isn't destroyed during the call to didReceiveData?

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/resource_loader...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/resource_loader...
webkit/glue/resource_loader_bridge.h:307: // be called multiple times or not at
all if an error occurs.
please document raw_data_length

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:702: OnReceivedData(data.data(), data.size(),
0);
hmm... passing 0 is curious, but it makes sense since this came from a Data URL.

http://codereview.chromium.org/6771043/diff/12002/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_resource_loader_bridge.cc:308:
peer_->OnReceivedData(buf_copy.get(), bytes_read, -1);
so -1 means unknown?  is that documented somewhere?

[vsevik, 2011-04-08 16:23:47]

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/multipart_respo...
File webkit/glue/multipart_response_delegate.cc (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/multipart_respo...
webkit/glue/multipart_response_delegate.cc:196: raw_data_length_ = 0;
On 2011/04/08 15:47:18, darin wrote:
> it can sometimes be dangerous to assume that |this| is still valid after
calling
> out to a client interface.  given that other calls to didReceiveData are
already
> followed by dereferencing |this|, it is probably safe to assume that it is
safe
> in this case too, but are you sure?  are you sure that the
> MultipartResponseDelegate isn't destroyed during the call to didReceiveData?
Yes, I am sure. It is destroyed right after the OnCompletedRequest call.
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/webkit/glue/w...

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/resource_loader...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/resource_loader...
webkit/glue/resource_loader_bridge.h:307: // be called multiple times or not at
all if an error occurs.
On 2011/04/08 15:47:18, darin wrote:
> please document raw_data_length

Done.

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:702: OnReceivedData(data.data(), data.size(),
0);
On 2011/04/08 15:47:18, darin wrote:
> hmm... passing 0 is curious, but it makes sense since this came from a Data
URL.
Yes, we pass zero here (and for cache hits also) since there wasn't any network
activity.

http://codereview.chromium.org/6771043/diff/12002/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/6771043/diff/12002/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_resource_loader_bridge.cc:308:
peer_->OnReceivedData(buf_copy.get(), bytes_read, -1);
On 2011/04/08 15:47:18, darin wrote:
> so -1 means unknown?  is that documented somewhere?
Yes, -1 is unknown. It is now mentioned in the comment I added in
resource_loader_bridge.h

[darin (slow to review), 2011-04-11 18:50:33]

On Fri, Apr 8, 2011 at 9:23 AM, <vsevik@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/glue/multipart_respo...
> File webkit/glue/multipart_response_delegate.cc (right):
>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/glue/multipart_respo...
> webkit/glue/multipart_response_delegate.cc:196: raw_data_length_ = 0;
> On 2011/04/08 15:47:18, darin wrote:
>
>> it can sometimes be dangerous to assume that |this| is still valid
>>
> after calling
>
>> out to a client interface.  given that other calls to didReceiveData
>>
> are already
>
>> followed by dereferencing |this|, it is probably safe to assume that
>>
> it is safe
>
>> in this case too, but are you sure?  are you sure that the
>> MultipartResponseDelegate isn't destroyed during the call to
>>
> didReceiveData?
> Yes, I am sure. It is destroyed right after the OnCompletedRequest call.
>
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/webkit/glue/w...
>
>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/glue/resource_loader...
> File webkit/glue/resource_loader_bridge.h (right):
>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/glue/resource_loader...
> webkit/glue/resource_loader_bridge.h:307: // be called multiple times or
> not at all if an error occurs.
> On 2011/04/08 15:47:18, darin wrote:
>
>> please document raw_data_length
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/glue/weburlloader_im...
> File webkit/glue/weburlloader_impl.cc (right):
>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/glue/weburlloader_im...
> webkit/glue/weburlloader_impl.cc:702: OnReceivedData(data.data(),
> data.size(), 0);
> On 2011/04/08 15:47:18, darin wrote:
>
>> hmm... passing 0 is curious, but it makes sense since this came from a
>>
> Data URL.
> Yes, we pass zero here (and for cache hits also) since there wasn't any
> network activity.
>
>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/tools/test_shell/sim...
> File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):
>
>
>
http://codereview.chromium.org/6771043/diff/12002/webkit/tools/test_shell/sim...
> webkit/tools/test_shell/simple_resource_loader_bridge.cc:308:
> peer_->OnReceivedData(buf_copy.get(), bytes_read, -1);
> On 2011/04/08 15:47:18, darin wrote:
>
>> so -1 means unknown?  is that documented somewhere?
>>
> Yes, -1 is unknown. It is now mentioned in the comment I added in
> resource_loader_bridge.h


OK, thanks!


>
>
> http://codereview.chromium.org/6771043/
>