Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727233002:
Don't collect a log per peerconnection if webrtc-internals isn't open. (Closed)
|
Created:
3 years, 9 months ago by tommi (sloooow) - chröme Modified:
3 years, 9 months ago Reviewers:
Henrik Grunell CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't collect a log per peerconnection if webrtc-internals isn't open.
Maintaining this list can be quite taxing in terms of memory and CPU.
Moving forward, I think we should change the behavior so that there's
no perf impact if chrome://webrtc-internals isn't open.
BUG=697618
Review-Url: https://codereview.chromium.org/2727233002
Cr-Commit-Position: refs/heads/master@{#454862}
Committed: https://chromium.googlesource.com/chromium/src/+/15e9cea557ba0b294d87cc8eb64317ad2490514e
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix bug! #Patch Set 3 : Fix issues #
Total comments: 15
Patch Set 4 : Address comments, add tests #
Messages
Total messages: 31 (19 generated)
tommi@chromium.org changed reviewers: + guidou@chromium.org
The CQ bit was checked by tommi@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...
Fix bug!
https://codereview.chromium.org/2727233002/diff/1/content/browser/webrtc/webr... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2727233002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_internals.cc:45: dict->Set("log", new base::ListValue()); oops... fix is on the way.
The CQ bit was checked by tommi@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fix issues
The CQ bit was checked by tommi@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.
tommi@chromium.org changed reviewers: + grunell@chromium.org - guidou@chromium.org
Henrik - can you take a look? -guidou since he's ooo
In addition to comments: is there an integration test for this that needs to be updated/extended? https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:54: auto* dict = static_cast<base::DictionaryValue*>(value); Not being particularly familiar with base::Value, is this really how one would get the value of the right type? Just doesn't seem very elegant, but if it's the way then fine. https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:158: void WebRTCInternals::OnRemovePeerConnection(ProcessId pid, int lid) { Great that you cleaned up the awkward code in this function! https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:161: size_t i; Nit: perhaps a more descriptive variable name? https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:201: SendUpdate("updatePeerConnection", std::move(update)); In the previous code, this would not be done if no record is found, in the new code it is. Is that intentional? (Maybe comment on why?) https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:271: FreeLogList(dictionary.get()); I don't know the design of WebRTC internals, so to make sure: we don't lose any data that we might want to display later when opening the chrome://webrtc-internals page when doing this? https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals_unittest.cc (right): https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals_unittest.cc:388: } // namespace content Would it make sense to add a test case for when there is no observer when adding a pc? And when removing the last observer when still having a pc?
Address comments, add tests
https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:54: auto* dict = static_cast<base::DictionaryValue*>(value); On 2017/03/03 12:40:09, Henrik Grunell wrote: > Not being particularly familiar with base::Value, is this really how one would > get the value of the right type? Just doesn't seem very elegant, but if it's the > way then fine. Yes, it's a polymorphic design with an enum that points to what actual type is used (comparable to a VARIANT). E.g. look at static_cast<>s here: https://cs.chromium.org/chromium/src/base/values.cc?type=cs&sq=package:chromium https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:158: void WebRTCInternals::OnRemovePeerConnection(ProcessId pid, int lid) { On 2017/03/03 12:40:09, Henrik Grunell wrote: > Great that you cleaned up the awkward code in this function! :D https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:161: size_t i; On 2017/03/03 12:40:09, Henrik Grunell wrote: > Nit: perhaps a more descriptive variable name? Done... although I only changed it to 'index'. |i| is what was being used before and typically stands for index anyway. https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:201: SendUpdate("updatePeerConnection", std::move(update)); On 2017/03/03 12:40:09, Henrik Grunell wrote: > In the previous code, this would not be done if no record is found, in the new > code it is. Is that intentional? (Maybe comment on why?) Not intentional. Changed the behavior back now. https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:271: FreeLogList(dictionary.get()); On 2017/03/03 12:40:09, Henrik Grunell wrote: > I don't know the design of WebRTC internals, so to make sure: we don't lose any > data that we might want to display later when opening the > chrome://webrtc-internals page when doing this? We do loose data but the thinking is that when chrome://webrtc-internals is closed, that's a signal that the data isn't needed anymore. https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals_unittest.cc (right): https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals_unittest.cc:388: } // namespace content On 2017/03/03 12:40:09, Henrik Grunell wrote: > Would it make sense to add a test case for when there is no observer when adding > a pc? And when removing the last observer when still having a pc? Good idea, done.
The CQ bit was checked by tommi@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.
lgtm https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:54: auto* dict = static_cast<base::DictionaryValue*>(value); On 2017/03/03 14:15:14, tommi - chröme wrote: > On 2017/03/03 12:40:09, Henrik Grunell wrote: > > Not being particularly familiar with base::Value, is this really how one would > > get the value of the right type? Just doesn't seem very elegant, but if it's > the > > way then fine. > > Yes, it's a polymorphic design with an enum that points to what actual type is > used (comparable to a VARIANT). > E.g. look at static_cast<>s here: > https://cs.chromium.org/chromium/src/base/values.cc?type=cs&sq=package:chromium Acknowledged. https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:271: FreeLogList(dictionary.get()); On 2017/03/03 14:15:14, tommi - chröme wrote: > On 2017/03/03 12:40:09, Henrik Grunell wrote: > > I don't know the design of WebRTC internals, so to make sure: we don't lose > any > > data that we might want to display later when opening the > > chrome://webrtc-internals page when doing this? > > We do loose data but the thinking is that when chrome://webrtc-internals is > closed, that's a signal that the data isn't needed anymore. OK, if that's the intention that's fine then. https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals_unittest.cc (right): https://codereview.chromium.org/2727233002/diff/40001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals_unittest.cc:388: } // namespace content On 2017/03/03 14:15:14, tommi - chröme wrote: > On 2017/03/03 12:40:09, Henrik Grunell wrote: > > Would it make sense to add a test case for when there is no observer when > adding > > a pc? And when removing the last observer when still having a pc? > > Good idea, done. Great!
The CQ bit was checked by tommi@chromium.org
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": 60001, "attempt_start_ts": 1488802311693060,
"parent_rev": "70079b5955de2a047669c39938839851fb107b44", "commit_rev":
"15e9cea557ba0b294d87cc8eb64317ad2490514e"}
Message was sent while issue was closed.
Description was changed from ========== Don't collect a log per peerconnection if webrtc-internals isn't open. Maintaining this list can be quite taxing in terms of memory and CPU. Moving forward, I think we should change the behavior so that there's no perf impact if chrome://webrtc-internals isn't open. BUG=697618 ========== to ========== Don't collect a log per peerconnection if webrtc-internals isn't open. Maintaining this list can be quite taxing in terms of memory and CPU. Moving forward, I think we should change the behavior so that there's no perf impact if chrome://webrtc-internals isn't open. BUG=697618 Review-Url: https://codereview.chromium.org/2727233002 Cr-Commit-Position: refs/heads/master@{#454862} Committed: https://chromium.googlesource.com/chromium/src/+/15e9cea557ba0b294d87cc8eb643... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/15e9cea557ba0b294d87cc8eb643... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
