Propagate the encoded data length through OnDownloadData delegate method

Propagate the encoded data length through OnDownloadData delegate method

Since the encoded data length is intended to be used for the Inspector,
it will only be valid while devtools are attached. Otherwise it becomes -1,
-1 represents that the encoded data length is unknown.

To propagate the encoded data length to Blink's didDownloadData delegate
method, the IPC dispatcher provides it to Blink.
Blink-side CL: https://chromiumcodereview.appspot.com/23632004/

After this change is landed, we'll remove the stub code in Blink-side.

BUG=269055
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221024

[Avi (use Gerrit), 2013-08-28 14:27:19]

I don't know if AsyncResourceHandler::OnDataDownloaded is correct (out of my
expertise; ask someone else) but you have my LGTM for everything that it covers.

[yusukesuzuki, 2013-08-29 02:12:28]

On 2013/08/28 14:27:19, Avi wrote:
> I don't know if AsyncResourceHandler::OnDataDownloaded is correct (out of my
> expertise; ask someone else) but you have my LGTM for everything that it
covers.

Thanks for your review. I've added michalen to the reviewers.

[kinuko, 2013-08-29 06:03:42]

I'm probably not a good reviewer for this either, but anyway lgtm

[kinuko, 2013-08-29 06:04:39]

or just would like to defer to michaeln@ or someone who knows this part better

[yusukesuzuki, 2013-08-29 06:24:00]

On 2013/08/29 06:04:39, kinuko wrote:
> or just would like to defer to michaeln@ or someone who knows this part better

Thanks! I added michaeln to the reviewers, the original author of
AsyncResourceHandler::OnDataDownloaded.

[Nico, 2013-08-29 14:13:35]

../../webkit/child/weburlloader_impl.cc:676:44:error: too many arguments to
function call, expected 2, have 3
    client_->didDownloadData(loader_, len, encoded_data_length);
    ~~~~~~~~~~~~~~~~~~~~~~~~               ^~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/public/platform/WebURLLoaderClient.h:58:5: note:
'didDownloadData' declared here
    virtual void didDownloadData(WebURLLoader*, int dataLength) { }
    ^
1 error generated.

[michaeln, 2013-08-29 20:19:04]

pending green'ness lgtm

https://codereview.chromium.org/23696002/diff/1/content/browser/loader/async_...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/23696002/diff/1/content/browser/loader/async_...
content/browser/loader/async_resource_handler.cc:288:
DevToolsNetLogObserver::GetAndResetEncodedDataLength(request_);
I'm not familiar with the inspector instrumentation but i see the exact same
handling for the non-download-to-file case in OnReadCompleted so i trust it
returns valid results in this case too since this step replace that step in the
d-t-f control flow.

https://codereview.chromium.org/23696002/diff/1/webkit/child/weburlloader_imp...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/23696002/diff/1/webkit/child/weburlloader_imp...
webkit/child/weburlloader_impl.cc:676: client_->didDownloadData(loader_, len,
encoded_data_length);
need to do the two-sided patch dance here

[yusukesuzuki, 2013-08-30 01:08:10]

On 2013/08/29 14:13:35, Nico wrote:
> ../../webkit/child/weburlloader_impl.cc:676:44:error: too many arguments to
> function call, expected 2, have 3
>     client_->didDownloadData(loader_, len, encoded_data_length);
>     ~~~~~~~~~~~~~~~~~~~~~~~~               ^~~~~~~~~~~~~~~~~~~
> ../../third_party/WebKit/public/platform/WebURLLoaderClient.h:58:5: note:
> 'didDownloadData' declared here
>     virtual void didDownloadData(WebURLLoader*, int dataLength) { }
>     ^
> 1 error generated.

The Blink-side patch has been landed as rev 156838[1], so after Blink is
rebaselined, this patch will become green.

[1]: https://chromiumcodereview.appspot.com/23632004/

[yusukesuzuki, 2013-08-30 01:13:23]

https://chromiumcodereview.appspot.com/23696002/diff/1/webkit/child/weburlloa...
File webkit/child/weburlloader_impl.cc (right):

https://chromiumcodereview.appspot.com/23696002/diff/1/webkit/child/weburlloa...
webkit/child/weburlloader_impl.cc:676: client_->didDownloadData(loader_, len,
encoded_data_length);
On 2013/08/29 20:19:05, michaeln wrote:
> need to do the two-sided patch dance here

Right. This patch will become green after Blink is rebaselined[1].
After this patch is landed, I'll remove the stub code in Blink allowing 2
arguments style didDownloadData not to break the build.

[1]: https://chromiumcodereview.appspot.com/23632004/

