Add net log to UploadDataStream.

Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload
to be able to discover where the hang is occurring in
situations like in the issue 611436.

BUG=613337
Committed: https://crrev.com/819ba85927138d71e488fb7a006d10e7abeec01b
Cr-Commit-Position: refs/heads/master@{#412474}

[maksims (do not use this acc), 2016-08-08 12:16:20]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 12:16:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-08-08 12:19:23]

Description was changed from

==========
netlog

BUG=
==========

to

==========
Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload to be able to
discover where the hang is occurring in situations like in the issue 611436.

BUG=613337
==========

[maksims (do not use this acc), 2016-08-08 12:19:23]

maksim.sisov@intel.com changed reviewers:
+ jam@chromium.org, mmenke@chromium.org

[maksims (do not use this acc), 2016-08-08 12:20:40]

Description was changed from

==========
Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload to be able to
discover where the hang is occurring in situations like in the issue 611436.

BUG=613337
==========

to

==========
Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload 
to be able to discover where the hang is occurring in 
situations like in the issue 611436.

BUG=613337
==========

[commit-bot: I haz the power, 2016-08-08 12:26:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 12:26:36]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[maksims (do not use this acc), 2016-08-09 07:17:53]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 07:18:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 07:33:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 07:33:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maksims (do not use this acc), 2016-08-09 07:53:57]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 07:54:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 08:08:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 08:08:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[maksims (do not use this acc), 2016-08-09 10:35:59]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 10:36:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 11:02:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 11:02:31]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[maksims (do not use this acc), 2016-08-09 11:45:38]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 11:45:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-08-09 11:56:55]

Patchset #2 (id:20001) has been deleted

[maksims (do not use this acc), 2016-08-09 11:57:04]

Patchset #2 (id:40001) has been deleted

[maksims (do not use this acc), 2016-08-09 11:57:14]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-08-09 13:05:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 13:05:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[maksims (do not use this acc), 2016-08-10 05:29:02]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 05:29:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 07:38:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 07:38:42]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-08-10 07:42:32]

Patchset #1 (id:1) has been deleted

[maksims (do not use this acc), 2016-08-10 07:43:48]

please review

[jam, 2016-08-10 17:30:30]

rs lgtm for code outside of net/

[mmenke, 2016-08-10 20:44:50]

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:26: dict->SetBoolean("is_chunked:", is_chunked);
Colons aren't needed on any of these.  If you want, you can see what they look
like by opening up about:net-internals#events, downloading something, and
looking for the URL_REQUEST for that entry (Just search for the URL of the
download in filters, or using ctrl-s).

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:43: dict->SetBoolean("is_chunked:", is_chunked);
Remove colons.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:70: net_log.EndEvent(event_type,
base::Bind(&NetLogStartreadingInfoCallback,
The way End event works is there should be a matching Begin event with the same
event type before it.  i.e., we've have a NetLog::TYPE_UPLOAD_DATA_STREAM_READ,
and have a BEGIN_EVENT on start reading, and an END_EVENT on stop reading.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:75: }
nit:  }  // namespace

(Also should have a blank line before it, to match the blank line after the
start of the namespace)

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:95: net_log_ = net_log;
Seems like we should have a begin event here, with no parameters.  That way, if
init hangs, we can see it.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:116: current_position_, total_size_, is_eof_,
is_chunked_);
total_size_ is constant, so should just log that when init completes, same with
is_chunked_.

I suggest just logging current_position_ here, and result in the end event. 
Don't think all the extra fields on every event get us enough to be worth the
increase in log size (Makes logs harder to read, and bigger in memory and on
disk)

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:119: return 0;
This case is missing an end event.  To make sure we always have the event,
suggest getting rid of the early return.  if is_eof_ is true, just set result to
0 and don't call read internal, but *do* call OnReadCompleted.

That makes the logging consistent.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:137: void UploadDataStream::Reset() {
If callback isn't NULL, we should log a read end event, as this also cancels any
pending read.  Maybe log it with the error ERR_ABORTED?

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:180: LogInit(net_log_, result, is_eof_,
is_chunked_);
I don't think we really need is_eof_ here.  total_size_, on the other hand,
would be useful.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
File net/base/upload_data_stream.h (right):

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.h:111: virtual int InitInternal(const BoundNetLog&
net_log) = 0;
Is a BoundNetLog needed here?  Doesn't look like any of the subclasses use this.

https://codereview.chromium.org/2227503003/diff/100001/net/log/net_log_event_...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2227503003/diff/100001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3028: //   "is_chunked_": <Shows whether data
is chunked or not>.
Remove ending underscore (And from the corresponding cc file, if they're there)

[maksims (do not use this acc), 2016-08-16 10:41:50]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 10:41:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 10:50:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 10:50:49]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[maksims (do not use this acc), 2016-08-16 11:14:07]

Patchset #3 (id:120001) has been deleted

[maksims (do not use this acc), 2016-08-16 11:15:59]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 11:16:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-08-16 12:00:19]

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:26: dict->SetBoolean("is_chunked:", is_chunked);
On 2016/08/10 20:44:49, mmenke wrote:
> Colons aren't needed on any of these.  If you want, you can see what they look
> like by opening up about:net-internals#events, downloading something, and
> looking for the URL_REQUEST for that entry (Just search for the URL of the
> download in filters, or using ctrl-s).

