Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1977543002:
[Cronet] Add network change events to Cronet's NetLog (Closed)
|
Created:
4 years, 7 months ago by pauljensen Modified:
4 years, 7 months ago Reviewers:
xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Add network change events to Cronet's NetLog
These NetLog events are quite useful for debugging.
BUG=610293
R=xunjieli
Committed: https://crrev.com/39a25a0c1fa4a6fd24ed3898e3231caa298a2e33
Cr-Commit-Position: refs/heads/master@{#395231}
Patch Set 1 #Patch Set 2 : #
Total comments: 9
Patch Set 3 : add comment #
Dependent Patchsets: Messages
Total messages: 15 (3 generated)
Helen, PTAL.
https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:72: net::NetLog net_log_; Can we make these into std::unique_ptr so we know that they are owned by this class? https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:73: net::LoggingNetworkChangeObserver net_change_logger_; Can we add a comment on why this is global?
https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:72: net::NetLog net_log_; On 2016/05/13 13:32:00, xunjieli wrote: > Can we make these into std::unique_ptr so we know that they are owned by this > class? Isn't having a definition of a variable even stronger ownership than a unique_ptr? A unique_ptr is an unnecessary layer of abstraction in this case I believe, for the same reason we write "int i" rather than "std::unique_ptr<Integer> i(new Integer)" https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:73: net::LoggingNetworkChangeObserver net_change_logger_; On 2016/05/13 13:32:00, xunjieli wrote: > Can we add a comment on why this is global? I don't think it is global, it's a member of a class type. g_net_log is global.
https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:72: net::NetLog net_log_; On 2016/05/13 16:16:18, pauljensen wrote: > On 2016/05/13 13:32:00, xunjieli wrote: > > Can we make these into std::unique_ptr so we know that they are owned by this > > class? > > Isn't having a definition of a variable even stronger ownership than a > unique_ptr? A unique_ptr is an unnecessary layer of abstraction in this case I > believe, for the same reason we write "int i" rather than > "std::unique_ptr<Integer> i(new Integer)" Acknowledged. Matt says if there isn't a use of unique_ptr, don't use one. I thought smart pointers are always superior :\ https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:73: net::LoggingNetworkChangeObserver net_change_logger_; On 2016/05/13 16:16:18, pauljensen wrote: > On 2016/05/13 13:32:00, xunjieli wrote: > > Can we add a comment on why this is global? > > I don't think it is global, it's a member of a class type. g_net_log is global. Yea. Semantically, it isn't a global. But it is essentially a global, since we have a global NetLog. Could you comment on why we have it here instead of making a new one for every URLRequestContextAdapter?
lgtm
https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:73: net::LoggingNetworkChangeObserver net_change_logger_; On 2016/05/13 18:09:23, xunjieli wrote: > On 2016/05/13 16:16:18, pauljensen wrote: > > On 2016/05/13 13:32:00, xunjieli wrote: > > > Can we add a comment on why this is global? > > > > I don't think it is global, it's a member of a class type. g_net_log is > global. > > Yea. Semantically, it isn't a global. But it is essentially a global, since we > have a global NetLog. Could you comment on why we have it here instead of making > a new one for every URLRequestContextAdapter? Well we have one LoggingNetworkChangeObserver for each NetLog because you only want the events logged once. Is that what you want me to make the comment say? g_net_log already has a comment explaining why we use a global one.
https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:73: net::LoggingNetworkChangeObserver net_change_logger_; On 2016/05/13 18:09:23, xunjieli wrote: > On 2016/05/13 16:16:18, pauljensen wrote: > > On 2016/05/13 13:32:00, xunjieli wrote: > > > Can we add a comment on why this is global? > > > > I don't think it is global, it's a member of a class type. g_net_log is > global. > > Yea. Semantically, it isn't a global. But it is essentially a global, since we > have a global NetLog. Could you comment on why we have it here instead of making > a new one for every URLRequestContextAdapter? Well we have one LoggingNetworkChangeObserver for each NetLog because you only want the events logged once. Is that what you want me to make the comment say? g_net_log already has a comment explaining why we use a global one.
https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:73: net::LoggingNetworkChangeObserver net_change_logger_; On 2016/05/13 18:13:56, pauljensen wrote: > On 2016/05/13 18:09:23, xunjieli wrote: > > On 2016/05/13 16:16:18, pauljensen wrote: > > > On 2016/05/13 13:32:00, xunjieli wrote: > > > > Can we add a comment on why this is global? > > > > > > I don't think it is global, it's a member of a class type. g_net_log is > > global. > > > > Yea. Semantically, it isn't a global. But it is essentially a global, since we > > have a global NetLog. Could you comment on why we have it here instead of > making > > a new one for every URLRequestContextAdapter? > > Well we have one LoggingNetworkChangeObserver for each NetLog because you only > want the events logged once. Is that what you want me to make the comment say? > g_net_log already has a comment explaining why we use a global one. Yes. It isn't obvious why LoggingNetworkChangeObserver needs to be boundled with the global object. Of course, if I do a codesearch and know what LoggingNetworkChangeObserver does, then sure. On the other hand, we do not bundle WriteToFileNetLogObserver. For someone who isn't familiar with these two classes, from the first glance, it looks like they are both observers and are related to logging, but they have different life cycles. This can be lead to confusion down the road. A quick comment along the line that you just mentioned will do.
https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1977543002/diff/20001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:73: net::LoggingNetworkChangeObserver net_change_logger_; On 2016/05/13 18:19:07, xunjieli wrote: > On 2016/05/13 18:13:56, pauljensen wrote: > > On 2016/05/13 18:09:23, xunjieli wrote: > > > On 2016/05/13 16:16:18, pauljensen wrote: > > > > On 2016/05/13 13:32:00, xunjieli wrote: > > > > > Can we add a comment on why this is global? > > > > > > > > I don't think it is global, it's a member of a class type. g_net_log is > > > global. > > > > > > Yea. Semantically, it isn't a global. But it is essentially a global, since > we > > > have a global NetLog. Could you comment on why we have it here instead of > > making > > > a new one for every URLRequestContextAdapter? > > > > Well we have one LoggingNetworkChangeObserver for each NetLog because you only > > want the events logged once. Is that what you want me to make the comment > say? > > g_net_log already has a comment explaining why we use a global one. > > Yes. It isn't obvious why LoggingNetworkChangeObserver needs to be boundled with > the global object. Of course, if I do a codesearch and know what > LoggingNetworkChangeObserver does, then sure. On the other hand, we do not > bundle WriteToFileNetLogObserver. For someone who isn't familiar with these two > classes, from the first glance, it looks like they are both observers and are > related to logging, but they have different life cycles. This can be lead to > confusion down the road. A quick comment along the line that you just mentioned > will do. Done.
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1977543002/#ps40001 (title: "add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977543002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977543002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Add network change events to Cronet's NetLog These NetLog events are quite useful for debugging. BUG=610293 R=xunjieli ========== to ========== [Cronet] Add network change events to Cronet's NetLog These NetLog events are quite useful for debugging. BUG=610293 R=xunjieli Committed: https://crrev.com/39a25a0c1fa4a6fd24ed3898e3231caa298a2e33 Cr-Commit-Position: refs/heads/master@{#395231} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/39a25a0c1fa4a6fd24ed3898e3231caa298a2e33 Cr-Commit-Position: refs/heads/master@{#395231} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
