Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857093002:
Expose changes in the network quality to the renderers (Closed)
|
Created:
3 years, 7 months ago by tbansal1 Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, net-reviews_chromium.org, Aaron Boodman, darin-cc_chromium.org, darin (slow to review), abarth-chromium, tbansal+watch-nqe_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose changes in the network quality to the renderers
A network quality observer owned by IOThread has been added. It listens
to the network quality changes and notifies renderers on the UI thread
using mojo. This is similar to how content::BrowserOnlineStateObserver
listens to the connection type changes.
BUG=719511
Review-Url: https://codereview.chromium.org/2857093002
Cr-Commit-Position: refs/heads/master@{#470654}
Committed: https://chromium.googlesource.com/chromium/src/+/15973c3f1ac558b3b3eed70a1a7a3ed256528a65
Patch Set 1 : ps #
Total comments: 38
Patch Set 2 : ryansturm, nasko comments #
Total comments: 6
Patch Set 3 : nasko comments #
Total comments: 10
Patch Set 4 : isherman comments #Patch Set 5 : isherman comments #Messages
Total messages: 65 (48 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Expose NQE to renderers BUG= ========== to ========== Expose changes in the network quality to the renderers BUG= ==========
Patchset #1 (id:60001) has been deleted
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. Thanks.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Description was changed from ========== Expose changes in the network quality to the renderers BUG= ========== to ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread. BUG= ==========
Description was changed from ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread. BUG= ========== to ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. BUG= ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. BUG= ========== to ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. BUG=719108 ==========
Description was changed from ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. BUG=719108 ========== to ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. This is similar to how content::BrowserOnlineStateObserver listens to the connection type changes. BUG=719108 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:140001) has been deleted
tbansal@chromium.org changed reviewers: + nasko@chromium.org
nasko: ptal at content/. Thanks.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Description was changed from ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. This is similar to how content::BrowserOnlineStateObserver listens to the connection type changes. BUG=719108 ========== to ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. This is similar to how content::BrowserOnlineStateObserver listens to the connection type changes. BUG=719511 ==========
On 2017/05/06 00:18:03, tbansal1 wrote: > nasko: ptal at content/. Thanks. Still looking at this for nits. The approach seems pretty parallel to browser_online_state_observer.cc, so I am fine with it at a broad level. I wonder if the observer could instead be in chrome/ and post a profile-based update message like: UpdateNetworkQualityForContext(BrowserContext* browser_context, ...). I believe this helps with the content/ boundary as it has to know less about meaningful changes in network quality. Further, I think if we use the UINetworkQualityObserver, we will automatically not send updates to incognito tabs (Should incognito tabs publish network quality? Probably not?). The current estimate would have to be stored on render frames and a callback to get the current estimate might need to be exposed (or BrowserContext could store the estimate) so that new frames could get the estimate. Like I said, the current approach works for me, but it doesn't seem to consider incognito, and it adds more to content than it might need to. What do you guys think?
On 2017/05/08 16:40:02, Ryan Sturm wrote: > On 2017/05/06 00:18:03, tbansal1 wrote: > > nasko: ptal at content/. Thanks. > > Still looking at this for nits. The approach seems pretty parallel to > browser_online_state_observer.cc, so I am fine with it at a broad level. > I > wonder if the observer could instead be in chrome/ and post a profile-based > update message like: UpdateNetworkQualityForContext(BrowserContext* > browser_context, ...). I believe this helps with the content/ boundary as it has > to know less about meaningful changes in network quality. This would not work if we want to expose the network quality to the renderers in off the record profiles. Also, this path is consistent to the one taken by BrowserOnlineStateObserver. > Further, I think if we > use the UINetworkQualityObserver, we will automatically not send updates to > incognito tabs (Should incognito tabs publish network quality? Probably not?). > The current estimate would have to be stored on render frames and a callback to > get the current estimate might need to be exposed (or BrowserContext could store > the estimate) so that new frames could get the estimate.> > Like I said, the current approach works for me, but it doesn't seem to consider > incognito, and it adds more to content than it might need to. What do you guys > think? I think the network quality should be exposed to renderers in off the record profiles too. e.g., most of the JS APIs do not differentiate between the profiles.
https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.cc (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:116: ui_oberver_ = base::WrapUnique(new UIObserver()); base::MakeUnique https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:131: ui_oberver_.release()); note: Seems odd in general that there is a pattern of std::unique_ptr getting deleted by DeleteSoon after a release. No matter what the unique_ptr doesn't call delete, so we might as well have a raw pointer... It's fine the way it is as well though I guess. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:133: // Silence unused variable warning in release builds. Not sure you really need this DCHECK. I don't know the best way to handle this, but a lot of places in content/ (maybe all places) trust the DeleteSoon. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.h (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:35: int32_t downstream_throughput_kbps) override; #include <stdint.h> Not sure if you have to because this is an override. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:40: std::unique_ptr<UIObserver> ui_oberver_; #include <memory> https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... File content/common/renderer.mojom (right): https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:164: // unavailable, it will be set to net::nqe::internal::InvalidRTT(). If the nit: Instead of/In addition to net::nqe::internal::InvalidRTT(), you might want to say -1. https://codereview.chromium.org/2857093002/diff/160001/content/public/browser... File content/public/browser/network_quality_observer_factory.h (right): https://codereview.chromium.org/2857093002/diff/160001/content/public/browser... content/public/browser/network_quality_observer_factory.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. Include this in content/public/browser/BUILD.gn
https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.cc (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No "(c)". https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:65: void InitOnUI() { nit: InitOnUIThread. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:106: net::nqe::internal::NetworkQuality last_notified_network_quality_; nit: No need for empty lines between members, if there are no comments. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:168: base::Unretained(ui_oberver_.get()), Why is this Unretained? ui_observer_ is owned by this object and if this object gets deleted before the task is executed on the UI thread, it could very well be deleted by then. You are using DeleteSoon as a workaround to that, which seems very hacky. All of the work in the UIObserver is done by OnRTTOrThroughputEstimatesComputed, which can be made a static method and posted to without lifetime concerns. If the requirement is to have all new processes also notified of the current condition, then it can also be stored in a static member? I guess overall that object can just be a singleton for the browser process and doesn't need to be owned by this object. Does that make sense? https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.h (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No "(c)". https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:28: class UIObserver; UIObserver implies that it is observing some user interface, not the UI thread. Maybe rename it to UiThreadObserver, so it is more explicit that it is about the UI thread? https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:42: net::NetworkQualityEstimator* network_quality_estimator_; Can you include a comment on how the lifetime of this member is related to the lifetime of the object? https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... File content/common/renderer.mojom (right): https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:160: // Tells the renderer that the network quality has changed. |http_rtt|, nit: s/renderer/renderer process/ https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:162: // of the HTTP RTT, transport RTT and downstream throughput (in kilobits per nit: You can omit the actual names, as the variables are very much self-describing. Do leave the units of those variables, as that is very important bit. https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:164: // unavailable, it will be set to net::nqe::internal::InvalidRTT(). If the On 2017/05/08 17:37:20, Ryan Sturm wrote: > nit: Instead of/In addition to net::nqe::internal::InvalidRTT(), you might want > to say -1. I'd rather not use magic values without defining it through a symbolic constant. There is also base::Optional, though I'm not sure what the overhead would be for a double parameter. https://codereview.chromium.org/2857093002/diff/160001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2857093002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2138: // the change in the network quality. Is there a reason why this code is not included in this CL? In general, we (security team) want to see the endpoint implementation of Mojo interfaces/methods in the same CL as the one introducing the interface/method.
Patchset #2 (id:180001) has been deleted
ryansturm, nasko: PTAL. Thanks! https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.cc (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/08 18:27:45, nasko wrote: > No "(c)". Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:65: void InitOnUI() { On 2017/05/08 18:27:45, nasko wrote: > nit: InitOnUIThread. Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:106: net::nqe::internal::NetworkQuality last_notified_network_quality_; On 2017/05/08 18:27:44, nasko wrote: > nit: No need for empty lines between members, if there are no comments. Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:116: ui_oberver_ = base::WrapUnique(new UIObserver()); On 2017/05/08 17:37:20, Ryan Sturm wrote: > base::MakeUnique Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:131: ui_oberver_.release()); On 2017/05/08 17:37:20, Ryan Sturm wrote: > note: Seems odd in general that there is a pattern of std::unique_ptr getting > deleted by DeleteSoon after a release. No matter what the unique_ptr doesn't > call delete, so we might as well have a raw pointer... It's fine the way it is > as well though I guess. Yes, it is okay to use a raw pointer as well. Using a unique ptr also seems like a pattern in content/. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:133: // Silence unused variable warning in release builds. On 2017/05/08 17:37:20, Ryan Sturm wrote: > Not sure you really need this DCHECK. I don't know the best way to handle this, > but a lot of places in content/ (maybe all places) trust the DeleteSoon. Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:168: base::Unretained(ui_oberver_.get()), On 2017/05/08 18:27:45, nasko wrote: > Why is this Unretained? ui_observer_ is owned by this object and if this object > gets deleted before the task is executed on the UI thread, |ui_observer_| is destroyed only after the destructor of |this| is invoked. So, IIUC, the code is safe (although it may be a bit hacky)? > it could very well be > deleted by then. You are using DeleteSoon as a workaround to that, which seems > very hacky. > > All of the work in the UIObserver is done by OnRTTOrThroughputEstimatesComputed, > which can be made a static method and posted to without lifetime concerns. If > the requirement is to have all new processes also notified of the current > condition, then it can also be stored in a static member? I guess overall that > object can just be a singleton for the browser process and doesn't need to be > owned by this object. Does that make sense? Using a singleton sounds reasonable. But, I thought that in general singletons are not preferred due to lifetime and testing issues? May be content/ has different design patterns and tradeoffs. Please let me know what works best here. Also, to make the context a bit more clear: I have to use UI thread because renderers can only be notified on UI thread and the new renderer notifications are delivered only on UI thread. I need to have a IO thread object because network quality is available only on IO thread. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.h (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/08 18:27:45, nasko wrote: > No "(c)". Done. I am not sure where I copied this from. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:28: class UIObserver; On 2017/05/08 18:27:45, nasko wrote: > UIObserver implies that it is observing some user interface, not the UI thread. > Maybe rename it to UiThreadObserver, so it is more explicit that it is about the > UI thread? Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:35: int32_t downstream_throughput_kbps) override; On 2017/05/08 17:37:20, Ryan Sturm wrote: > #include <stdint.h> > Not sure if you have to because this is an override. Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:40: std::unique_ptr<UIObserver> ui_oberver_; On 2017/05/08 17:37:20, Ryan Sturm wrote: > #include <memory> Done. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.h:42: net::NetworkQualityEstimator* network_quality_estimator_; On 2017/05/08 18:27:45, nasko wrote: > Can you include a comment on how the lifetime of this member is related to the > lifetime of the object? Done. https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... File content/common/renderer.mojom (right): https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:160: // Tells the renderer that the network quality has changed. |http_rtt|, On 2017/05/08 18:27:45, nasko wrote: > nit: s/renderer/renderer process/ Done. https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:162: // of the HTTP RTT, transport RTT and downstream throughput (in kilobits per On 2017/05/08 18:27:45, nasko wrote: > nit: You can omit the actual names, as the variables are very much > self-describing. Do leave the units of those variables, as that is very > important bit. Done. https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:164: // unavailable, it will be set to net::nqe::internal::InvalidRTT(). If the On 2017/05/08 17:37:20, Ryan Sturm wrote: > nit: Instead of/In addition to net::nqe::internal::InvalidRTT(), you might want > to say -1. I do not think I want to hardcode the number here. https://codereview.chromium.org/2857093002/diff/160001/content/common/rendere... content/common/renderer.mojom:164: // unavailable, it will be set to net::nqe::internal::InvalidRTT(). If the On 2017/05/08 18:27:45, nasko wrote: > On 2017/05/08 17:37:20, Ryan Sturm wrote: > > nit: Instead of/In addition to net::nqe::internal::InvalidRTT(), you might > want > > to say -1. > > I'd rather not use magic values without defining it through a symbolic constant. > There is also base::Optional, though I'm not sure what the overhead would be for > a double parameter. is it possible to IPC/mojoify base::Optional? https://codereview.chromium.org/2857093002/diff/160001/content/public/browser... File content/public/browser/network_quality_observer_factory.h (right): https://codereview.chromium.org/2857093002/diff/160001/content/public/browser... content/public/browser/network_quality_observer_factory.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/08 17:37:20, Ryan Sturm wrote: > Include this in content/public/browser/BUILD.gn Done. https://codereview.chromium.org/2857093002/diff/160001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2857093002/diff/160001/content/renderer/rende... content/renderer/render_thread_impl.cc:2138: // the change in the network quality. On 2017/05/08 18:27:45, nasko wrote: > Is there a reason why this code is not included in this CL? In general, we > (security team) want to see the endpoint implementation of Mojo > interfaces/methods in the same CL as the one introducing the interface/method. I can include that but I thought it would make the CL too long and a bit unwieldy. Here is the complete code path: https://codereview.chromium.org/2863973003/ (please disregard try failures. It needs to be rebased).
net/nqe: lgtm The whole approach seems fine, but I'll defer to content/ reviewers on policy/lifetime of content/ objects
LGTM, but please ensure there is sufficient comments documentation on the lifetime and cleanup of UiThreadObserver. https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.cc (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:168: base::Unretained(ui_oberver_.get()), On 2017/05/08 19:50:18, tbansal1 wrote: > On 2017/05/08 18:27:45, nasko wrote: > > Why is this Unretained? ui_observer_ is owned by this object and if this > object > > gets deleted before the task is executed on the UI thread, > |ui_observer_| is destroyed only after the destructor of > |this| is invoked. So, IIUC, the code > is safe (although it may be a bit hacky)? > > it could very well be > > deleted by then. You are using DeleteSoon as a workaround to that, which seems > > very hacky. > > > > > All of the work in the UIObserver is done by > OnRTTOrThroughputEstimatesComputed, > > which can be made a static method and posted to without lifetime concerns. If > > the requirement is to have all new processes also notified of the current > > condition, then it can also be stored in a static member? I guess overall that > > object can just be a singleton for the browser process and doesn't need to be > > owned by this object. Does that make sense? > > Using a singleton sounds reasonable. But, I thought that in general > singletons are not preferred due to lifetime and testing issues? > May be content/ has different design patterns and tradeoffs. Please let me know > what works best here. It depends. I think what you have here is alternative, but please make sure it is documented well, as it isn't as common of a pattern. > Also, to make the context a bit more clear: > I have to use UI thread because renderers can only be notified > on UI thread and the new renderer notifications are delivered only on UI thread. > > I need to have a IO thread object because network quality is available only on > IO thread. Yes, that part is clear. https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.cc (right): https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:67: registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CREATED, Shouldn't there be a Remove couterpart of this call in the destructor? https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:106: // The network quality that was last notified to the renderers. nit: s/notified/sent/ https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:133: ui_thread_observer_.release()); You probably want to check the return value of DeleteSoon and if false, then delete the object here. An example: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_io_data....
tbansal@chromium.org changed reviewers: + isherman@chromium.org, mmenke@chromium.org
mmenke: io_thread* isherman: histograms.xml. Thanks! https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.cc (right): https://codereview.chromium.org/2857093002/diff/160001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:168: base::Unretained(ui_oberver_.get()), On 2017/05/09 20:27:47, nasko wrote: > On 2017/05/08 19:50:18, tbansal1 wrote: > > On 2017/05/08 18:27:45, nasko wrote: > > > Why is this Unretained? ui_observer_ is owned by this object and if this > > object > > > gets deleted before the task is executed on the UI thread, > > |ui_observer_| is destroyed only after the destructor of > > |this| is invoked. So, IIUC, the code > > is safe (although it may be a bit hacky)? > > > it could very well be > > > deleted by then. You are using DeleteSoon as a workaround to that, which > seems > > > very hacky. > > > > > > > > All of the work in the UIObserver is done by > > OnRTTOrThroughputEstimatesComputed, > > > which can be made a static method and posted to without lifetime concerns. > If > > > the requirement is to have all new processes also notified of the current > > > condition, then it can also be stored in a static member? I guess overall > that > > > object can just be a singleton for the browser process and doesn't need to > be > > > owned by this object. Does that make sense? > > > > Using a singleton sounds reasonable. But, I thought that in general > > singletons are not preferred due to lifetime and testing issues? > > May be content/ has different design patterns and tradeoffs. Please let me > know > > what works best here. > > It depends. I think what you have here is alternative, but please make sure it > is documented well, as it isn't as common of a pattern. > > > Also, to make the context a bit more clear: > > I have to use UI thread because renderers can only be notified > > on UI thread and the new renderer notifications are delivered only on UI > thread. > > > > I need to have a IO thread object because network quality is available only on > > IO thread. > > Yes, that part is clear. Done. https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... File content/browser/net/network_quality_observer_impl.cc (right): https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:67: registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CREATED, On 2017/05/09 20:27:47, nasko wrote: > Shouldn't there be a Remove couterpart of this call in the destructor? Done. https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:106: // The network quality that was last notified to the renderers. On 2017/05/09 20:27:47, nasko wrote: > nit: s/notified/sent/ Done. https://codereview.chromium.org/2857093002/diff/200001/content/browser/net/ne... content/browser/net/network_quality_observer_impl.cc:133: ui_thread_observer_.release()); On 2017/05/09 20:27:47, nasko wrote: > You probably want to check the return value of DeleteSoon and if false, then > delete the object here. An example: > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_io_data.... Done.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... content/renderer/render_thread_impl.cc:2136: UMA_HISTOGRAM_EXACT_LINEAR("NQE.RenderThreadNotified", 1, 2); nit: UMA_HISTOGRAM_BOOLEAN("NQE.RenderThreadNotified", true); https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44389: +<histogram name="NQE.RenderThreadNotified" units="count"> nit: enum="BooleanHit" would be appropriate here. https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44394: + network quality estimator. How do you plan to interpret this metric? Is there some sort of baseline that you are planning to compare to? If so, could you build it into this baseline? Next-best option: Could you document the baseline that's expected to be used here?
https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... content/renderer/render_thread_impl.cc:2136: UMA_HISTOGRAM_EXACT_LINEAR("NQE.RenderThreadNotified", 1, 2); On 2017/05/09 21:51:25, Ilya Sherman wrote: > nit: UMA_HISTOGRAM_BOOLEAN("NQE.RenderThreadNotified", true); Done. Can you comment a bit on why EXACT_LINEAR is not the right macro here? https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44389: +<histogram name="NQE.RenderThreadNotified" units="count"> On 2017/05/09 21:51:25, Ilya Sherman wrote: > nit: enum="BooleanHit" would be appropriate here. Done. https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44394: + network quality estimator. On 2017/05/09 21:51:25, Ilya Sherman wrote: > How do you plan to interpret this metric? Is there some sort of baseline that > you are planning to compare to? If so, could you build it into this baseline? > Next-best option: Could you document the baseline that's expected to be used > here? I plan to evaluate how frequently the notification happens over some other metric (e.g., number of page loads or number of renderers). There is no baseline except that we know that if the ratio is too high (e.g., 100 notifications per renderer) or too low (< 1) then it is bad.
Thanks. LGTM % comment: https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... content/renderer/render_thread_impl.cc:2136: UMA_HISTOGRAM_EXACT_LINEAR("NQE.RenderThreadNotified", 1, 2); On 2017/05/09 23:49:30, tbansal1 wrote: > On 2017/05/09 21:51:25, Ilya Sherman wrote: > > nit: UMA_HISTOGRAM_BOOLEAN("NQE.RenderThreadNotified", true); > > Done. Can you comment a bit on why EXACT_LINEAR is not the right macro here? EXACT_LINEAR would be fine as well, I think -- though the final param probably needs to be 3 rather than 2, as all histograms are expected to have an underflow and an overflow bucket, plus at least one additional bucket for "real" data. Mostly, UMA_HISTOGRAM_BOOLEAN is just less verbose, and hence more readable =) https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44394: + network quality estimator. On 2017/05/09 23:49:31, tbansal1 wrote: > On 2017/05/09 21:51:25, Ilya Sherman wrote: > > How do you plan to interpret this metric? Is there some sort of baseline that > > you are planning to compare to? If so, could you build it into this baseline? > > > Next-best option: Could you document the baseline that's expected to be used > > here? > > I plan to evaluate how frequently the notification happens over some other > metric (e.g., number of page loads or number of renderers). There is no baseline > except that we know that if the ratio is too high (e.g., 100 notifications per > renderer) or too low (< 1) then it is bad. Okay. Could you please add this hint to the histogram description? Something like "This metric makes sense to compare in frequency relative to other events like the number of page loads or the number of renderers seen. It's expected that we'll see at least 1 notification per renderer (on average), and < 100." (This is actually what I meant by a baseline -- the metric relative to which to compare. Not the expected value at the baseline =].) Even better would be to build these events into an enum that's recorded as part of this same metric, so that it's easy to visually compare the numbers. But if that requires jumping through too many hoops, then just providing a textual reference is okay as a fallback.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
mmenke: ptal at io_thread*. Thanks. https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... content/renderer/render_thread_impl.cc:2136: UMA_HISTOGRAM_EXACT_LINEAR("NQE.RenderThreadNotified", 1, 2); On 2017/05/09 23:55:52, Ilya Sherman wrote: > On 2017/05/09 23:49:30, tbansal1 wrote: > > On 2017/05/09 21:51:25, Ilya Sherman wrote: > > > nit: UMA_HISTOGRAM_BOOLEAN("NQE.RenderThreadNotified", true); > > > > Done. Can you comment a bit on why EXACT_LINEAR is not the right macro here? > > EXACT_LINEAR would be fine as well, I think -- though the final param probably > needs to be 3 rather than 2, as all histograms are expected to have an underflow > and an overflow bucket, plus at least one additional bucket for "real" data. > Mostly, UMA_HISTOGRAM_BOOLEAN is just less verbose, and hence more readable =) Acknowledged. https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44394: + network quality estimator. On 2017/05/09 23:55:52, Ilya Sherman wrote: > On 2017/05/09 23:49:31, tbansal1 wrote: > > On 2017/05/09 21:51:25, Ilya Sherman wrote: > > > How do you plan to interpret this metric? Is there some sort of baseline > that > > > you are planning to compare to? If so, could you build it into this > baseline? > > > > > Next-best option: Could you document the baseline that's expected to be used > > > here? > > > > I plan to evaluate how frequently the notification happens over some other > > metric (e.g., number of page loads or number of renderers). There is no > baseline > > except that we know that if the ratio is too high (e.g., 100 notifications per > > renderer) or too low (< 1) then it is bad. > > Okay. Could you please add this hint to the histogram description? Something > like "This metric makes sense to compare in frequency relative to other events > like the number of page loads or the number of renderers seen. It's expected > that we'll see at least 1 notification per renderer (on average), and < 100." > (This is actually what I meant by a baseline -- the metric relative to which to > compare. Not the expected value at the baseline =].) > > Even better would be to build these events into an enum that's recorded as part > of this same metric, so that it's easy to visually compare the numbers. But if > that requires jumping through too many hoops, then just providing a textual > reference is okay as a fallback. I have expanded on the histogram description. Thanks. I think visually comparing is okay for now.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/10 16:46:42, tbansal1 wrote: > mmenke: ptal at io_thread*. Thanks. > > https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/2857093002/diff/220001/content/renderer/rende... > content/renderer/render_thread_impl.cc:2136: > UMA_HISTOGRAM_EXACT_LINEAR("NQE.RenderThreadNotified", 1, 2); > On 2017/05/09 23:55:52, Ilya Sherman wrote: > > On 2017/05/09 23:49:30, tbansal1 wrote: > > > On 2017/05/09 21:51:25, Ilya Sherman wrote: > > > > nit: UMA_HISTOGRAM_BOOLEAN("NQE.RenderThreadNotified", true); > > > > > > Done. Can you comment a bit on why EXACT_LINEAR is not the right macro here? > > > > EXACT_LINEAR would be fine as well, I think -- though the final param probably > > needs to be 3 rather than 2, as all histograms are expected to have an > underflow > > and an overflow bucket, plus at least one additional bucket for "real" data. > > Mostly, UMA_HISTOGRAM_BOOLEAN is just less verbose, and hence more readable =) > > Acknowledged. > > https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2857093002/diff/220001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:44394: + network quality estimator. > On 2017/05/09 23:55:52, Ilya Sherman wrote: > > On 2017/05/09 23:49:31, tbansal1 wrote: > > > On 2017/05/09 21:51:25, Ilya Sherman wrote: > > > > How do you plan to interpret this metric? Is there some sort of baseline > > that > > > > you are planning to compare to? If so, could you build it into this > > baseline? > > > > > > > Next-best option: Could you document the baseline that's expected to be > used > > > > here? > > > > > > I plan to evaluate how frequently the notification happens over some other > > > metric (e.g., number of page loads or number of renderers). There is no > > baseline > > > except that we know that if the ratio is too high (e.g., 100 notifications > per > > > renderer) or too low (< 1) then it is bad. > > > > Okay. Could you please add this hint to the histogram description? Something > > like "This metric makes sense to compare in frequency relative to other events > > like the number of page loads or the number of renderers seen. It's expected > > that we'll see at least 1 notification per renderer (on average), and < 100." > > (This is actually what I meant by a baseline -- the metric relative to which > to > > compare. Not the expected value at the baseline =].) > > > > Even better would be to build these events into an enum that's recorded as > part > > of this same metric, so that it's easy to visually compare the numbers. But > if > > that requires jumping through too many hoops, then just providing a textual > > reference is okay as a fallback. > > I have expanded on the histogram description. Thanks. > I think visually comparing is okay for now. io_thread LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, nasko@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2857093002/#ps260001 (title: "isherman comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1494440706061500,
"parent_rev": "0d281667bf4b01cb681063b0aef79846a7b06ac1", "commit_rev":
"15973c3f1ac558b3b3eed70a1a7a3ed256528a65"}
Message was sent while issue was closed.
Description was changed from ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. This is similar to how content::BrowserOnlineStateObserver listens to the connection type changes. BUG=719511 ========== to ========== Expose changes in the network quality to the renderers A network quality observer owned by IOThread has been added. It listens to the network quality changes and notifies renderers on the UI thread using mojo. This is similar to how content::BrowserOnlineStateObserver listens to the connection type changes. BUG=719511 Review-Url: https://codereview.chromium.org/2857093002 Cr-Commit-Position: refs/heads/master@{#470654} Committed: https://chromium.googlesource.com/chromium/src/+/15973c3f1ac558b3b3eed70a1a7a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:260001) as https://chromium.googlesource.com/chromium/src/+/15973c3f1ac558b3b3eed70a1a7a... |
