Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285903002:
Framework to ascribe all network data use to a source. (Closed)
|
Created:
4 years, 3 months ago by Not at Google. Contact bengr Modified:
4 years, 3 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFramework to ascribe all network data use to a source.
DataUseAscriber is responsible for grouping URLRequest from the
same source by mapping them to the same DataUseRecorder instance.
Applications must provide an override if they are interested in data
use ascription.
ChromeDataUseAscriber is the browser override that maps URL requests
from the a page load to the same DataUseRecorder. It maintains
navigation and frame information on the IO thread to perform this
many-to-one mapping. ChromeDataUseAscriberService is the UI thread
counterpart that observes navigation and frame events on the UI thread
and forwards the calls to ChromeDataUseAscriber.
BUG=645188
Committed: https://crrev.com/f617523bd19d6e884986664ec8f9bc045353b702
Cr-Commit-Position: refs/heads/master@{#417740}
Patch Set 1 #Patch Set 2 : Improve build riles #Patch Set 3 : Fix tests #Patch Set 4 : Fix unit tests #Patch Set 5 : Move ascriber to chrome/browser #
Total comments: 31
Patch Set 6 : Add test #
Total comments: 2
Patch Set 7 : Comments #
Total comments: 46
Patch Set 8 : Addressed comments from thestig@ #
Total comments: 10
Patch Set 9 : Addressed nits #Messages
Total messages: 52 (38 generated)
The CQ bit was checked by kundaji@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 ========== Framework to ascribe all network data use to a source. |DataUseAscriber| is responsible for grouping |URLRequest| from the same source by mapping them to the same |DataUseRecorder| instance. Applications must provide an override if they are interested in data use ascription. |ChromeDataUseAscriber| is the browser override that maps URL requests from the a page load to the same |DataUseRecorder|. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. BUG=607591 ========== to ========== Framework to ascribe all network data use to a source. |DataUseAscriber| is responsible for grouping |URLRequest| from the same source by mapping them to the same |DataUseRecorder| instance. Applications must provide an override if they are interested in data use ascription. |ChromeDataUseAscriber| is the browser override that maps URL requests from the a page load to the same |DataUseRecorder|. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. |ChromeDataUseAscriberService| is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to |ChromeDataUseAscriber|. BUG=607591 ==========
kundaji@chromium.org changed reviewers: + bengr@chromium.org
The CQ bit was checked by kundaji@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...
PTAL. This was the smallest cl that seemed to make logical sense. Even though it touches a fair number of files, this cl only sets up the ChromeDataUseAscriber to receive WebContents events.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kundaji@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 checked by kundaji@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 kundaji@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.
There's a fair amount of UI to IO posting. It would be nice if you could somehow avoid that. Otherwise the structure seems ok, provided there's not document ID that can be used instead. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:5: #include "chrome/browser/data_use_measurement/chrome_data_use_ascriber.h" Why is this in chrome? It's hard to tell from the stubs, but the only dependency might be content/public/browser. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:45: void RenderFrameCreated(int render_process_id, All of these public methods need comments. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:72: } // namespace data_use_measurement DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:40: void ChromeDataUseAscriberService::RenderFrameCreated( So rather than posting data use from every request to the UI thread, you post render frame events to the IO thread. Have you thought about plumbing data use info through Blink instead, on existing IPC? https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:44: if (!ascriber_) Why wouldn't the service have an ascriber? https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:151: void ChromeDataUseAscriberService::InitOnIOThread(IOThread* io_thread) { Obviously, you need tests for all of this. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:22: // Listens to navigation and frame events on UI thread and propagates them to on -> on the https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:63: void SetDataUseAscriber(ChromeDataUseAscriber* ascriber); I don't understand the relationship. Who owns the ascriber? Why are these methods duplicated there? https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:69: } // namespace data_use_measurement DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/data_use_web_contents_observer.cc (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/data_use_web_contents_observer.cc:23: if (!service || FromWebContents(web_contents)) { Remove curly braces. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/data_use_web_contents_observer.h (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:33: void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override; Comments needed here and below. https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... File components/data_use_measurement/core/data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:16: // Abstract class that manages instances of |DataUseRecorder| and maps -> maps a |URLRequest| instance to its ... https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:17: // |URLRequest| instances to their appropriate |DataUseRecorder|. Applications Applications -> An embedder https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:18: // should provide overrides if they are interested in tracking data usage overrides -> an override if they are -> if it is https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:19: // and surfacing it to the end user. Data use from all URLRequests mapped to the Why does it matter that it is surfaced to the end user. I'd drop this clause.
The CQ bit was checked by kundaji@chromium.org to run a CQ dry run
https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:5: #include "chrome/browser/data_use_measurement/chrome_data_use_ascriber.h" On 2016/09/01 00:16:55, bengr wrote: > Why is this in chrome? It's hard to tell from the stubs, but the only dependency > might be content/public/browser. What are the alternatives? In the component? Didn't think other embedders would want to compile in the Chrome implementation. Is there a better location? https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:45: void RenderFrameCreated(int render_process_id, On 2016/09/01 00:16:55, bengr wrote: > All of these public methods need comments. Done. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:72: } // namespace data_use_measurement On 2016/09/01 00:16:55, bengr wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:40: void ChromeDataUseAscriberService::RenderFrameCreated( On 2016/09/01 00:16:55, bengr wrote: > So rather than posting data use from every request to the UI thread, you post > render frame events to the IO thread. Have you thought about plumbing data use > info through Blink instead, on existing IPC? Yes, the design doc talk about these possibilities and why they were ruled out. We should send it out to a wider audience in case I am missing something. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:44: if (!ascriber_) On 2016/09/01 00:16:55, bengr wrote: > Why wouldn't the service have an ascriber? During startup, frame events might be called on the UI thread before ascriber_ is set on the IO thread. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:151: void ChromeDataUseAscriberService::InitOnIOThread(IOThread* io_thread) { On 2016/09/01 00:16:55, bengr wrote: > Obviously, you need tests for all of this. Done. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:22: // Listens to navigation and frame events on UI thread and propagates them to On 2016/09/01 00:16:55, bengr wrote: > on -> on the Done. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:63: void SetDataUseAscriber(ChromeDataUseAscriber* ascriber); On 2016/09/01 00:16:55, bengr wrote: > I don't understand the relationship. Who owns the ascriber? Why are these > methods duplicated there? This class listens to callbacks on the UI thread and propagates them to ChromeDataUseAscriber on the IO thread. Ascriber is owned in IOThread globals. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:69: } // namespace data_use_measurement On 2016/09/01 00:16:55, bengr wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/data_use_web_contents_observer.cc (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/data_use_web_contents_observer.cc:23: if (!service || FromWebContents(web_contents)) { On 2016/09/01 00:16:55, bengr wrote: > Remove curly braces. Done. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/data_use_web_contents_observer.h (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:33: void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override; On 2016/09/01 00:16:55, bengr wrote: > Comments needed here and below. Done. https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... File components/data_use_measurement/core/data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:16: // Abstract class that manages instances of |DataUseRecorder| and maps On 2016/09/01 00:16:55, bengr wrote: > -> maps a |URLRequest| instance to its ... > Done. https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:17: // |URLRequest| instances to their appropriate |DataUseRecorder|. Applications On 2016/09/01 00:16:55, bengr wrote: > Applications -> An embedder Done. https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:18: // should provide overrides if they are interested in tracking data usage On 2016/09/01 00:16:55, bengr wrote: > overrides -> an override > if they are -> if it is Done. https://codereview.chromium.org/2285903002/diff/80001/components/data_use_mea... components/data_use_measurement/core/data_use_ascriber.h:19: // and surfacing it to the end user. Data use from all URLRequests mapped to the On 2016/09/01 00:16:55, bengr wrote: > Why does it matter that it is surfaced to the end user. I'd drop this clause. Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm. https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2285903002/diff/80001/chrome/browser/data_use... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:5: #include "chrome/browser/data_use_measurement/chrome_data_use_ascriber.h" On 2016/09/07 23:38:37, kundaji wrote: > On 2016/09/01 00:16:55, bengr wrote: > > Why is this in chrome? It's hard to tell from the stubs, but the only > dependency > > might be content/public/browser. > > What are the alternatives? In the component? Didn't think other embedders would > want to compile in the Chrome implementation. Is there a better location? I think chrome is fine. Was wondering if it could go in content/public/browser. https://codereview.chromium.org/2285903002/diff/100001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/100001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:53: // Must be called on IO thread when a render frame host is deleted. on -> on the on all of these comments.
Description was changed from ========== Framework to ascribe all network data use to a source. |DataUseAscriber| is responsible for grouping |URLRequest| from the same source by mapping them to the same |DataUseRecorder| instance. Applications must provide an override if they are interested in data use ascription. |ChromeDataUseAscriber| is the browser override that maps URL requests from the a page load to the same |DataUseRecorder|. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. |ChromeDataUseAscriberService| is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to |ChromeDataUseAscriber|. BUG=607591 ========== to ========== Framework to ascribe all network data use to a source. |DataUseAscriber| is responsible for grouping |URLRequest| from the same source by mapping them to the same |DataUseRecorder| instance. Applications must provide an override if they are interested in data use ascription. |ChromeDataUseAscriber| is the browser override that maps URL requests from the a page load to the same |DataUseRecorder|. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. |ChromeDataUseAscriberService| is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to |ChromeDataUseAscriber|. BUG=645188 ==========
The CQ bit was checked by kundaji@chromium.org to run a CQ dry run
https://codereview.chromium.org/2285903002/diff/100001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/100001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:53: // Must be called on IO thread when a render frame host is deleted. On 2016/09/08 00:47:48, bengr wrote: > on -> on the > > on all of these comments. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kundaji@chromium.org changed reviewers: + thestig@chromium.org
thestig: chrome/browser/BUILD.gn chrome/browser/data_use_measurement/OWNERS chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber.h chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_browsertest.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.h chrome/browser/data_use_measurement/data_use_web_contents_observer.cc chrome/browser/data_use_measurement/data_use_web_contents_observer.h chrome/browser/io_thread.cc chrome/browser/io_thread.h chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc chrome/browser/ui/tab_helpers.cc bengr: chrome/browser/data_use_measurement/OWNERS chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber.h chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_browsertest.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.cc chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.h chrome/browser/data_use_measurement/data_use_web_contents_observer.cc chrome/browser/data_use_measurement/data_use_web_contents_observer.h components/data_use_measurement/core/BUILD.gn components/data_use_measurement/core/data_use_ascriber.h
On 2016/09/08 20:00:38, kundaji wrote: > chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc You can just say chrome/browser/data_use_measurement/ if it's all the files in that directory. It's rather cumbersome to list them all out explicitly. I'll take a peek but I defer to bengr.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:27: void ChromeDataUseAscriber::RenderFrameCreated(int render_process_id, I haven't looked very hard - why is the implementation empty? https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:10: #include "net/base/layered_network_delegate.h" What's this for? https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:18: namespace data_use_measurement { nit: blank line after when it's not fwrd decls. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:19: // Browser implementation of |DataUseAscriber|. Maintains a list of |variable_name|, Classes are written normally. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:26: // change, etc End sentences with periods. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:44: // Chrome override of |GetDataUseRecorder()|. This is not helpful. Just: // DataUseAscriber: [All the DataUseAscriber overrides go here] https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:47: // Must be called on the IO thread when a render frame host is created. You can probably omit the "Must be called on the IO thread" bit, since you already mentioned this class lives on the IO thread. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:60: void DidStartMainFrameNavigation(GURL gurl, Pass by const-ref, here and below? https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:78: DISALLOW_COPY_AND_ASSIGN(ChromeDataUseAscriber); DISALLOW_COPY_AND_ASSIGN() does no good if it's public. If you run "git cl lint" it would tell you this. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:30: content::BrowserThread::PostTask( You can also try PostTaskAndReplyWithResult(), then InitOnIOThread() can just be a function in an anonymous namespace that simply returns io_thread->globals()->data_use_ascriber.get(); https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:70: int parent_render_process_id, parent_render_frame_id = -1; - One decl per line. - Did |parent_render_process_id| get initialized? https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:93: if (navigation_handle->IsInMainFrame()) { I'd just do: if (!navigation_handle->IsInMainFrame()) return; and the PostTask gets 2 more columns to work with. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:99: navigation_handle->GetWebContents() Create a local variable for navigation_handle->GetWebContents(). https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:16: class IOThread; I'd put this before all the fwrd decls with namespaces. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_browsertest.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_browsertest.cc:19: class ChromeDataUseAscriberServiceTest : public InProcessBrowserTest { Do you plan to add more test cases? As is, it may be possible to write this as a cheaper unit test using TestBrowserThreadBundle and TestingBrowserProcess. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.h:9: #include "base/memory/singleton.h" Please read the top of singleton.h and try using a LazyInstance instead. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/data_use_web_contents_observer.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:33: // Called when a render frame host is created. Propagates this event to Just write a class summary to explain this class forwards events. Group all the overrides together. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:57: explicit DataUseWebContentsObserver(content::WebContents* web_contents, Multiple parameters - not explicit. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:59: friend class content::WebContentsUserData<DataUseWebContentsObserver>; Put the friend first and add a blank line before the member variables. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:60: ChromeDataUseAscriberService* service_; ChromeDataUseAscriberService* const service_; https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/io_thre... chrome/browser/io_thread.cc:506: globals_->data_use_ascriber.reset( data_use_ascriber = base::MakeUnique<data_use_measurement::ChromeDataUseAscriber>(); https://codereview.chromium.org/2285903002/diff/120001/components/data_use_me... File components/data_use_measurement/core/data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/120001/components/data_use_me... components/data_use_measurement/core/data_use_ascriber.h:17: // a |URLRequest| instance to its appropriate |DataUseRecorder|. An enbedder typo - embedder https://codereview.chromium.org/2285903002/diff/120001/components/data_use_me... components/data_use_measurement/core/data_use_ascriber.h:28: virtual DataUseRecorder* GetDataUseRecorder( To be clear, the Ascriber still owns the returned DataUseRecorder*, right?
Description was changed from ========== Framework to ascribe all network data use to a source. |DataUseAscriber| is responsible for grouping |URLRequest| from the same source by mapping them to the same |DataUseRecorder| instance. Applications must provide an override if they are interested in data use ascription. |ChromeDataUseAscriber| is the browser override that maps URL requests from the a page load to the same |DataUseRecorder|. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. |ChromeDataUseAscriberService| is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to |ChromeDataUseAscriber|. BUG=645188 ========== to ========== Framework to ascribe all network data use to a source. DataUseAscriber is responsible for grouping URLRequest from the same source by mapping them to the same DataUseRecorder instance. Applications must provide an override if they are interested in data use ascription. ChromeDataUseAscriber is the browser override that maps URL requests from the a page load to the same DataUseRecorder. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. ChromeDataUseAscriberService is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to ChromeDataUseAscriber. BUG=645188 ==========
The CQ bit was checked by kundaji@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...
Thanks for the review! PTAL. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.cc:27: void ChromeDataUseAscriber::RenderFrameCreated(int render_process_id, On 2016/09/08 22:04:42, Lei Zhang wrote: > I haven't looked very hard - why is the implementation empty? Will fill in implementation in subsequent cls. Left it out to keep this cl reviewable. Scope of this cl is to set up a way of propagating these calls from the UI thread to the IO thread here. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:10: #include "net/base/layered_network_delegate.h" On 2016/09/08 22:04:42, Lei Zhang wrote: > What's this for? Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:18: namespace data_use_measurement { On 2016/09/08 22:04:42, Lei Zhang wrote: > nit: blank line after when it's not fwrd decls. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:19: // Browser implementation of |DataUseAscriber|. Maintains a list of On 2016/09/08 22:04:42, Lei Zhang wrote: > |variable_name|, Classes are written normally. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:26: // change, etc On 2016/09/08 22:04:42, Lei Zhang wrote: > End sentences with periods. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:44: // Chrome override of |GetDataUseRecorder()|. On 2016/09/08 22:04:42, Lei Zhang wrote: > This is not helpful. Just: > > // DataUseAscriber: > [All the DataUseAscriber overrides go here] Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:47: // Must be called on the IO thread when a render frame host is created. On 2016/09/08 22:04:42, Lei Zhang wrote: > You can probably omit the "Must be called on the IO thread" bit, since you > already mentioned this class lives on the IO thread. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:60: void DidStartMainFrameNavigation(GURL gurl, On 2016/09/08 22:04:42, Lei Zhang wrote: > Pass by const-ref, here and below? Comes from a different thread, so cannot use ref. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber.h:78: DISALLOW_COPY_AND_ASSIGN(ChromeDataUseAscriber); On 2016/09/08 22:04:42, Lei Zhang wrote: > DISALLOW_COPY_AND_ASSIGN() does no good if it's public. If you run "git cl lint" > it would tell you this. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:30: content::BrowserThread::PostTask( On 2016/09/08 22:04:43, Lei Zhang wrote: > You can also try PostTaskAndReplyWithResult(), then InitOnIOThread() can just be > a function in an anonymous namespace that simply returns > io_thread->globals()->data_use_ascriber.get(); Good idea. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:70: int parent_render_process_id, parent_render_frame_id = -1; On 2016/09/08 22:04:43, Lei Zhang wrote: > - One decl per line. > - Did |parent_render_process_id| get initialized? Done. Nope, |parent_render_process_id| will not be initialized as written earlier. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:93: if (navigation_handle->IsInMainFrame()) { On 2016/09/08 22:04:43, Lei Zhang wrote: > I'd just do: > > if (!navigation_handle->IsInMainFrame()) > return; > > and the PostTask gets 2 more columns to work with. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:99: navigation_handle->GetWebContents() On 2016/09/08 22:04:43, Lei Zhang wrote: > Create a local variable for navigation_handle->GetWebContents(). Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.h:16: class IOThread; On 2016/09/08 22:04:43, Lei Zhang wrote: > I'd put this before all the fwrd decls with namespaces. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_browsertest.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_browsertest.cc:19: class ChromeDataUseAscriberServiceTest : public InProcessBrowserTest { On 2016/09/08 22:04:43, Lei Zhang wrote: > Do you plan to add more test cases? > > As is, it may be possible to write this as a cheaper unit test using > TestBrowserThreadBundle and TestingBrowserProcess. Done. It was a little awkward to instantiate IOThread in a unittest, so I had gone with browsertest. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_factory.h:9: #include "base/memory/singleton.h" On 2016/09/08 22:04:43, Lei Zhang wrote: > Please read the top of singleton.h and try using a LazyInstance instead. Don't have a choice in this case unfortunately. This class derives from BrowserContextKeyedServiceFactory and must be a singleton as specified in browser_context_keyed_service_factory.h. On the bright side, this is a very popular pattern so should be safe: https://cs.chromium.org/search/?q=Singleton+f:service_factory.h&p=1&sq=packag... https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... File chrome/browser/data_use_measurement/data_use_web_contents_observer.h (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:33: // Called when a render frame host is created. Propagates this event to On 2016/09/08 22:04:43, Lei Zhang wrote: > Just write a class summary to explain this class forwards events. Group all the > overrides together. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:57: explicit DataUseWebContentsObserver(content::WebContents* web_contents, On 2016/09/08 22:04:43, Lei Zhang wrote: > Multiple parameters - not explicit. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:59: friend class content::WebContentsUserData<DataUseWebContentsObserver>; On 2016/09/08 22:04:43, Lei Zhang wrote: > Put the friend first and add a blank line before the member variables. Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/data_us... chrome/browser/data_use_measurement/data_use_web_contents_observer.h:60: ChromeDataUseAscriberService* service_; On 2016/09/08 22:04:43, Lei Zhang wrote: > ChromeDataUseAscriberService* const service_; Done. https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2285903002/diff/120001/chrome/browser/io_thre... chrome/browser/io_thread.cc:506: globals_->data_use_ascriber.reset( On 2016/09/08 22:04:43, Lei Zhang wrote: > data_use_ascriber = > base::MakeUnique<data_use_measurement::ChromeDataUseAscriber>(); Done. https://codereview.chromium.org/2285903002/diff/120001/components/data_use_me... File components/data_use_measurement/core/data_use_ascriber.h (right): https://codereview.chromium.org/2285903002/diff/120001/components/data_use_me... components/data_use_measurement/core/data_use_ascriber.h:17: // a |URLRequest| instance to its appropriate |DataUseRecorder|. An enbedder On 2016/09/08 22:04:43, Lei Zhang wrote: > typo - embedder Done. https://codereview.chromium.org/2285903002/diff/120001/components/data_use_me... components/data_use_measurement/core/data_use_ascriber.h:28: virtual DataUseRecorder* GetDataUseRecorder( On 2016/09/08 22:04:43, Lei Zhang wrote: > To be clear, the Ascriber still owns the returned DataUseRecorder*, right? Yes, that's why return type is DataUseRecorder* rather than unique_ptr.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with some more nits. https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc (right): https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:115: navigation_handle->GetWebContents()->GetMainFrame()->GetRoutingID(), Use |web_contents| here too. https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:131: navigation_handle->GetWebContents() Same pattern as above. https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:152: navigation_handle->GetWebContents() One more... https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_unittest.cc (right): https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_unittest.cc:24: prefs_.reset(new TestingPrefServiceSimple()); Can you use base::MakeUnique() here and below? https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_unittest.cc:38: TestingBrowserProcess::GetGlobal()->SetLocalState(NULL); s/NULL/nullptr/
https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc (right): https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:115: navigation_handle->GetWebContents()->GetMainFrame()->GetRoutingID(), On 2016/09/09 20:58:42, Lei Zhang wrote: > Use |web_contents| here too. Done. https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:131: navigation_handle->GetWebContents() On 2016/09/09 20:58:42, Lei Zhang wrote: > Same pattern as above. Done. https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service.cc:152: navigation_handle->GetWebContents() On 2016/09/09 20:58:42, Lei Zhang wrote: > One more... Done. https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... File chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_unittest.cc (right): https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_unittest.cc:24: prefs_.reset(new TestingPrefServiceSimple()); On 2016/09/09 20:58:42, Lei Zhang wrote: > Can you use base::MakeUnique() here and below? Done. https://codereview.chromium.org/2285903002/diff/140001/chrome/browser/data_us... chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_unittest.cc:38: TestingBrowserProcess::GetGlobal()->SetLocalState(NULL); On 2016/09/09 20:58:42, Lei Zhang wrote: > s/NULL/nullptr/ Done.
The CQ bit was checked by kundaji@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2285903002/#ps160001 (title: "Addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Framework to ascribe all network data use to a source. DataUseAscriber is responsible for grouping URLRequest from the same source by mapping them to the same DataUseRecorder instance. Applications must provide an override if they are interested in data use ascription. ChromeDataUseAscriber is the browser override that maps URL requests from the a page load to the same DataUseRecorder. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. ChromeDataUseAscriberService is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to ChromeDataUseAscriber. BUG=645188 ========== to ========== Framework to ascribe all network data use to a source. DataUseAscriber is responsible for grouping URLRequest from the same source by mapping them to the same DataUseRecorder instance. Applications must provide an override if they are interested in data use ascription. ChromeDataUseAscriber is the browser override that maps URL requests from the a page load to the same DataUseRecorder. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. ChromeDataUseAscriberService is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to ChromeDataUseAscriber. BUG=645188 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Framework to ascribe all network data use to a source. DataUseAscriber is responsible for grouping URLRequest from the same source by mapping them to the same DataUseRecorder instance. Applications must provide an override if they are interested in data use ascription. ChromeDataUseAscriber is the browser override that maps URL requests from the a page load to the same DataUseRecorder. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. ChromeDataUseAscriberService is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to ChromeDataUseAscriber. BUG=645188 ========== to ========== Framework to ascribe all network data use to a source. DataUseAscriber is responsible for grouping URLRequest from the same source by mapping them to the same DataUseRecorder instance. Applications must provide an override if they are interested in data use ascription. ChromeDataUseAscriber is the browser override that maps URL requests from the a page load to the same DataUseRecorder. It maintains navigation and frame information on the IO thread to perform this many-to-one mapping. ChromeDataUseAscriberService is the UI thread counterpart that observes navigation and frame events on the UI thread and forwards the calls to ChromeDataUseAscriber. BUG=645188 Committed: https://crrev.com/f617523bd19d6e884986664ec8f9bc045353b702 Cr-Commit-Position: refs/heads/master@{#417740} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f617523bd19d6e884986664ec8f9bc045353b702 Cr-Commit-Position: refs/heads/master@{#417740}
Message was sent while issue was closed.
Patchset #10 (id:180001) has been deleted |
