Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1492033002:
Add UMA histogram for the number of SiteInstances per BrowsingInstance. (Closed)
DescriptionAdd UMA histogram for the number of SiteInstances per BrowsingInstance.
BUG=542921
Committed: https://crrev.com/502737131f5327ade8f728c66d7278f3ad3cbaa5
Cr-Commit-Position: refs/heads/master@{#362988}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixes based on Nick's review. #
Total comments: 6
Patch Set 3 : Fixes based on Scott's review. #
Messages
Total messages: 27 (11 generated)
nasko@chromium.org changed reviewers: + nick@chromium.org
Hey Nick, Can you review this CL for me? It adds UMA histogram for the count of SiteInstances per BrowsingInstance. Thanks in advance! Nasko
https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details.cc File chrome/browser/site_details.cc (right): https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:7: #include "base/containers/hash_tables.h" This #include belongs in the .h file. https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:119: break; Just return here. That'll let you get rid of the |primary| local var altogether, and shave a couple lines off the function. https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:126: std::make_pair(instance, std::set<SiteInstance*>())); Shouldn't the new std::set contain |instance| initially? Seems like a subsequent calls to DeterminePrimarySiteInstance will insert it, so that seems inconsistent. (Probably this isn't a bug in practice because DeterminePrimarySiteInstance always happens twice on the main frame) See my next comment, however. https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:138: DeterminePrimarySiteInstance(frame->GetSiteInstance(), site_data); We've already determined the primary site instance for the webcontents's browsinginstance; if we plumbed it in here, this operation could become: site_data->instances[primary].insert(frame->GetSiteInstance())
https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details.cc File chrome/browser/site_details.cc (right): https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:7: #include "base/containers/hash_tables.h" On 2015/12/02 18:16:49, ncarter wrote: > This #include belongs in the .h file. Done. https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:119: break; On 2015/12/02 18:16:49, ncarter wrote: > Just return here. That'll let you get rid of the |primary| local var altogether, > and shave a couple lines off the function. Done. https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:126: std::make_pair(instance, std::set<SiteInstance*>())); On 2015/12/02 18:16:49, ncarter wrote: > Shouldn't the new std::set contain |instance| initially? Seems like a subsequent > calls to DeterminePrimarySiteInstance will insert it, so that seems > inconsistent. (Probably this isn't a bug in practice because > DeterminePrimarySiteInstance always happens twice on the main frame) > > See my next comment, however. Acknowledged. https://codereview.chromium.org/1492033002/diff/1/chrome/browser/site_details... chrome/browser/site_details.cc:138: DeterminePrimarySiteInstance(frame->GetSiteInstance(), site_data); On 2015/12/02 18:16:49, ncarter wrote: > We've already determined the primary site instance for the webcontents's > browsinginstance; if we plumbed it in here, this operation could become: > > site_data->instances[primary].insert(frame->GetSiteInstance()) Unfortunately not. Due to crbug.com/522302 not all SiteInstances in a FrameTree are in the same BrowsingInstance. We should be counting reality until we fix this.
nasko@chromium.org changed reviewers: + isherman@chromium.org, sky@chromium.org
Adding OWNERS reviewers: sky@ - chrome/ isherman@ - histograms
Description was changed from ========== Add UMA histogram for the number of SiteInstances per BrowsingInstance. BUG=542921 ========== to ========== Add UMA histogram for the number of SiteInstances per BrowsingInstance. BUG=542921 ==========
lgtm
Histograms LGTM
https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... File chrome/browser/site_details.cc (right): https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... chrome/browser/site_details.cc:113: for (auto& already_collected_instance : site_data->instances) { nit: already_collected_instance is a bit confusing as a name as it's really a pair. https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... chrome/browser/site_details.cc:126: result.first->second.insert(instance); Unless I'm missing something, isn't what you have above the same as: site_data->instances[instance].clear(); site_data->instances[instance].insert(instance); which is a lot easier to understand. https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... chrome/browser/site_details.cc:200: for (const auto& site : i->second.instances) { Above you call 'site' 'already_collected_instance'. Both are confusing, but be consistent.
https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... File chrome/browser/site_details.cc (right): https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... chrome/browser/site_details.cc:113: for (auto& already_collected_instance : site_data->instances) { On 2015/12/03 00:04:05, sky wrote: > nit: already_collected_instance is a bit confusing as a name as it's really a > pair. Yes, it is a pair, but it is the SiteInstance and a set of its related other SiteInstances. So we are in a sense iterating through all of the known SiteInstances. I've changed it to existing_site_instance as I can't find a better name. Any suggestions are welcome. https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... chrome/browser/site_details.cc:126: result.first->second.insert(instance); On 2015/12/03 00:04:05, sky wrote: > Unless I'm missing something, isn't what you have above the same as: > site_data->instances[instance].clear(); > site_data->instances[instance].insert(instance); > > which is a lot easier to understand. Done. https://codereview.chromium.org/1492033002/diff/20001/chrome/browser/site_det... chrome/browser/site_details.cc:200: for (const auto& site : i->second.instances) { On 2015/12/03 00:04:05, sky wrote: > Above you call 'site' 'already_collected_instance'. Both are confusing, but be > consistent. Changed to be more consistent.
Personally I would go with pair, but what you have is fine. LGTM
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1492033002/#ps40001 (title: "Fixes based on Scott's review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492033002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492033002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492033002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492033002/40001
Message was sent while issue was closed.
Description was changed from ========== Add UMA histogram for the number of SiteInstances per BrowsingInstance. BUG=542921 ========== to ========== Add UMA histogram for the number of SiteInstances per BrowsingInstance. BUG=542921 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA histogram for the number of SiteInstances per BrowsingInstance. BUG=542921 ========== to ========== Add UMA histogram for the number of SiteInstances per BrowsingInstance. BUG=542921 Committed: https://crrev.com/502737131f5327ade8f728c66d7278f3ad3cbaa5 Cr-Commit-Position: refs/heads/master@{#362988} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/502737131f5327ade8f728c66d7278f3ad3cbaa5 Cr-Commit-Position: refs/heads/master@{#362988} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
