HttpServerProperties - Track UpdatePrefs callers. In offical builds, on

HttpServerProperties - Track UpdatePrefs callers. In offical builds, on
Mac we had noticed UpdatePrefs is being called a lot. Would like to
track the caller (we think it could be extensions).

BUG=485314, 451256, 431476
R=mmenke@chromium.org, asvitkine@chromium.org

Committed: https://crrev.com/848d07dfae3bd43c0573bceb311cdb92b4da53d9
Cr-Commit-Position: refs/heads/master@{#329452}

[ramant (doing other things), 2015-05-12 00:15:48]

rtenneti@chromium.org changed reviewers:
+ asvitkine@chromium.org

[ramant (doing other things), 2015-05-12 00:19:44]

On my linux box, I have been running the browser for more than an hour and
Net.HttpServerProperties.UpdatePrefs was called 72 times. Would like to collect
this data for the the official build on Mac and see which save/update properties
are being a called during idle time.

The following is the histogram. If it makes sense, we could leave the histogram
for tracking purposes.

Net.HttpServerProperties.UpdatePrefs recorded 72 samples

[mmenke, 2015-05-12 14:35:03]

https://codereview.chromium.org/1139843002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/1139843002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:357:
RecordUpdatePrefs(SET_SERVER_NETWORK_STATS);
Suggest making this an argument to ScheduleUpdatePrefsOnNetworkThread instead -
makes sure you catch all calls, and new callers add a histogram entry.

[ramant (doing other things), 2015-05-12 17:32:22]

https://codereview.chromium.org/1139843002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/1139843002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:357:
RecordUpdatePrefs(SET_SERVER_NETWORK_STATS);
On 2015/05/12 14:35:03, mmenke wrote:
> Suggest making this an argument to ScheduleUpdatePrefsOnNetworkThread instead
-
> makes sure you catch all calls, and new callers add a histogram entry.

Doh!.

Thanks and Done.

[mmenke, 2015-05-12 17:38:52]

LGTM

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

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:70: void
RecordUpdatePrefs(HttpServerPropertiesManager::Location location) {
optional:  Could inline this now, with the comment.

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.h:55: enum Location {
Suggest putting these in protected section and then exposing them in the test
subclass, like you do for ScheduleUpdatePrefsOnNetworkThread.  Makes it clearer
what the public API is.

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.h:69: UNIT_TESTS = 13,
Don't think this is really needed - can just use a random one.

[Alexei Svitkine (slow), 2015-05-12 17:41:17]

lgtm

[ramant (doing other things), 2015-05-12 17:54:33]

Patchset #3 (id:40001) has been deleted

[ramant (doing other things), 2015-05-12 17:56:25]

Patchset #3 (id:50005) has been deleted

[ramant (doing other things), 2015-05-12 17:57:17]

Thanks mmenke an Alexei. Made all the changes.

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

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.cc:70: void
RecordUpdatePrefs(HttpServerPropertiesManager::Location location) {
On 2015/05/12 17:38:52, mmenke wrote:
> optional:  Could inline this now, with the comment.

Done.

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.h:55: enum Location {
On 2015/05/12 17:38:52, mmenke wrote:
> Suggest putting these in protected section and then exposing them in the test
> subclass, like you do for ScheduleUpdatePrefsOnNetworkThread.  Makes it
clearer
> what the public API is.

Done.

https://codereview.chromium.org/1139843002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_manager.h:69: UNIT_TESTS = 13,
On 2015/05/12 17:38:52, mmenke wrote:
> Don't think this is really needed - can just use a random one.

Done.

[ramant (doing other things), 2015-05-12 17:57:33]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2015-05-12 17:57:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1139843002/#ps70001
(title: "Fixed comments for patch set 2")

[commit-bot: I haz the power, 2015-05-12 17:58:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139843002/70001

[commit-bot: I haz the power, 2015-05-12 19:23:19]

Committed patchset #3 (id:70001)

[commit-bot: I haz the power, 2015-05-12 19:24:43]

Patchset 3 (id:??) landed as
https://crrev.com/848d07dfae3bd43c0573bceb311cdb92b4da53d9
Cr-Commit-Position: refs/heads/master@{#329452}