I have already checked that before but did not pay attention to the colons.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:43: dict->SetBoolean("is_chunked:", is_chunked);
On 2016/08/10 20:44:49, mmenke wrote:
> Remove colons.

Done.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:70: net_log.EndEvent(event_type,
base::Bind(&NetLogStartreadingInfoCallback,
On 2016/08/10 20:44:49, mmenke wrote:
> The way End event works is there should be a matching Begin event with the
same
> event type before it.  i.e., we've have a
NetLog::TYPE_UPLOAD_DATA_STREAM_READ,
> and have a BEGIN_EVENT on start reading, and an END_EVENT on stop reading.

Done.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:75: }
On 2016/08/10 20:44:50, mmenke wrote:
> nit:  }  // namespace
> 
> (Also should have a blank line before it, to match the blank line after the
> start of the namespace)

Done.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:95: net_log_ = net_log;
On 2016/08/10 20:44:50, mmenke wrote:
> Seems like we should have a begin event here, with no parameters.  That way,
if
> init hangs, we can see it.

Done.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:116: current_position_, total_size_, is_eof_,
is_chunked_);
On 2016/08/10 20:44:49, mmenke wrote:
> total_size_ is constant, so should just log that when init completes, same
with
> is_chunked_.
> 
> I suggest just logging current_position_ here, and result in the end event. 
> Don't think all the extra fields on every event get us enough to be worth the
> increase in log size (Makes logs harder to read, and bigger in memory and on
> disk)

Done.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:119: return 0;
On 2016/08/10 20:44:49, mmenke wrote:
> This case is missing an end event.  To make sure we always have the event,
> suggest getting rid of the early return.  if is_eof_ is true, just set result
to
> 0 and don't call read internal, but *do* call OnReadCompleted.
> 
> That makes the logging consistent.

Done.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:137: void UploadDataStream::Reset() {
On 2016/08/10 20:44:49, mmenke wrote:
> If callback isn't NULL, we should log a read end event, as this also cancels
any
> pending read.  Maybe log it with the error ERR_ABORTED?

Reset() is called on Init(). It will add more confusion.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.cc:180: LogInit(net_log_, result, is_eof_,
is_chunked_);
On 2016/08/10 20:44:49, mmenke wrote:
> I don't think we really need is_eof_ here.  total_size_, on the other hand,
> would be useful.

Done.

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
File net/base/upload_data_stream.h (right):

https://codereview.chromium.org/2227503003/diff/100001/net/base/upload_data_s...
net/base/upload_data_stream.h:111: virtual int InitInternal(const BoundNetLog&
net_log) = 0;
On 2016/08/10 20:44:50, mmenke wrote:
> Is a BoundNetLog needed here?  Doesn't look like any of the subclasses use
this.

DevToolsNetworkUploadDataStream::InitInternal calls upload_data_stream->Init().

[commit-bot: I haz the power, 2016-08-16 13:06:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 13:06:08]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-08-16 15:40:06]

Looking good!

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:49: void LogInitEnd(const BoundNetLog& net_log,
I don't think these methods really get us anything.  Can just inline them
instead.

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:73: NetLog::TYPE_CANCELLED);
TYPE_CANCELLED isn't a network error code, it's a NetLog event.

Can just get rid of this method, and use LogReadEnd(net_log, ERR_ABORTED)
instead.

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:98:
net_log_.BeginEvent(NetLog::TYPE_UPLOAD_DATA_STREAM_READ);
TYPE_UPLOAD_DATA_STREAM_INIT

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:142: LogCancelRead(net_log_);
We may actually be completing initialization.  Can just check if
initialized_successfully_ is true or not (If it's true, we were reading, if
false, we were initializing).  The fact that we'd have to worry about both cases
didn't occur to me earlier.

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3024: // Contains the following infromation:
"The END phase contains the following parameters:"

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3044: // Contains the following information:
"The BEGIN phase contains the following parameters:"

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3051: //   "is_chunked": <Shows whether data
is chunked or not>.
only current_position is still here.

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3052: // }
document end phase paremter, which just has result.

[maksims (do not use this acc), 2016-08-16 18:40:39]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 18:41:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-08-16 18:45:38]

