Add NetLog events for HttpServerPropertiesManager

Add NetLog events for HttpServerPropertiesManager

This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.

A sample NetLog:

https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsVHVmaTJacjNlQzg/view?usp=sharing

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2944043002
Cr-Commit-Position: refs/heads/master@{#481267}
Committed: https://chromium.googlesource.com/chromium/src/+/cb70779863bf9dfe419601a6de536c336feaf591

[xunjieli, 2017-06-19 17:03:03]

Description was changed from

==========
Add NetLog events for HttpServerPropertiesManager


This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.


https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsNnZ5MXlHdE8tYTA/v...

BUG=718576
==========

to

==========
Add NetLog events for HttpServerPropertiesManager


This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.


https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsNnZ5MXlHdE8tYTA/v...

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-06-19 17:22:02]

Description was changed from

==========
Add NetLog events for HttpServerPropertiesManager


This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.


https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsNnZ5MXlHdE8tYTA/v...

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add NetLog events for HttpServerPropertiesManager


This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.

A sample NetLog:

https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsVHVmaTJacjNlQzg/v...

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-06-19 17:22:51]

Description was changed from

==========
Add NetLog events for HttpServerPropertiesManager


This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.

A sample NetLog:

https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsVHVmaTJacjNlQzg/v...

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add NetLog events for HttpServerPropertiesManager

This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.

A sample NetLog:

https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsVHVmaTJacjNlQzg/v...

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-06-19 17:22:51]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org, zhongyi@chromium.org

[xunjieli, 2017-06-19 17:23:26]

Matt: PTAL at 	chrome/browser/profiles/profile_impl_io_data.cc
Cherie: PTAL at the rest

Thank you.

[xunjieli, 2017-06-19 17:23:33]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 17:23:59]

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, 2017-06-19 17:41:18]

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

[commit-bot: I haz the power, 2017-06-19 17:41:19]

Dry run: 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...)

[mmenke, 2017-06-19 17:48:51]

On 2017/06/19 17:23:26, xunjieli wrote:
> Matt: PTAL at 	chrome/browser/profiles/profile_impl_io_data.cc
> Cherie: PTAL at the rest
> 
> Thank you.

profiles/ LGTM

[xunjieli, 2017-06-19 19:00:25]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 19:00:53]

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, 2017-06-19 19:26:48]

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

[commit-bot: I haz the power, 2017-06-19 19:26: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...)

[Zhongyi Shi, 2017-06-19 19:50:25]

Mostly looks good, I am on bug triage today and slowly responding to incoming
code reviews. I'll take another look later this afternoon. Sorry about the
delay.

https://codereview.chromium.org/2944043002/diff/40001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2944043002/diff/40001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:73: return std::move(dict);
nit: return
base::MakeUnique<base::DictionaryValue>(http_server_properties_dict.Deepcopy());

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:3190:
EVENT_TYPE(HTTP_SERVER_PROPERTIES_INITIALIZATION)
nit: since these are all manager specific, could you change those event_type
names to be HTPP_SERVER_PROPERTIES_MANAGER_*

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_source_...
File net/log/net_log_source_type_list.h (right):

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_source_...
net/log/net_log_source_type_list.h:44: SOURCE_TYPE(HTTP_SERVER_PROPERTIES)
nit: SOURCE_TYPE(HTTP_SERVER_PROPERTIES_MANAGER)

[xunjieli, 2017-06-19 20:09:32]

Thanks, PTAL.

https://codereview.chromium.org/2944043002/diff/40001/net/http/http_server_pr...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2944043002/diff/40001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:73: return std::move(dict);
On 2017/06/19 19:50:24, Zhongyi Shi wrote:
> nit: return
>
base::MakeUnique<base::DictionaryValue>(http_server_properties_dict.Deepcopy());

Done.

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:3190:
EVENT_TYPE(HTTP_SERVER_PROPERTIES_INITIALIZATION)
On 2017/06/19 19:50:24, Zhongyi Shi wrote:
> nit: since these are all manager specific, could you change those event_type
> names to be HTPP_SERVER_PROPERTIES_MANAGER_*

Do you feel strongly about adding a MANAGER_ suffix?
The names are getting rather long. Some of the events are not named exactly
after the classes. E.g. We have HTTP_STREAM_JOB_CONTROLLER rather than the full
class name HTTP_STREAM_FACTORY_[IMPL_JOB]_CONTROLLER. We might in the future add
NetLog events in HttpServerProperties and group them together with these. My
preference will be to keep the names short while not introducing ambiguity.

[xunjieli, 2017-06-19 20:33:42]

xunjieli@chromium.org changed reviewers:
+ droger@chromium.org

[xunjieli, 2017-06-19 20:33:43]

