Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2526643002:
Re-enable rappor for vrshell (Closed)
DescriptionRe-enable rappor for vrshell
There was a dependency cycle and resulting test code build break when
consuming g_browser_process from vr_shell. This fix provides a way to
report rappor metrics in components that don't depend on chrome/browser.
BUG=667900
Committed: https://crrev.com/fc622a50ae2f9501d07b271fe22181ed23bde1f5
Cr-Commit-Position: refs/heads/master@{#435119}
Patch Set 1 #
Total comments: 10
Patch Set 2 : CR feedback #Patch Set 3 : CR feedback #
Messages
Total messages: 31 (18 generated)
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
billorr@chromium.org changed reviewers: + amp@chromium.org, ddorwin@chromium.org
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.
https://codereview.chromium.org/2526643002/diff/1/chrome/browser/browser_proc... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2526643002/diff/1/chrome/browser/browser_proc... chrome/browser/browser_process_impl.cc:226: rappor::GetDefaultService = &GetBrowserRapporService; Use a setter instead. This will also allow you to DCHECK that it isn't being called multiple times. Better yet, why not pass rappor_service() rather than a callback? https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.cc:48: void TrySampleDomainAndRegistryWithDefaultService(const std::string& metric, Since we don't even check whether there is a default service, could we just have callers pass GetDefaultService() and eliminate the need for this? https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.h:38: void TrySampleDomainAndRegistryWithDefaultService(const std::string& metric, Is "Try" a common pattern in the code? If not, probably drop it. https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.h:41: RapporService* TryGetDefaultService(); Ditto, though this should have a comment noting that it can return NULL if there is no such service.
https://codereview.chromium.org/2526643002/diff/1/chrome/browser/browser_proc... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2526643002/diff/1/chrome/browser/browser_proc... chrome/browser/browser_process_impl.cc:226: rappor::GetDefaultService = &GetBrowserRapporService; On 2016/11/28 21:45:42, ddorwin wrote: > Use a setter instead. This will also allow you to DCHECK that it isn't being > called multiple times. > > Better yet, why not pass rappor_service() rather than a callback? Using a setting sounds good. If I recall looking at the code/documentation, the rappor_service() may be recreated, so you don't want to hold a reference to it. https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.cc:48: void TrySampleDomainAndRegistryWithDefaultService(const std::string& metric, On 2016/11/28 21:45:42, ddorwin wrote: > Since we don't even check whether there is a default service, could we just have > callers pass GetDefaultService() and eliminate the need for this? Done. https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.h:38: void TrySampleDomainAndRegistryWithDefaultService(const std::string& metric, On 2016/11/28 21:45:42, ddorwin wrote: > Is "Try" a common pattern in the code? If not, probably drop it. Removing this function per your other comment. https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.h:41: RapporService* TryGetDefaultService(); On 2016/11/28 21:45:42, ddorwin wrote: > Ditto, though this should have a comment noting that it can return NULL if there > is no such service. Done.
The CQ bit was checked by billorr@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.
billorr@chromium.org changed reviewers: + bshe@chromium.org, holte@chromium.org, sky@chromium.org
sky@chromium.org: Please review changes in browser_process_impl.cc holte@chromium.org: Please review changes in rappor_utils bshe@chromium.org: Please review changes in vrshell
LGTM
lgtm once function declaration is cleaned up https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.h:38: void TrySampleDomainAndRegistryWithDefaultService(const std::string& metric, On 2016/11/28 23:13:03, billorr wrote: > On 2016/11/28 21:45:42, ddorwin wrote: > > Is "Try" a common pattern in the code? If not, probably drop it. > > Removing this function per your other comment. It's still here?
https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... components/rappor/rappor_utils.h:38: void TrySampleDomainAndRegistryWithDefaultService(const std::string& metric, On 2016/11/29 20:42:09, Steven Holte wrote: > On 2016/11/28 23:13:03, billorr wrote: > > On 2016/11/28 21:45:42, ddorwin wrote: > > > Is "Try" a common pattern in the code? If not, probably drop it. > > > > Removing this function per your other comment. > > It's still here? Done.
On 2016/11/29 21:32:26, billorr wrote: > https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... > File components/rappor/rappor_utils.h (right): > > https://codereview.chromium.org/2526643002/diff/1/components/rappor/rappor_ut... > components/rappor/rappor_utils.h:38: void > TrySampleDomainAndRegistryWithDefaultService(const std::string& metric, > On 2016/11/29 20:42:09, Steven Holte wrote: > > On 2016/11/28 23:13:03, billorr wrote: > > > On 2016/11/28 21:45:42, ddorwin wrote: > > > > Is "Try" a common pattern in the code? If not, probably drop it. > > > > > > Removing this function per your other comment. > > > > It's still here? > > Done. lgtm
The CQ bit was checked by billorr@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...
lgtm
lgtm lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by billorr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2526643002/#ps40001 (title: "CR feedback")
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": 40001, "attempt_start_ts": 1480463999937840,
"parent_rev": "1c4ffb1ae76f23f207f4b3288d0807b01bb6df8f", "commit_rev":
"f5cd1ab53896371bab481fca13e4cabfa9d2f9e6"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Re-enable rappor for vrshell There was a dependency cycle and resulting test code build break when consuming g_browser_process from vr_shell. This fix provides a way to report rappor metrics in components that don't depend on chrome/browser. BUG=667900 ========== to ========== Re-enable rappor for vrshell There was a dependency cycle and resulting test code build break when consuming g_browser_process from vr_shell. This fix provides a way to report rappor metrics in components that don't depend on chrome/browser. BUG=667900 Committed: https://crrev.com/fc622a50ae2f9501d07b271fe22181ed23bde1f5 Cr-Commit-Position: refs/heads/master@{#435119} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fc622a50ae2f9501d07b271fe22181ed23bde1f5 Cr-Commit-Position: refs/heads/master@{#435119} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
