content/browser/DEPS - Issue 2510803003: Pass RapporService to content/browser/

Side by Side Diff: content/browser/DEPS

Issue 2510803003: Pass RapporService to content/browser/ (Closed)

Patch Set: Add a test for the current url API Created 4 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« chrome/browser/ui/browser.h ('K') | « content/browser/BUILD.gn ('k') | content/browser/renderer_host/render_widget_host_delegate.h » ('j') | content/browser/renderer_host/render_widget_host_delegate.h » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 include_rules = [	1 include_rules = [

2 # Allow inclusion of specific components that we depend on.	2 # Allow inclusion of specific components that we depend on.

3 # See comment in content/DEPS for which components are allowed.	3 # See comment in content/DEPS for which components are allowed.
	Charlie Reis 2016/11/16 23:55:21 It's not clear to me whether rappor passes the bar It's not clear to me whether rappor passes the bar mentioned in content/DEPS or not. This service only seems to exist in chrome/, so this feels uncomfortably close to introducing a dependency on chrome/ in content/. Nick, do you have a sense for whether this is ok? I know you added some Rappor metrics and have dealt with the chrome/content divide. (I'd ask jam@, but he's OOO.) ncarter (slow) 2016/11/17 00:08:30 If we've already got rappor metrics plumbed into c Show quoted text On 2016/11/16 23:55:21, Charlie Reis (slow) wrote: > It's not clear to me whether rappor passes the bar mentioned in content/DEPS or > not. This service only seems to exist in chrome/, so this feels uncomfortably > close to introducing a dependency on chrome/ in content/. > > Nick, do you have a sense for whether this is ok? I know you added some Rappor > metrics and have dealt with the chrome/content divide. (I'd ask jam@, but he's > OOO.) If we've already got rappor metrics plumbed into content/renderer without needing a DEPS relaxation, then I would expect it should be possible to use a similar API to get it working from content/browser. If that suffices for this use case. In general I think of rappor as being very close to base/metrics, so this doesn't seem particularly crazy. Navid Zolghadr 2016/11/17 20:05:49 The existing methods that we use in content/render Show quoted text On 2016/11/17 00:08:30, ncarter wrote: > On 2016/11/16 23:55:21, Charlie Reis (slow) wrote: > > It's not clear to me whether rappor passes the bar mentioned in content/DEPS > or > > not. This service only seems to exist in chrome/, so this feels uncomfortably > > close to introducing a dependency on chrome/ in content/. > > > > Nick, do you have a sense for whether this is ok? I know you added some > Rappor > > metrics and have dealt with the chrome/content divide. (I'd ask jam@, but > he's > > OOO.) > > If we've already got rappor metrics plumbed into content/renderer without > needing a DEPS relaxation, then I would expect it should be possible to use a > similar API to get it working from content/browser. If that suffices for this > use case. > > In general I think of rappor as being very close to base/metrics, so this > doesn't seem particularly crazy. The existing methods that we use in content/renderer are recording simple stuff like a string. Originally I wanted to expose the rappor::sample so one can record anything. Here is the original review for this having this change and one example of using it in one of our metrics: https://codereview.chromium.org/2492793004/ So I should either add the dependency or find a way to expose more APIs that satisfy our needs for now here https://cs.chromium.org/chromium/src/components/rappor/rappor_recorder_impl.h... to the RapporRecorder. Which way do you think is better? Having what I have in this review or do the other suggestion? Navid Zolghadr 2016/12/01 18:27:15 I added the subclass/interface under component/rap Show quoted text On 2016/11/17 20:05:49, Navid Zolghadr wrote: > On 2016/11/17 00:08:30, ncarter wrote: > > On 2016/11/16 23:55:21, Charlie Reis (slow) wrote: > > > It's not clear to me whether rappor passes the bar mentioned in content/DEPS > > or > > > not. This service only seems to exist in chrome/, so this feels > uncomfortably > > > close to introducing a dependency on chrome/ in content/. > > > > > > Nick, do you have a sense for whether this is ok? I know you added some > > Rappor > > > metrics and have dealt with the chrome/content divide. (I'd ask jam@, but > > he's > > > OOO.) > > > > If we've already got rappor metrics plumbed into content/renderer without > > needing a DEPS relaxation, then I would expect it should be possible to use a > > similar API to get it working from content/browser. If that suffices for this > > use case. > > > > In general I think of rappor as being very close to base/metrics, so this > > doesn't seem particularly crazy. > > The existing methods that we use in content/renderer are recording simple stuff > like a string. Originally I wanted to expose the rappor::sample so one can > record anything. Here is the original review for this having this change and one > example of using it in one of our metrics: > https://codereview.chromium.org/2492793004/ > > So I should either add the dependency or find a way to expose more APIs that > satisfy our needs for now here > https://cs.chromium.org/chromium/src/components/rappor/rappor_recorder_impl.h... > > to the RapporRecorder. Which way do you think is better? Having what I have in > this review or do the other suggestion? > > I added the subclass/interface under component/rappor/public and I managed to makes the DEPS to use that. Would this be enough? I realized it is also possible to make use of the abstracted interface in a lot of other places in chrome/browser as well. I wasn't sure if you wanted to me to refactor those as well or write a one pager or something for it first or not. I also renamed one of the functions as holte@ added a TODO in the code.
4 "+components/discardable_memory/common",	4 "+components/discardable_memory/common",

5 "+components/discardable_memory/service",	5 "+components/discardable_memory/service",

6 "+components/filesystem",	6 "+components/filesystem",

7 "+components/leveldb",	7 "+components/leveldb",

8 "+components/link_header_util",	8 "+components/link_header_util",

9 "+components/mime_util",	9 "+components/mime_util",

10 "+components/payments",	10 "+components/payments",

11 "+components/profile_service",	11 "+components/profile_service",

	12 "+components/rappor",

12 "+components/scheduler/common",	13 "+components/scheduler/common",

13 "+components/tracing",	14 "+components/tracing",

14 "+components/url_formatter",	15 "+components/url_formatter",

15	16

16 "+content/app/strings/grit", # For generated headers	17 "+content/app/strings/grit", # For generated headers

17 "+content/public/browser",	18 "+content/public/browser",

18 "+device/battery", # For battery status service.	19 "+device/battery", # For battery status service.

19 "+device/gamepad", # For gamepad API	20 "+device/gamepad", # For gamepad API

20 "+device/generic_sensor", # For sensors service.	21 "+device/generic_sensor", # For sensors service.

21 "+device/geolocation",	22 "+device/geolocation",

(...skipping 99 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
121 # Until we define where mojo interfaces should live in blink we whitelist each	122 # Until we define where mojo interfaces should live in blink we whitelist each

122 # one separately.	123 # one separately.

123 "+third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom.h",	124 "+third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom.h",

124 "+third_party/WebKit/public/platform/modules/notifications/notification_servic e.mojom.h",	125 "+third_party/WebKit/public/platform/modules/notifications/notification_servic e.mojom.h",

125 "+third_party/WebKit/public/platform/modules/shapedetection/shapedetection.moj om.h",	126 "+third_party/WebKit/public/platform/modules/shapedetection/shapedetection.moj om.h",

126	127

127 # DO NOT ADD ANY CHROME OR COMPONENTS INCLUDES HERE!!!	128 # DO NOT ADD ANY CHROME OR COMPONENTS INCLUDES HERE!!!

128 # See https://sites.google.com/a/chromium.org/dev/developers/content-module	129 # See https://sites.google.com/a/chromium.org/dev/developers/content-module

129 # for more information.	130 # for more information.

130 ]	131 ]

OLD	NEW