+droger@: PTAL at ios/chrome/*
Thank you.

[xunjieli, 2017-06-19 20:34:03]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 20:34: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, 2017-06-19 22:21:48]

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

[commit-bot: I haz the power, 2017-06-19 22:21:49]

Dry run: This issue passed the CQ dry run.

[Zhongyi Shi, 2017-06-20 20:00:53]

Back from the QUIC merge rotation and Network triage. Sorry about the delay.

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_event_t...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2944043002/diff/40001/net/log/net_log_event_t...
net/log/net_log_event_type_list.h:3190:
EVENT_TYPE(HTTP_SERVER_PROPERTIES_INITIALIZATION)
On 2017/06/19 20:09:32, xunjieli wrote:
> On 2017/06/19 19:50:24, Zhongyi Shi wrote:
> > nit: since these are all manager specific, could you change those event_type
> > names to be HTPP_SERVER_PROPERTIES_MANAGER_*
> 
> Do you feel strongly about adding a MANAGER_ suffix?
> The names are getting rather long. Some of the events are not named exactly
> after the classes. E.g. We have HTTP_STREAM_JOB_CONTROLLER rather than the
full
> class name HTTP_STREAM_FACTORY_[IMPL_JOB]_CONTROLLER. We might in the future
add
> NetLog events in HttpServerProperties and group them together with these. My
> preference will be to keep the names short while not introducing ambiguity.

Acknowledged. Okay, sounds like you have considered this. Yeah, I was thinking
that we  might will add NetLog events for HttpServerPropertiesImpl, and we might
want to differentiate or aggregate the NetLogs from impl and manager.

https://codereview.chromium.org/2944043002/diff/120001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2944043002/diff/120001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:1070: base::Bind(&NetLogCallback,
http_server_properties_dict));
I am not sure whether it would be a good idea to log all the entries from
HttpServerProperties dict in NetLogs. Additionally, this is logged every time a
update is scheduled, regardless of diff size.

[xunjieli, 2017-06-20 20:19:08]

https://codereview.chromium.org/2944043002/diff/120001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2944043002/diff/120001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:1070: base::Bind(&NetLogCallback,
http_server_properties_dict));
On 2017/06/20 20:00:53, Zhongyi Shi wrote:
> I am not sure whether it would be a good idea to log all the entries from
> HttpServerProperties dict in NetLogs. Additionally, this is logged every time
a
> update is scheduled, regardless of diff size. 

Are you concerned about the performance cost? I am not sure if that's an issue.
The entire dictionary is serialized and persisted every 60s regardless. It's
more work if we want to figure out the exact property that has changed.

[Zhongyi Shi, 2017-06-20 21:38:48]

lgtm

https://codereview.chromium.org/2944043002/diff/120001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2944043002/diff/120001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:1070: base::Bind(&NetLogCallback,
http_server_properties_dict));
On 2017/06/20 20:19:08, xunjieli wrote:
> On 2017/06/20 20:00:53, Zhongyi Shi wrote:
> > I am not sure whether it would be a good idea to log all the entries from
> > HttpServerProperties dict in NetLogs. Additionally, this is logged every
time
> a
> > update is scheduled, regardless of diff size. 
> 
> Are you concerned about the performance cost? I am not sure if that's an
issue.
> The entire dictionary is serialized and persisted every 60s regardless. It's
> more work if we want to figure out the exact property that has changed.

Acknowledged. Discussed offline, since the server properties is only logged when
update finishes, which is every 60s. Agreed that it's still reasonable.

[xunjieli, 2017-06-20 21:56:01]

Thank you for the review.

droger@: PTAL.

[xunjieli, 2017-06-20 21:56:10]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 21:56:43]

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, 2017-06-20 23:33:39]

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

[commit-bot: I haz the power, 2017-06-20 23:33:40]

Dry run: This issue passed the CQ dry run.

[droger, 2017-06-21 16:28:55]

Why are you passing null on iOS? See suggestion below.

https://codereview.chromium.org/2944043002/diff/140001/ios/chrome/browser/bro...
File ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm
(right):

https://codereview.chromium.org/2944043002/diff/140001/ios/chrome/browser/bro...
ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm:162:
/*net_log=*/nullptr);
GetApplicationContext()->GetIOSChromeIOThread()->net_log();

[xunjieli, 2017-06-21 16:38:28]

Thank you. PTAL.

https://codereview.chromium.org/2944043002/diff/140001/ios/chrome/browser/bro...
File ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm
(right):

https://codereview.chromium.org/2944043002/diff/140001/ios/chrome/browser/bro...
ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm:162:
/*net_log=*/nullptr);
On 2017/06/21 16:28:55, droger wrote:
> GetApplicationContext()->GetIOSChromeIOThread()->net_log();

Done.

[droger, 2017-06-21 16:41:26]

lgtm

[xunjieli, 2017-06-21 16:50:25]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 16:50:35]

Dry run: CQ is trying da patch.

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

[xunjieli, 2017-06-21 17:10:58]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-06-21 17:11:01]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-06-21 17:11:02]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
zhongyi@chromium.org
Link to the patchset: https://codereview.chromium.org/2944043002/#ps160001
(title: "address droger comment")

[commit-bot: I haz the power, 2017-06-21 17:11:23]

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, 2017-06-21 19:12:10]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498065061713870,
"parent_rev": "2fa54e0672ef0dd85ef3aa2878cdb80db245a006", "commit_rev":
"cb70779863bf9dfe419601a6de536c336feaf591"}

[commit-bot: I haz the power, 2017-06-21 19:12:23]

Description was changed from

==========
Add NetLog events for HttpServerPropertiesManager

This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.

A sample NetLog:

https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsVHVmaTJacjNlQzg/v...

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add NetLog events for HttpServerPropertiesManager

This CL adds NetLog events for HttpServerPropertiesManager.
(1) HTTP_SERVER_PROPERTIES_INITIALIZATION
This is emitted when initialization starts and finishes. This is useful to know
if server information is used in making network requests.
(2) HTTP_SERVER_PROPERTIES_UPDATE_CACHE
This is emitted when we are populating the in-memory HttpServerProperties from
the on-disk version.
(2) HTTP_SERVER_PROPERTIES_UPDATE_PREFS
This is emitted when we are persisting the in-memory HttpServerProperties to
disk.

A sample NetLog:

https://drive.google.com/a/chromium.org/file/d/0Bw0C8-TgzJfsVHVmaTJacjNlQzg/v...

BUG=718576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2944043002
Cr-Commit-Position: refs/heads/master@{#481267}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb70779863bf9dfe419601a6de53...
==========

[commit-bot: I haz the power, 2017-06-21 19:12:25]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/cb70779863bf9dfe419601a6de53...