[yusukesuzuki, 2013-08-30 06:17:55]

Blink is rolled and the patch becomes green.

[Nico, 2013-08-31 20:49:46]

https://chromiumcodereview.appspot.com/23696002/diff/1/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/23696002/diff/1/content/browser/loader...
content/browser/loader/async_resource_handler.cc:288:
DevToolsNetLogObserver::GetAndResetEncodedDataLength(request_);
On 2013/08/29 20:19:05, michaeln wrote:
> I'm not familiar with the inspector instrumentation but i see the exact same
> handling for the non-download-to-file case in OnReadCompleted so i trust it
> returns valid results in this case too since this step replace that step in
the
> d-t-f control flow.

I'm also curious about
this.DevToolsNetLogObserver::GetAndResetEncodedDataLength() seems to return -1
if dev_tools_net_log_observer == NULL, and instance_ in
content/browser/devtools/devtools_netlog_observer.cc seems to be only non-NULL
after Attach() was called (and before Detach() was called), which after skimming
seems to be only the case while devtools is active (?)

https://chromiumcodereview.appspot.com/23696002/diff/1/webkit/child/resource_...
File webkit/child/resource_loader_bridge.h (right):

https://chromiumcodereview.appspot.com/23696002/diff/1/webkit/child/resource_...
webkit/child/resource_loader_bridge.h:155: // that case, OnReceivedData will not
be called.
Document encoded_data_length here.

[yusukesuzuki, 2013-09-02 02:05:09]

https://chromiumcodereview.appspot.com/23696002/diff/1/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/23696002/diff/1/content/browser/loader...
content/browser/loader/async_resource_handler.cc:288:
DevToolsNetLogObserver::GetAndResetEncodedDataLength(request_);
On 2013/08/31 20:49:47, Nico wrote:
> On 2013/08/29 20:19:05, michaeln wrote:
> > I'm not familiar with the inspector instrumentation but i see the exact same
> > handling for the non-download-to-file case in OnReadCompleted so i trust it
> > returns valid results in this case too since this step replace that step in
> the
> > d-t-f control flow.
> 
> I'm also curious about
> this.DevToolsNetLogObserver::GetAndResetEncodedDataLength() seems to return -1
> if dev_tools_net_log_observer == NULL, and instance_ in
> content/browser/devtools/devtools_netlog_observer.cc seems to be only non-NULL
> after Attach() was called (and before Detach() was called), which after
skimming
> seems to be only the case while devtools is active (?)

Right. The encoded_data_length is used for the Inspector, so it becomes valid
while devtools is active.
The encoded_data_length may become -1 and it represents that the encoded data
length is unknown.

https://chromiumcodereview.appspot.com/23696002/diff/1/webkit/child/resource_...
File webkit/child/resource_loader_bridge.h (right):

https://chromiumcodereview.appspot.com/23696002/diff/1/webkit/child/resource_...
webkit/child/resource_loader_bridge.h:155: // that case, OnReceivedData will not
be called.
On 2013/08/31 20:49:47, Nico wrote:
> Document encoded_data_length here.

Done.

[Nico, 2013-09-02 03:50:25]

https://chromiumcodereview.appspot.com/23696002/diff/23001/webkit/child/resou...
File webkit/child/resource_loader_bridge.h (right):

https://chromiumcodereview.appspot.com/23696002/diff/23001/webkit/child/resou...
webkit/child/resource_loader_bridge.h:158: // gzipped content), or -1 if if
unknown.
s/if if/if/

So this will only be valid while devtools are open? If so, and that's
intentional, mention that in the CL description which why that is sufficient.
With that, lgtm.

[yusukesuzuki, 2013-09-02 04:10:14]

https://chromiumcodereview.appspot.com/23696002/diff/23001/webkit/child/resou...
File webkit/child/resource_loader_bridge.h (right):

https://chromiumcodereview.appspot.com/23696002/diff/23001/webkit/child/resou...
webkit/child/resource_loader_bridge.h:158: // gzipped content), or -1 if if
unknown.
On 2013/09/02 03:50:25, Nico wrote:
> s/if if/if/
> 
> So this will only be valid while devtools are open? If so, and that's
> intentional, mention that in the CL description which why that is sufficient.
> With that, lgtm.

Done. I've mentioned it in the CL description and documented it in the comment.
And I've done s/if if/if/. Thanks.

[Tom Sepez, 2013-09-03 17:04:26]

Messages LGTM

[commit-bot: I haz the power, 2013-09-03 17:47:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukesuzuki@chromium.org/23696002/28001

[commit-bot: I haz the power, 2013-09-03 21:18:49]

Change committed as 221024