it should be better now

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:49: void LogInitEnd(const BoundNetLog& net_log,
On 2016/08/16 15:40:06, mmenke wrote:
> I don't think these methods really get us anything.  Can just inline them
> instead.

Done.

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:73: NetLog::TYPE_CANCELLED);
On 2016/08/16 15:40:06, mmenke wrote:
> TYPE_CANCELLED isn't a network error code, it's a NetLog event.
> 
> Can just get rid of this method, and use LogReadEnd(net_log, ERR_ABORTED)
> instead.

Done.

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:98:
net_log_.BeginEvent(NetLog::TYPE_UPLOAD_DATA_STREAM_READ);
On 2016/08/16 15:40:06, mmenke wrote:
> TYPE_UPLOAD_DATA_STREAM_INIT

oops.

https://codereview.chromium.org/2227503003/diff/140001/net/base/upload_data_s...
net/base/upload_data_stream.cc:142: LogCancelRead(net_log_);
On 2016/08/16 15:40:06, mmenke wrote:
> We may actually be completing initialization.  Can just check if
> initialized_successfully_ is true or not (If it's true, we were reading, if
> false, we were initializing).  The fact that we'd have to worry about both
cases
> didn't occur to me earlier.

I though a bit different way. If callback is set, then anyway init() is
"successful"..almost.

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3024: // Contains the following infromation:
On 2016/08/16 15:40:06, mmenke wrote:
> "The END phase contains the following parameters:"

Done.

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3044: // Contains the following information:
On 2016/08/16 15:40:06, mmenke wrote:
> "The BEGIN phase contains the following parameters:"

Done.

https://codereview.chromium.org/2227503003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3052: // }
On 2016/08/16 15:40:06, mmenke wrote:
> document end phase paremter, which just has result.

Done.

[commit-bot: I haz the power, 2016-08-16 18:51:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 18:51:09]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mmenke, 2016-08-16 18:54:13]

LGTM, modulo comments.

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:39: std::unique_ptr<base::Value>
NetLogReadEndInfoCallback(
Can actually get rid of this, and use "EndEventWithNetErrorCode" instead.

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:115: base::Bind(&NetLogReadEndInfoCallback,
ERR_ABORTED));
Sorry, should have been clearer.  My suggestion was in addition to the old
logic, so:

// If there's a pending callback, there's a pending init or read call that is
being
// canceled.
if (!callback_.is_null()) {
  if (!initialized_successfully_) {
    // If initialization has not yet succeeded, this call is aborting
initialization.
    net_log.EndEventWithNetErrorCode(NetLog::TYPE_UPLOAD_DATA_STREAM_INIT,
ERR_ABORTED);
  } else {
    // Otherwise, a read is being aborted.
    net_log.EndEventWithNetErrorCode(NetLog::TYPE_UPLOAD_DATA_STREAM_READ,
ERR_ABORTED);
  }
}

[maksims (do not use this acc), 2016-08-16 19:21:37]

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:39: std::unique_ptr<base::Value>
NetLogReadEndInfoCallback(
On 2016/08/16 18:54:13, mmenke wrote:
> Can actually get rid of this, and use "EndEventWithNetErrorCode" instead.

but wouldn't it mean that there will be no "result" key in the log? Just an
integer.

[mmenke, 2016-08-16 19:40:09]

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:24: dict->SetInteger("result", result);
Per below comment, call this net_error to get it formatter correctly in
net-internals, and update the event_list.

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:39: std::unique_ptr<base::Value>
NetLogReadEndInfoCallback(
On 2016/08/16 19:21:37, maksims wrote:
> On 2016/08/16 18:54:13, mmenke wrote:
> > Can actually get rid of this, and use "EndEventWithNetErrorCode" instead.
> 
> but wouldn't it mean that there will be no "result" key in the log? Just an
> integer.

EndEventWithNetErrorCode takes a network error code as a second parameter (And
calls it net_error - so should update the event descriptions, actually).  It's
both a convenience method that does this, and makes sure we use the correct
name.

[maksims (do not use this acc), 2016-08-17 05:11:49]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-08-17 05:11:50]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2227503003/#ps180001
(title: "minor fixes")

[maksims (do not use this acc), 2016-08-17 05:11:57]

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
File net/base/upload_data_stream.cc (right):

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:24: dict->SetInteger("result", result);
On 2016/08/16 19:40:09, mmenke wrote:
> Per below comment, call this net_error to get it formatter correctly in
> net-internals, and update the event_list.

Done.

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:39: std::unique_ptr<base::Value>
NetLogReadEndInfoCallback(
On 2016/08/16 19:40:09, mmenke wrote:
> On 2016/08/16 19:21:37, maksims wrote:
> > On 2016/08/16 18:54:13, mmenke wrote:
> > > Can actually get rid of this, and use "EndEventWithNetErrorCode" instead.
> > 
> > but wouldn't it mean that there will be no "result" key in the log? Just an
> > integer.
> 
> EndEventWithNetErrorCode takes a network error code as a second parameter (And
> calls it net_error - so should update the event descriptions, actually).  It's
> both a convenience method that does this, and makes sure we use the correct
> name.

Done.

https://codereview.chromium.org/2227503003/diff/160001/net/base/upload_data_s...
net/base/upload_data_stream.cc:115: base::Bind(&NetLogReadEndInfoCallback,
ERR_ABORTED));
On 2016/08/16 18:54:13, mmenke wrote:
> Sorry, should have been clearer.  My suggestion was in addition to the old
> logic, so:
> 
> // If there's a pending callback, there's a pending init or read call that is
> being
> // canceled.
> if (!callback_.is_null()) {
>   if (!initialized_successfully_) {
>     // If initialization has not yet succeeded, this call is aborting
> initialization.
>     net_log.EndEventWithNetErrorCode(NetLog::TYPE_UPLOAD_DATA_STREAM_INIT,
> ERR_ABORTED);
>   } else {
>     // Otherwise, a read is being aborted.
>     net_log.EndEventWithNetErrorCode(NetLog::TYPE_UPLOAD_DATA_STREAM_READ,
> ERR_ABORTED);
>   }
> }

Oh, silly me. I've left your comments to make this part of the code easily
understandable.

[commit-bot: I haz the power, 2016-08-17 05:12:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 05:21:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 05:21:21]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[maksims (do not use this acc), 2016-08-17 06:13:13]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 06:13:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 06:24:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 06:24:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maksims (do not use this acc), 2016-08-17 07:08:22]

Patchset #6 (id:200001) has been deleted

[maksims (do not use this acc), 2016-08-17 07:08:41]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 07:08:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 08:08:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 08:08:48]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-08-17 08:18:55]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-08-17 08:18:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2227503003/#ps220001
(title: "typo")

[commit-bot: I haz the power, 2016-08-17 08:19:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 08:23:10]

Description was changed from

==========
Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload 
to be able to discover where the hang is occurring in 
situations like in the issue 611436.

BUG=613337
==========

to

==========
Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload 
to be able to discover where the hang is occurring in 
situations like in the issue 611436.

BUG=613337
==========

[commit-bot: I haz the power, 2016-08-17 08:23:12]

Committed patchset #6 (id:220001)

[commit-bot: I haz the power, 2016-08-17 08:25:04]

Description was changed from

==========
Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload 
to be able to discover where the hang is occurring in 
situations like in the issue 611436.

BUG=613337
==========

to

==========
Add net log to UploadDataStream.

Improve the granularity of net-internals logging for upload
to be able to discover where the hang is occurring in
situations like in the issue 611436.

BUG=613337

Committed: https://crrev.com/819ba85927138d71e488fb7a006d10e7abeec01b
Cr-Commit-Position: refs/heads/master@{#412474}
==========

[commit-bot: I haz the power, 2016-08-17 08:25:06]

Patchset 6 (id:??) landed as
https://crrev.com/819ba85927138d71e488fb7a006d10e7abeec01b
Cr-Commit-Position: refs/heads/master@{#412474}

[vinodsonkusare77_gmail.com, 2016-08-17 10:06:59]

https://youtu.be/yD6KioQhnAc?list=UUWQUZfAdokV83KKjLl-sDeQ

On Wed, Aug 17, 2016 at 1:55 PM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Patchset 6 (id:??) landed as
> https://crrev.com/819ba85927138d71e488fb7a006d10e7abeec01b
> Cr-Commit-Position: refs/heads/master@{#412474}
>
> https://codereview.chromium.org/2227503003/
>
> --
> You received this message because you are subscribed to the Google Groups
> "DevTools Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to devtools-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vinodsonkusare77_gmail.com, 2016-08-17 10:07:35]

https://youtu.be/ZB29OFBS2w8?list=UUWQUZfAdokV83KKjLl-sDeQ

On Wed, Aug 17, 2016 at 3:36 PM, Vinod kumar Sonkusare <
vinodsonkusare77@gmail.com> wrote:

> https://youtu.be/yD6KioQhnAc?list=UUWQUZfAdokV83KKjLl-sDeQ
>
> On Wed, Aug 17, 2016 at 1:55 PM, commit-bot@chromium.org via
> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
> wrote:
>
>> Patchset 6 (id:??) landed as
>> https://crrev.com/819ba85927138d71e488fb7a006d10e7abeec01b
>> Cr-Commit-Position: refs/heads/master@{#412474}
>>
>> https://codereview.chromium.org/2227503003/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "DevTools Reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to devtools-reviews+unsubscribe@chromium.org.
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.