Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1443683002:
Notify DataUseTabModel of navigations and tab closures (Closed)
DescriptionNotify DataUseTabModel of navigations and tab closures.
package name will be later plumbed through a separate function call.
BUG=552131, 552133
Committed: https://crrev.com/224e4f0ba66c17fe3776976f476b87fdb85adaf3
Cr-Commit-Position: refs/heads/master@{#362582}
Patch Set 1 : patch #
Total comments: 16
Patch Set 2 : Addressed comments, Using two weak ptrs now. #
Total comments: 30
Patch Set 3 : Addressed sclittle comments #
Total comments: 59
Patch Set 4 : Rebased, addressed comments #
Total comments: 32
Patch Set 5 : Addressed comments #
Total comments: 7
Patch Set 6 : Using map of weak pointers #Patch Set 7 : #
Total comments: 14
Patch Set 8 : Rebased #
Total comments: 26
Patch Set 9 : Addressed bengr comments #
Total comments: 22
Patch Set 10 : Addressed sclittle comments #
Total comments: 14
Patch Set 11 : Addressed sclittle comments #
Total comments: 14
Patch Set 12 : Forward declare GURL #Patch Set 13 : Addressed comments #
Total comments: 28
Patch Set 14 : Addressed comments #Patch Set 15 : Rebased #
Total comments: 8
Patch Set 16 : Addressed comments #
Total comments: 4
Patch Set 17 : Addressed mmenke comments, moved out of profile impl #
Total comments: 4
Patch Set 18 : Addressed mmenke comments #Patch Set 19 : Remove unnecessary thread checks in the factory class #Messages
Total messages: 79 (32 generated)
Description was changed from ========== forma Convert transition type working whitespace patched bengr patvh fixed some errors include order oreder commit before switch c commit commit before switch commit before rebase commit commit before switch lot of stuff removed passing passing, going to dleete more stuff Working rebased remove useless files working whitespace works w Passing passing passing passing passing passing passing passing passing passing passing probably passing order comments aDDED tests Removed io thread passing More rename working renamed passing include include order endif comments Addressed bengr comment comment rebase Rebased t rebase commit before switch switching passing working whitespace working format format Added files BUG= ========== to ========== Convert Transition Type BUG=552131, 552133 ==========
Description was changed from ========== Convert Transition Type BUG=552131, 552133 ========== to ========== Notify DataUseTabModel of navigations and tab closures. BUG=552131, 552133 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Notify DataUseTabModel of navigations and tab closures. BUG=552131, 552133 ========== to ========== Notify DataUseTabModel of navigations and tab closures. package name will be later plumbed through a separate function call. BUG=552131, 552133 ==========
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: PTAL. Thanks.
bengr: PTAL. Thanks.
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: PTAL at profile_impl_io_data. Thanks.
https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:34: if (data_use_tab_model_) { Is this legal? You're checking if a weak ptr is NULL on a thread other than the one it lives on. Seems like at the very least, you could trigger a thread sanitizer warning doing this. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:237: #if defined(OS_ANDROID) I don't know what this stuff is for, but I assume you only want to do it for non-incognito profiles? https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:240: Profile::FromBrowserContext(profile_)); Remove the "Profile::FromBrowserContext"?
https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:34: if (data_use_tab_model_) { On 2015/11/13 22:49:55, mmenke (OOO Nov 11 - Nov 15) wrote: > Is this legal? You're checking if a weak ptr is NULL on a thread other than the > one it lives on. Seems like at the very least, you could trigger a thread > sanitizer warning doing this. This doesn't seem kosher. I think it should just post to the method on the weak pointer. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:49: if (data_use_tab_model_) { Same here. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:135: case ui::PAGE_TRANSITION_AUTO_BOOKMARK: Where do we track navigation from history? Also, is the an exhaustive list? Seems thin. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.h:66: data_use_tab_model_ = data_use_tab_model_; bug? You are setting it to itself? https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:237: #if defined(OS_ANDROID) On 2015/11/13 22:49:55, mmenke (OOO Nov 11 - Nov 15) wrote: > I don't know what this stuff is for, but I assume you only want to do it for > non-incognito profiles? Yes, should only be for non-incognito.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
ptal. thanks. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:34: if (data_use_tab_model_) { On 2015/11/14 03:54:39, bengr wrote: > On 2015/11/13 22:49:55, mmenke (OOO Nov 11 - Nov 15) wrote: > > Is this legal? You're checking if a weak ptr is NULL on a thread other than > the > > one it lives on. Seems like at the very least, you could trigger a thread > > sanitizer warning doing this. > > This doesn't seem kosher. I think it should just post to the method on the weak > pointer. Done. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:34: if (data_use_tab_model_) { On 2015/11/13 22:49:55, mmenke (OOO Nov 11 - Nov 15) wrote: > Is this legal? You're checking if a weak ptr is NULL on a thread other than the > one it lives on. Seems like at the very least, you could trigger a thread > sanitizer warning doing this. Now using two weak pointers (one for UI, one for IO), and dereferencing them on the correct threads. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:49: if (data_use_tab_model_) { On 2015/11/14 03:54:39, bengr wrote: > Same here. Done. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:135: case ui::PAGE_TRANSITION_AUTO_BOOKMARK: On 2015/11/14 03:54:39, bengr wrote: > Where do we track navigation from history? Also, is the an exhaustive list? > Seems thin. List should be exhaustive now. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.h:66: data_use_tab_model_ = data_use_tab_model_; On 2015/11/14 03:54:39, bengr wrote: > bug? You are setting it to itself? Done. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:237: #if defined(OS_ANDROID) On 2015/11/13 22:49:55, mmenke (OOO Nov 11 - Nov 15) wrote: > I don't know what this stuff is for, but I assume you only want to do it for > non-incognito profiles? Done. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:237: #if defined(OS_ANDROID) On 2015/11/13 22:49:55, mmenke (OOO Nov 11 - Nov 15) wrote: > I don't know what this stuff is for, but I assume you only want to do it for > non-incognito profiles? Done. https://codereview.chromium.org/1443683002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:240: Profile::FromBrowserContext(profile_)); On 2015/11/13 22:49:55, mmenke (OOO Nov 11 - Nov 15) wrote: > Remove the "Profile::FromBrowserContext"? Done.
tbansal@chromium.org changed reviewers: + sclittle@chromium.org - bengr@chromium.org
sclittle: ptal (changing because bengr@ is out). thanks!
tbansal@chromium.org changed reviewers: + rajendrant@chromium.org
https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:106: // |transition_type| must not be null. nits: rename transition_type to output_transition_type to be more clear.
On 2015/11/16 22:00:09, Raj wrote: > https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... > File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): > > https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... > chrome/browser/android/data_usage/data_use_ui_tab_model.h:106: // > |transition_type| must not be null. > nits: > rename transition_type to output_transition_type to be more clear. lgtm
https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_helper.cc (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:40: SessionTabHelper::IdForTab(web_contents())); SessionTabHelper::IdForTab could return -1 if web_contents() isn't actually a tab. Is there a guarantee that web_contents() is definitely a tab? If it's not guaranteed, could you handle the -1 case here? https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:60: SessionTabHelper::IdForTab(web_contents())); Same here about handling -1 case if web_contents() isn't absolutely guaranteed to be a tab. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:84: base::WeakPtr<DataUseTabModel> GetUIWeakPtr(); WeakPtrs aren't safe to be used on a different thread than the one that the object lives on. See my comment on the WeakPtrFactories below. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:166: base::WeakPtrFactory<DataUseTabModel> ui_weak_factory_; Having WeakPtrFactories for different threads isn't safe, since WeakPtrFactories aren't thread safe. E.g., the DataUseTabModel could be destroyed on the IO thread while some UI thread code is in the process of using the DataUseTabModel, even though the WeakPtr was valid earlier. The simplest solution is usually to have an object live on only one thread, and always PostTask to that thread when dealing with that object. One way would be to split up the IO and UI functionality of this class into different classes. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:20: void OnNavigationEventOnIOThread( Can these functions be moved to an anonymous namespace? https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:72: base::WeakPtr<DataUseTabModel> ui_data_use_tab_model); I don't understand why the DataUseTabModel needs to be used on the UI thread. Shouldn't all the UI thread logic be part of the DataUseUITabModel instead? https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:115: base::WeakPtr<DataUseTabModel> ui_data_use_tab_model_; It doesn't make sense to have a WeakPtr tied to a different thread than the DataUseTabModel, since the DataUseTabModel will always be destroyed on the IO thread. See my other comments about WeakPtr thread un-safety. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:32: class DataUseTabModelTest : public DataUseTabModel { nit: Name this TestDataUseTabModel, so that it isn't confused with a test harness class. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:69: scoped_ptr<data_usage::DataUseAggregator> data_use_aggregator( nit: This doesn't have to be a scoped_ptr, this could just be a regular object on the stack. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:74: scoped_ptr<ExternalDataUseObserver> external_data_use_observer( nit: same here, this doesn't need to be a scoped_ptr. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:128: data_usage::DataUse data_use(GURL("http://foo.com/#q=abc"), Add an include for data_use.h https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:131: net::NetworkChangeNotifier::CONNECTION_UNKNOWN, nit: Pull this DataUse creation and the DataUse creation below out into a function, so that it's easier for someone to add a new argument to the DataUse constructor later. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:143: GURL("http://foo.com/#q=abc"), base::TimeTicks::Now(), base::TimeTicks::Now() could be the same as it was earlier, so this test seems inherently flaky. If you really want to test timing stuff, you could either use specific TimeTicks values for each of these and the ReportTabClosure event. If you can't use specific TimeTicks values, maybe TickClock and SimpleTestTickClock could be helpful: https://code.google.com/p/chromium/codesearch#chromium/src/base/time/tick_clo... https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:94: DataUseTabModel* data_use_tab_model() const { Why does this need to be exposed here?
Patchset #3 (id:120001) has been deleted
ptal. thanks. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_helper.cc (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:40: SessionTabHelper::IdForTab(web_contents())); On 2015/11/16 23:13:42, sclittle wrote: > SessionTabHelper::IdForTab could return -1 if web_contents() isn't actually a > tab. Is there a guarantee that web_contents() is definitely a tab? > > If it's not guaranteed, could you handle the -1 case here? Handled it in DataUseUITabModel::ReportBrowserNavigation() and DataUseUITabModel::ReportTabClosure. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:60: SessionTabHelper::IdForTab(web_contents())); On 2015/11/16 23:13:42, sclittle wrote: > Same here about handling -1 case if web_contents() isn't absolutely guaranteed > to be a tab. Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:84: base::WeakPtr<DataUseTabModel> GetUIWeakPtr(); On 2015/11/16 23:13:42, sclittle wrote: > WeakPtrs aren't safe to be used on a different thread than the one that the > object lives on. See my comment on the WeakPtrFactories below. Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:166: base::WeakPtrFactory<DataUseTabModel> ui_weak_factory_; On 2015/11/16 23:13:42, sclittle wrote: > Having WeakPtrFactories for different threads isn't safe, since WeakPtrFactories > aren't thread safe. > > E.g., the DataUseTabModel could be destroyed on the IO thread while some UI > thread code is in the process of using the DataUseTabModel, even though the > WeakPtr was valid earlier. > > The simplest solution is usually to have an object live on only one thread, and > always PostTask to that thread when dealing with that object. One way would be > to split up the IO and UI functionality of this class into different classes. Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:20: void OnNavigationEventOnIOThread( On 2015/11/16 23:13:42, sclittle wrote: > Can these functions be moved to an anonymous namespace? Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:72: base::WeakPtr<DataUseTabModel> ui_data_use_tab_model); On 2015/11/16 23:13:42, sclittle wrote: > I don't understand why the DataUseTabModel needs to be used on the UI thread. > Shouldn't all the UI thread logic be part of the DataUseUITabModel instead? Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:106: // |transition_type| must not be null. On 2015/11/16 22:00:09, Raj wrote: > nits: > rename transition_type to output_transition_type to be more clear. I think |transition_type| is okay, since it is a pointer and not getting passed by a const ref (so it is clear that function may modify it). Also, the function name makes it clear that something is getting converted. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:115: base::WeakPtr<DataUseTabModel> ui_data_use_tab_model_; On 2015/11/16 23:13:42, sclittle wrote: > It doesn't make sense to have a WeakPtr tied to a different thread than the > DataUseTabModel, since the DataUseTabModel will always be destroyed on the IO > thread. See my other comments about WeakPtr thread un-safety. Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:32: class DataUseTabModelTest : public DataUseTabModel { On 2015/11/16 23:13:43, sclittle wrote: > nit: Name this TestDataUseTabModel, so that it isn't confused with a test > harness class. Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:69: scoped_ptr<data_usage::DataUseAggregator> data_use_aggregator( On 2015/11/16 23:13:42, sclittle wrote: > nit: This doesn't have to be a scoped_ptr, this could just be a regular object > on the stack. Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:74: scoped_ptr<ExternalDataUseObserver> external_data_use_observer( On 2015/11/16 23:13:43, sclittle wrote: > nit: same here, this doesn't need to be a scoped_ptr. Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:128: data_usage::DataUse data_use(GURL("http://foo.com/#q=abc"), On 2015/11/16 23:13:42, sclittle wrote: > Add an include for data_use.h Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:131: net::NetworkChangeNotifier::CONNECTION_UNKNOWN, On 2015/11/16 23:13:42, sclittle wrote: > nit: Pull this DataUse creation and the DataUse creation below out into a > function, so that it's easier for someone to add a new argument to the DataUse > constructor later. Reduced it to one function call. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:143: GURL("http://foo.com/#q=abc"), base::TimeTicks::Now(), On 2015/11/16 23:13:43, sclittle wrote: > base::TimeTicks::Now() could be the same as it was earlier, so this test seems > inherently flaky. If you really want to test timing stuff, you could either use > specific TimeTicks values for each of these and the ReportTabClosure event. > > If you can't use specific TimeTicks values, maybe TickClock and > SimpleTestTickClock could be helpful: > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/tick_clo... Done. https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/100001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:94: DataUseTabModel* data_use_tab_model() const { On 2015/11/16 23:13:43, sclittle wrote: > Why does this need to be exposed here? It is called in io_thread.cc
Not going to do a full review of the code I don't own, but a couple comments on it. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:43: base::WeakPtr<chrome::android::DataUseUITabModel> | data_use_ui_tab_model | |? https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:85: class TabObserverOnIOThread This doesn't need to be public, does it? In fact, you can just forward declare it as a private class in this file. The methods in the anonymous namespace that much with this can be made into static methods of DataUseUITabModel. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... chrome/browser/io_thread.h:271: chrome::android::ExternalDataUseObserver* external_data_use_observer() const { nit: Both of these methods should be indented 2. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... chrome/browser/io_thread.h:273: } nit: Blank line between methods https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... chrome/browser/io_thread.h:274: base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model() const { Hrm...actually, do we even need a weak pointer here? The IOThread class is called the IOThread for a reason. Even ignoring that, as long as there's a profile on the UI thread, the corresponding stuff will still exist on the IOThread when a task is posted there. Seems like we could just get rid of the accessor entirely, and have the consumer get the data_use_tab_model from the IOThread's external_data_use_observer directly. More generally, exposing weak pointers widely obscures lifetimes, and is generally considered a bit of an anti-pattern.
https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_helper.cc (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:40: SessionTabHelper::IdForTab(web_contents())); For clarity, is it possible to just check at the source here for -1 tab IDs, and everywhere else that calls SessionTabHelper::IdForTab? Then, everywhere else that takes in a tab ID from those could just DCHECK_GE(tab_id, 0) or something. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:153: const scoped_refptr<base::ObserverListThreadSafe<TabDataUseObserver>> nit: If you're certain that all observers will be on the IO thread, then it's faster to use a regular ObserverList than an ObserverListThreadSafe. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:162: // DataUseTabUIManager on the UI thread. nit: What do you mean, passing the WeakPtrs to the UI thread? It might just be clearer to remove this comment altogether. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:18: base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model, For readability, you could move this anonymous namespace inside of namespace chrome { namespace android { so that you don't have to prefix everything with chrome::android:: here and below. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:44: , I don't think this compiles, please fix this. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:109: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) Instead of taking in a ui_task_runner, you could just use base::ThreadTaskRunnerHandle::Get(): https://code.google.com/p/chromium/codesearch#chromium/src/base/thread_task_r... https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:116: io_task_runner_->PostTask( It might just be simpler to have CreateTabObserverOnIOThread return the scoped_refptr<TabObserverOnIOThread>, and then here just call: base::PostTaskAndReplyWithResult( io_task_runner_.get(), FROM_HERE, base::Bind(&CreateTabObserverOnIOThread, GetWeakPtr()), base::Bind(&DataUseUITabModel::SetTabDataUseObserverOnUIThread, GetWeakPtr())); See https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner_u... That way, you could make DataUseUITabModel::SetTabDataUseObserverOnUIThread private, and you wouldn't have to take in a |ui_task_runner| in this constructor. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:126: // Removes the last refptr to |tab_observer|, so that it is destroyed on IO nit: Say "Removes the last reference" instead of "Removes the last refptr" https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:140: if (tab_id < 0) For clarity, test for the -1 tab ID at the point where it's generated instead of here, and just DCHECK here that it's a valid tab ID. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:146: io_task_runner_->PostTask( PostTask already has built-in support for cancelling callbacks for methods on WeakPtrs if the WeakPtr is no longer valid, so you should just PostTask to the DataUseTabModel method directly. You can remove the anonymous function that you added to do this. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:160: io_task_runner_->PostTask( Same here, just PostTask directly on the DataUseTabModel method directly, since PostTask will automatically cancel the task if the WeakPtr is invalid. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:187: FROM_HERE, base::Bind(&SetDataUseTabModelForTabDataUseObserver, Just post a task to create the observer here with the WeakPtrs, instead of creating the observer in the constructor. It would simplify the lifecycle of the Observer. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:214: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); nit: Just use the thread_checker everywhere, since AIUI this object is now only ever used on the UI thread. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:238: DCHECK(thread_checker_.CalledOnValidThread()); Should you also DCHECK that this method is only ever called once? https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:239: tab_data_use_observer_.swap(tab_data_use_observer); Why swap? Why not just assign it? The previous value must be NULL or else you'd have to release it properly on the IO thread. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:331: return; This is weird. It looks like this Observer supports changing the DataUseTabModel while it's in use, but it never actually removes itself as an observer anywhere. I'd suggest redesigning the Observer to take in both WeakPtrs on construction and add itself as an observer immediately. You could get rid of the |registered_as_observer_| variable as well. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:335: data_use_tab_model_->AddObserver(this); You should just DCHECK(data_use_tab_model_) when you get it, since there's no point in having an Observer that doesn't observe anything. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:342: ui_task_runner_->PostTask(FROM_HERE, PostTask already has built-in support for cancelling callbacks for methods on WeakPtrs if the WeakPtr is no longer valid. You can get rid of the anonymous functions for this and the other call below and just post the method directly on the WeakPtr. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:73: void SetIODataUseTabModel(base::WeakPtr<DataUseTabModel> data_use_tab_model); This should be private. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:85: class TabObserverOnIOThread Does this inner class definition need to be in the header file? Could you just forward declare it here, and move all this class definition code to the .cc file? Also, this should be private, and no tests should have to access the Observer directly. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:36: scoped_refptr<base::SingleThreadTaskRunner> task_runner_) What thread is |task_runner_| for? Name it |io_task_runner| or whatever. Also, remove the trailing "_" https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:99: data_use_ui_tab_model()->tab_data_use_observer_->NotifyTrackingStarting( Instead of reaching into the |tab_data_use_observer_| directly, couldn't you just make a TestDataUseTabModel notify its observers? That way, you actually test that the |tab_data_use_observer_| registers itself as an observer properly. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:115: FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, ReportTabEventsTest); Why is this necessary? Can we please not make this necessary? If many tests might want to call RegisterURLRegexes, then you could add a public RegisterURLRegexesForTesting method, which is checked by presubmits to ensure that it's only called by test code. At the very least, please add a TODO for this and alphabetize this list.
https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:318: base::WeakPtrFactory<ExternalDataUseObserver> io_weak_factory_; For a future CL: WeakPtrs aren't safe to be invalidated or dereferenced on different threads than the thread that the object will be destroyed on. Please fix this.
ptal. thanks. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_helper.cc (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:40: SessionTabHelper::IdForTab(web_contents())); On 2015/11/17 22:50:11, sclittle wrote: > For clarity, is it possible to just check at the source here for -1 tab IDs, and > everywhere else that calls SessionTabHelper::IdForTab? Then, everywhere else > that takes in a tab ID from those could just DCHECK_GE(tab_id, 0) or something. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:153: const scoped_refptr<base::ObserverListThreadSafe<TabDataUseObserver>> On 2015/11/17 22:50:11, sclittle wrote: > nit: If you're certain that all observers will be on the IO thread, then it's > faster to use a regular ObserverList than an ObserverListThreadSafe. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:162: // DataUseTabUIManager on the UI thread. On 2015/11/17 22:50:11, sclittle wrote: > nit: What do you mean, passing the WeakPtrs to the UI thread? It might just be > clearer to remove this comment altogether. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:18: base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model, On 2015/11/17 22:50:11, sclittle wrote: > For readability, you could move this anonymous namespace inside of > > namespace chrome { > namespace android { > > so that you don't have to prefix everything with chrome::android:: here and > below. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:43: base::WeakPtr<chrome::android::DataUseUITabModel> | data_use_ui_tab_model | On 2015/11/17 22:48:40, mmenke wrote: > |? Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:44: , On 2015/11/17 22:50:11, sclittle wrote: > I don't think this compiles, please fix this. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:109: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) On 2015/11/17 22:50:11, sclittle wrote: > Instead of taking in a ui_task_runner, you could just use > base::ThreadTaskRunnerHandle::Get(): > https://code.google.com/p/chromium/codesearch#chromium/src/base/thread_task_r... Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:116: io_task_runner_->PostTask( On 2015/11/17 22:50:11, sclittle wrote: > It might just be simpler to have CreateTabObserverOnIOThread return the > scoped_refptr<TabObserverOnIOThread>, and then here just call: > > base::PostTaskAndReplyWithResult( > io_task_runner_.get(), FROM_HERE, > base::Bind(&CreateTabObserverOnIOThread, GetWeakPtr()), > base::Bind(&DataUseUITabModel::SetTabDataUseObserverOnUIThread, > GetWeakPtr())); > > See > https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner_u... > > That way, you could make DataUseUITabModel::SetTabDataUseObserverOnUIThread > private, and you wouldn't have to take in a |ui_task_runner| in this > constructor. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:126: // Removes the last refptr to |tab_observer|, so that it is destroyed on IO On 2015/11/17 22:50:11, sclittle wrote: > nit: Say "Removes the last reference" instead of "Removes the last refptr" Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:140: if (tab_id < 0) On 2015/11/17 22:50:11, sclittle wrote: > For clarity, test for the -1 tab ID at the point where it's generated instead of > here, and just DCHECK here that it's a valid tab ID. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:146: io_task_runner_->PostTask( On 2015/11/17 22:50:11, sclittle wrote: > PostTask already has built-in support for cancelling callbacks for methods on > WeakPtrs if the WeakPtr is no longer valid, so you should just PostTask to the > DataUseTabModel method directly. > > You can remove the anonymous function that you added to do this. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:160: io_task_runner_->PostTask( On 2015/11/17 22:50:11, sclittle wrote: > Same here, just PostTask directly on the DataUseTabModel method directly, since > PostTask will automatically cancel the task if the WeakPtr is invalid. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:187: FROM_HERE, base::Bind(&SetDataUseTabModelForTabDataUseObserver, On 2015/11/17 22:50:11, sclittle wrote: > Just post a task to create the observer here with the WeakPtrs, instead of > creating the observer in the constructor. It would simplify the lifecycle of the > Observer. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:214: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2015/11/17 22:50:11, sclittle wrote: > nit: Just use the thread_checker everywhere, since AIUI this object is now only > ever used on the UI thread. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:238: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/11/17 22:50:11, sclittle wrote: > Should you also DCHECK that this method is only ever called once? Added DCHECK https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:239: tab_data_use_observer_.swap(tab_data_use_observer); On 2015/11/17 22:50:11, sclittle wrote: > Why swap? Why not just assign it? The previous value must be NULL or else you'd > have to release it properly on the IO thread. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:331: return; On 2015/11/17 22:50:11, sclittle wrote: > This is weird. It looks like this Observer supports changing the DataUseTabModel > while it's in use, but it never actually removes itself as an observer anywhere. > > I'd suggest redesigning the Observer to take in both WeakPtrs on construction > and add itself as an observer immediately. You could get rid of the > |registered_as_observer_| variable as well. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:335: data_use_tab_model_->AddObserver(this); On 2015/11/17 22:50:11, sclittle wrote: > You should just DCHECK(data_use_tab_model_) when you get it, since there's no > point in having an Observer that doesn't observe anything. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:342: ui_task_runner_->PostTask(FROM_HERE, On 2015/11/17 22:50:11, sclittle wrote: > PostTask already has built-in support for cancelling callbacks for methods on > WeakPtrs if the WeakPtr is no longer valid. > > You can get rid of the anonymous functions for this and the other call below and > just post the method directly on the WeakPtr. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:73: void SetIODataUseTabModel(base::WeakPtr<DataUseTabModel> data_use_tab_model); On 2015/11/17 22:50:11, sclittle wrote: > This should be private. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:85: class TabObserverOnIOThread On 2015/11/17 22:50:12, sclittle wrote: > Does this inner class definition need to be in the header file? Could you just > forward declare it here, and move all this class definition code to the .cc > file? > > Also, this should be private, and no tests should have to access the Observer > directly. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:85: class TabObserverOnIOThread On 2015/11/17 22:48:40, mmenke wrote: > This doesn't need to be public, does it? In fact, you can just forward declare > it as a private class in this file. > > The methods in the anonymous namespace that much with this can be made into > static methods of DataUseUITabModel. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:36: scoped_refptr<base::SingleThreadTaskRunner> task_runner_) On 2015/11/17 22:50:12, sclittle wrote: > What thread is |task_runner_| for? Name it |io_task_runner| or whatever. > > Also, remove the trailing "_" Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:99: data_use_ui_tab_model()->tab_data_use_observer_->NotifyTrackingStarting( On 2015/11/17 22:50:12, sclittle wrote: > Instead of reaching into the |tab_data_use_observer_| directly, couldn't you > just make a TestDataUseTabModel notify its observers? That way, you actually > test that the |tab_data_use_observer_| registers itself as an observer properly. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:115: FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, ReportTabEventsTest); On 2015/11/17 22:50:12, sclittle wrote: > Why is this necessary? Can we please not make this necessary? The public function requires Java object and Java environment setup. > > If many tests might want to call RegisterURLRegexes, then you could add a public > RegisterURLRegexesForTesting method, which is checked by presubmits to ensure > that it's only called by test code. I thought FRIEND_TEST was slightly better because it avoid adding methods that are called only by tests (which may increase the size of binary). I am probably wrong? > > At the very least, please add a TODO for this and alphabetize this list. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:318: base::WeakPtrFactory<ExternalDataUseObserver> io_weak_factory_; On 2015/11/17 23:13:46, sclittle wrote: > For a future CL: WeakPtrs aren't safe to be invalidated or dereferenced on > different threads than the thread that the object will be destroyed on. Please > fix this. Acknowledged. Yes, currently doing that in a separate CL. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... chrome/browser/io_thread.h:271: chrome::android::ExternalDataUseObserver* external_data_use_observer() const { On 2015/11/17 22:48:40, mmenke wrote: > nit: Both of these methods should be indented 2. Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... chrome/browser/io_thread.h:273: } On 2015/11/17 22:48:40, mmenke wrote: > nit: Blank line between methods Done. https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/io_thre... chrome/browser/io_thread.h:274: base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model() const { On 2015/11/17 22:48:40, mmenke wrote: > Hrm...actually, do we even need a weak pointer here? The IOThread class is > called the IOThread for a reason. Even ignoring that, as long as there's a > profile on the UI thread, the corresponding stuff will still exist on the > IOThread when a task is posted there. > > Seems like we could just get rid of the accessor entirely, and have the consumer > get the data_use_tab_model from the IOThread's external_data_use_observer > directly. > > More generally, exposing weak pointers widely obscures lifetimes, and is > generally considered a bit of an anti-pattern. But there could be a function on DataUseTabModel posted from UI thread on IO thread. In that case, it is possible that profile is destroyed on UI thread, but the function with raw pointer is still pending. So, it seems that we need to have a weak pointer to DataUseTabModel which is generated on IO thread.
https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:73: void SetIODataUseTabModel(base::WeakPtr<DataUseTabModel> data_use_tab_model); On 2015/11/18 01:32:23, tbansal1 wrote: > On 2015/11/17 22:50:11, sclittle wrote: > > This should be private. > > Done. This is called by profile_impl*, so can't be private.
https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:119: observer_list_.AddObserver(observer); nit: Add DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:123: observer_list_.RemoveObserver(observer); nit: Add DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:127: FOR_EACH_OBSERVER(TabDataUseObserver, observer_list_, nit: Add DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:132: FOR_EACH_OBSERVER(TabDataUseObserver, observer_list_, nit: Add DCHECK(thread_checker_.CalledOnValidThread()); same with the other methods https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:69: base::WeakPtr<DataUseTabModel> data_use_tab_model, nit: Pass in weak ptrs as const refs here, to avoid extra copy operations. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:85: ui_task_runner_(base::ThreadTaskRunnerHandle::Get()), Is this |ui_task_runner_| necessary? Can't you just call base::ThreadTaskRunnerHandle::Get() where you use it? https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:145: base::WeakPtr<DataUseTabModel> io_data_use_tab_model) { nit: pass in the WeakPtr as a const ref https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:153: weak_factory_.GetWeakPtr(), ui_task_runner_), Could you remove |ui_task_runner_| and just call base::ThreadTaskRunnerHandle::Get() here? https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:209: TabEvents::iterator it = tab_events_.find(tab_id); nit: this entire method could just be replaced with: return tab_events_.insert(std::make_pair(tab_id, event)).second; See http://www.cplusplus.com/reference/unordered_map/unordered_map/insert/ https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:220: TabEvents::iterator it = tab_events_.find(tab_id); nit: this entire method could just be replaced with: return tab_events_.erase(tab_id) > 0; See http://www.cplusplus.com/reference/unordered_map/unordered_map/erase/ https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:272: base::WeakPtr<DataUseUITabModel> data_use_ui_tab_model, nit: pass the WeakPtrs as const WeakPtr& to avoid extra copy operations https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:280: DCHECK(data_use_tab_model_); This DCHECK isn't guaranteed, the tab model could have been destroyed before the posted Create() task executed. Instead, just use an if statement around adding the observer. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:74: void SetIODataUseTabModel(base::WeakPtr<DataUseTabModel> data_use_tab_model); nit: pass in a const base::WeakPtr<DataUseTabModel>& to avoid an extra copy operation. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:81: class TabObserverOnIOThread; nit: Can this be moved to private? https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:122: // tracking. nit: Could you also mention here that it should only be used on the IO thread? https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.h:275: base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model() const { Why expose this here? Why not just call external_data_use_observer()->data_use_tab_model()->GetWeakPtr(), and remove the stored WeakPtr |data_use_tab_model_| below?
Patchset #5 (id:180001) has been deleted
ptal. thanks https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:119: observer_list_.AddObserver(observer); On 2015/11/18 21:42:03, sclittle wrote: > nit: Add DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:123: observer_list_.RemoveObserver(observer); On 2015/11/18 21:42:03, sclittle wrote: > nit: Add DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:127: FOR_EACH_OBSERVER(TabDataUseObserver, observer_list_, On 2015/11/18 21:42:03, sclittle wrote: > nit: Add DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:132: FOR_EACH_OBSERVER(TabDataUseObserver, observer_list_, On 2015/11/18 21:42:03, sclittle wrote: > nit: Add DCHECK(thread_checker_.CalledOnValidThread()); > > same with the other methods Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:69: base::WeakPtr<DataUseTabModel> data_use_tab_model, On 2015/11/18 21:42:03, sclittle wrote: > nit: Pass in weak ptrs as const refs here, to avoid extra copy operations. Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:85: ui_task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2015/11/18 21:42:03, sclittle wrote: > Is this |ui_task_runner_| necessary? Can't you just call > base::ThreadTaskRunnerHandle::Get() where you use it? Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:145: base::WeakPtr<DataUseTabModel> io_data_use_tab_model) { On 2015/11/18 21:42:03, sclittle wrote: > nit: pass in the WeakPtr as a const ref Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:153: weak_factory_.GetWeakPtr(), ui_task_runner_), On 2015/11/18 21:42:03, sclittle wrote: > Could you remove |ui_task_runner_| and just call > base::ThreadTaskRunnerHandle::Get() here? Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:209: TabEvents::iterator it = tab_events_.find(tab_id); On 2015/11/18 21:42:03, sclittle wrote: > nit: this entire method could just be replaced with: > > return tab_events_.insert(std::make_pair(tab_id, event)).second; > > See http://www.cplusplus.com/reference/unordered_map/unordered_map/insert/ Done. This is good! https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:220: TabEvents::iterator it = tab_events_.find(tab_id); On 2015/11/18 21:42:03, sclittle wrote: > nit: this entire method could just be replaced with: > > return tab_events_.erase(tab_id) > 0; > > See http://www.cplusplus.com/reference/unordered_map/unordered_map/erase/ Probably not, we match both the Key and Value before deleting. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:272: base::WeakPtr<DataUseUITabModel> data_use_ui_tab_model, On 2015/11/18 21:42:03, sclittle wrote: > nit: pass the WeakPtrs as const WeakPtr& to avoid extra copy operations Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:280: DCHECK(data_use_tab_model_); On 2015/11/18 21:42:03, sclittle wrote: > This DCHECK isn't guaranteed, the tab model could have been destroyed before the > posted Create() task executed. Instead, just use an if statement around adding > the observer. Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:74: void SetIODataUseTabModel(base::WeakPtr<DataUseTabModel> data_use_tab_model); On 2015/11/18 21:42:04, sclittle wrote: > nit: pass in a const base::WeakPtr<DataUseTabModel>& to avoid an extra copy > operation. Done. Made the CreateTabObserverOnIOThread a static function as mmenke had suggested. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:81: class TabObserverOnIOThread; On 2015/11/18 21:42:04, sclittle wrote: > nit: Can this be moved to private? No, CreateTabObserverOnIOThread in .cc file needs this. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:122: // tracking. On 2015/11/18 21:42:04, sclittle wrote: > nit: Could you also mention here that it should only be used on the IO thread? Done. https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.h:275: base::WeakPtr<chrome::android::DataUseTabModel> data_use_tab_model() const { On 2015/11/18 21:42:04, sclittle wrote: > Why expose this here? Why not just call > external_data_use_observer()->data_use_tab_model()->GetWeakPtr(), and remove the > stored WeakPtr |data_use_tab_model_| below? GetWeakPtr() needs to be called on IO thread. profile_impl* is on UI thread. I call GetWeakPtr() here on IO, save it, and then just pass it along in profile_impl*.
https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:30: TabObserverOnIOThread( Maybe this class isn't necessary. What if DataUseTabModel just had a map of WeakPtr<DataUseUITabModel>s and each DataUseUITabModel just registered itself directly with its own WeakPtr? Then, both classes could just PostTasks at each other to communicate. You'd probably want to key the map like std::map<const DataUseUITabModel*, WeakPtr<DataUseUITabModel>> so that the DataUseUITabModels can remove themselves as observers. It would be safe to use a regular std::map instead of a special ObserverList because all notifications would happen asyncronously with PostTask, so it would be impossible for the map to change while it's being iterated over. https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:36: friend class DataUseUITabModelTest; This friending does nothing, so remove it. https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:77: io_task_runner_->ReleaseSoon(FROM_HERE, tab_data_use_observer_.get()); I don't think this is safe - ReleaseSoon could fail if the other thread doesn't exist anymore, see https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas... It would be simpler to just remove the observer class altogether like I commented above. Alternatively you could have the IOTabModel own the observers, but at that point you might as well just link the IOTabModel with the UITabModel directly without the observer. https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.h:547: // WeakPtr to DataUseTabModel to be used on IO thread. Is there some better way to expose the WeakPtr to the UI thread than this? This just seems a bit cluttered.
Patchset #6 (id:220001) has been deleted
Patchset #7 (id:260001) has been deleted
ptal. thanks. https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:30: TabObserverOnIOThread( On 2015/11/19 21:53:24, sclittle wrote: > Maybe this class isn't necessary. What if DataUseTabModel just had a map of > WeakPtr<DataUseUITabModel>s and each DataUseUITabModel just registered itself > directly with its own WeakPtr? Then, both classes could just PostTasks at each > other to communicate. > > You'd probably want to key the map like std::map<const DataUseUITabModel*, > WeakPtr<DataUseUITabModel>> so that the DataUseUITabModels can remove themselves > as observers. > > It would be safe to use a regular std::map instead of a special ObserverList > because all notifications would happen asyncronously with PostTask, so it would > be impossible for the map to change while it's being iterated over. Done. https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:36: friend class DataUseUITabModelTest; On 2015/11/19 21:53:24, sclittle wrote: > This friending does nothing, so remove it. Done. https://codereview.chromium.org/1443683002/diff/200001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:77: io_task_runner_->ReleaseSoon(FROM_HERE, tab_data_use_observer_.get()); On 2015/11/19 21:53:24, sclittle wrote: > I don't think this is safe - ReleaseSoon could fail if the other thread doesn't > exist anymore, see > https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas... > > It would be simpler to just remove the observer class altogether like I > commented above. Alternatively you could have the IOTabModel own the observers, > but at that point you might as well just link the IOTabModel with the UITabModel > directly without the observer. Done.
https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:10: #include "content/public/browser/browser_thread.h" Remove this include, since you're not actually using BrowserThread here. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:19: #include "components/data_usage/core/data_use.h" Replace this include with a forward declaration of DataUse. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:77: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); Add a forward declaration for base::SingleThreadTaskRunner in this header file and an #include in the cc file. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:136: base::WeakPtrFactory<MockTabDataUseObserver> weak_ptr_factory_; You should DISALLOW_COPY_AND_ASSIGN, since it doesn't make any sense to copy these observers (i.e. only the original observer is notified anyways, as this is implemented right now). https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:17: #include "base/single_thread_task_runner.h" Just forward declare it here in the header file, and move this #include to the cc file. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:640: data_use_tab_model_ = Is it safe to set |data_use_tab_model_| on the IO thread here but read it on the UI thread like you are in profile_impl_io_data? https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.h:545: scoped_ptr<chrome::android::ExternalDataUseObserver> You should rebase on master; I've moved these objects into IOThread::Globals so that WeakPtrs work correctly.
Patchset #8 (id:300001) has been deleted
Patchset #8 (id:320001) has been deleted
Patchset #8 (id:340001) has been deleted
Patchset #8 (id:360001) has been deleted
tbansal@chromium.org changed reviewers: + bengr@chromium.org - mmenke@chromium.org
bengr: PTAL at profile_impl_io_data* sclittle: * Thanks! https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:10: #include "content/public/browser/browser_thread.h" On 2015/11/20 22:50:54, sclittle wrote: > Remove this include, since you're not actually using BrowserThread here. Done. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:19: #include "components/data_usage/core/data_use.h" On 2015/11/20 22:50:54, sclittle wrote: > Replace this include with a forward declaration of DataUse. Done. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:77: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); On 2015/11/20 22:50:54, sclittle wrote: > Add a forward declaration for base::SingleThreadTaskRunner in this header file > and an #include in the cc file. Done. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:136: base::WeakPtrFactory<MockTabDataUseObserver> weak_ptr_factory_; On 2015/11/20 22:50:54, sclittle wrote: > You should DISALLOW_COPY_AND_ASSIGN, since it doesn't make any sense to copy > these observers (i.e. only the original observer is notified anyways, as this is > implemented right now). Done. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:17: #include "base/single_thread_task_runner.h" On 2015/11/20 22:50:54, sclittle wrote: > Just forward declare it here in the header file, and move this #include to the > cc file. Done. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:640: data_use_tab_model_ = On 2015/11/20 22:50:54, sclittle wrote: > Is it safe to set |data_use_tab_model_| on the IO thread here but read it on the > UI thread like you are in profile_impl_io_data? Obsolete. https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.h:545: scoped_ptr<chrome::android::ExternalDataUseObserver> On 2015/11/20 22:50:54, sclittle wrote: > You should rebase on master; I've moved these objects into IOThread::Globals so > that WeakPtrs work correctly. Done.
https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_helper.cc (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:29: if (!navigation_handle->IsInMainFrame()) I suppose this makes sense. Add a TODO to consider the case of a page that provides a navigation bar and loads pages in a sub-frame. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:36: int32_t tab_id = SessionTabHelper::IdForTab(web_contents()); Why not make tab_id a SessionID::id_type and include session_id.h? https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:51: render_frame_host->IsRenderFrameLive()) { Why do you only proceed if the render frame is not live? https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:59: int32_t tab_id = SessionTabHelper::IdForTab(web_contents()); SessionID::id_type? https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:24: } // namespace base No need for the end comment for a single-line namespace declaration. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:28: } // namespace data_usage No need for the end comment for a single-line namespace declaration. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:116: protected: Since you've already friended a bunch of stuff, why not make these private? https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:119: void NotifyObserversOfTrackingStarting(int32_t tab_id); You should probably use the SessionID::tab_id type everywhere, though I don't feel strongly about that. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:171: // notifications. Map is keyed by raw pointers, and notifications are posted This comment is very confusing. Why do you need the raw pointers at all? Why not just have a list of WeakPtrs? https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (left): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:616: DataUseTabModel::TRANSITION_OMNIBOX_NAVIGATION, Why are these transitions being removed from the test? https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:294: EXPECT_EQ(expected_observer_count, data_use_tab_model_->observers_.size()); Should probably also check that this size increases with each additional observer. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:176: bool DataUseUITabModel::ConvertTransitionType( Can this be a method in an anonymous namespace, instead of being a class method?
ptal. thanks. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_helper.cc (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:29: if (!navigation_handle->IsInMainFrame()) On 2015/11/23 18:26:25, bengr wrote: > I suppose this makes sense. Add a TODO to consider the case of a page that > provides a navigation bar and loads pages in a sub-frame. Done. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:36: int32_t tab_id = SessionTabHelper::IdForTab(web_contents()); On 2015/11/23 18:26:25, bengr wrote: > Why not make tab_id a SessionID::id_type and include session_id.h? Done. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:51: render_frame_host->IsRenderFrameLive()) { On 2015/11/23 18:26:25, bengr wrote: > Why do you only proceed if the render frame is not live? Done. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_helper.cc:59: int32_t tab_id = SessionTabHelper::IdForTab(web_contents()); On 2015/11/23 18:26:25, bengr wrote: > SessionID::id_type? Done. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:24: } // namespace base On 2015/11/23 18:26:25, bengr wrote: > No need for the end comment for a single-line namespace declaration. Done. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:28: } // namespace data_usage On 2015/11/23 18:26:25, bengr wrote: > No need for the end comment for a single-line namespace declaration. Done. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:116: protected: On 2015/11/23 18:26:25, bengr wrote: > Since you've already friended a bunch of stuff, why not make these private? Friending is used by tests in data_use_tab_model_unittest.cc. "Protecting" makes it easier for derived test classes in data_use_ui_tab_model_unittest.cc https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:119: void NotifyObserversOfTrackingStarting(int32_t tab_id); On 2015/11/23 18:26:25, bengr wrote: > You should probably use the SessionID::tab_id type everywhere, though I don't > feel strongly about that. Done. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:171: // notifications. Map is keyed by raw pointers, and notifications are posted On 2015/11/23 18:26:25, bengr wrote: > This comment is very confusing. Why do you need the raw pointers at all? Why not > just have a list of WeakPtrs? Raw pointers are needed to remove observers. It is not possible to compare weak pointers. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (left): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:616: DataUseTabModel::TRANSITION_OMNIBOX_NAVIGATION, On 2015/11/23 18:26:25, bengr wrote: > Why are these transitions being removed from the test? EXTERNAL_APP is not used. NAVSUGGEST is same as OMNIBOX_NAVIGATION. OMNIBOX_NAVIGATION is still used but it is not an exit event. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:294: EXPECT_EQ(expected_observer_count, data_use_tab_model_->observers_.size()); On 2015/11/23 18:26:25, bengr wrote: > Should probably also check that this size increases with each additional > observer. I do not understand this comment. I am checking that size is increasing. Did you mean something else? https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:176: bool DataUseUITabModel::ConvertTransitionType( On 2015/11/23 18:26:25, bengr wrote: > Can this be a method in an anonymous namespace, instead of being a class method? Needed for Testing.
https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:30: bool IsValidTabID(int32_t tab_id) { nit: either use SessionID::id_type everywhere, or int32_t everywhere https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:8: #include <stdint.h> nit: this include isn't needed anymore if you're not using int32_t https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:21: #include "url/gurl.h" nit: Remove this include and make it a forward declaration instead https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:65: void ReportTabClosure(int32_t tab_id); nit: I think you forgot to change the type of this tab ID https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:99: typedef base::hash_map<int32_t, DataUseTrackingEvent> TabEvents; nit: either consistently use int32_t, or SessionID::id_type, here and in the rest of these. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:54: base::WeakPtrFactory<TestDataUseTabModel> weak_factory_; I don't think you need this weak factory, since you could just use the GetWeakPtr() from DataUseTabModel. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:99: const base::WeakPtr<DataUseTabModel>& data_use_tab_model_weak_ptr() const { nit: Why can't this just be as it was before? Or make this a non-const method and name it data_use_tab_model() and return data_use_tab_model_->GetWeakPtr()? You'd want to move the implementation to the .cc file and add a thread_checker_ check. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:242: data_use_ui_tab_model->SetIOThread(io_thread); You have an extra hop to the IO thread and back just to get the WeakPtr<DataUseTabModel>, which seems suboptimal, and you're passing a raw IOThread pointer all the way down just to make this hop, which is weird for layering. I think your original design of having a WeakPtr<DataUseTabModel> member in IOThread was nicer, unless you know of a nicer way to do this than that.
Patchset #10 (id:420001) has been deleted
ptal. thanks!! https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:30: bool IsValidTabID(int32_t tab_id) { On 2015/11/23 23:06:16, sclittle wrote: > nit: either use SessionID::id_type everywhere, or int32_t everywhere Done. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:8: #include <stdint.h> On 2015/11/23 23:06:16, sclittle wrote: > nit: this include isn't needed anymore if you're not using int32_t Done. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:21: #include "url/gurl.h" On 2015/11/23 23:06:16, sclittle wrote: > nit: Remove this include and make it a forward declaration instead Not sure how to forward declare a typedef without repeating the definition. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:65: void ReportTabClosure(int32_t tab_id); On 2015/11/23 23:06:16, sclittle wrote: > nit: I think you forgot to change the type of this tab ID Done. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:99: typedef base::hash_map<int32_t, DataUseTrackingEvent> TabEvents; On 2015/11/23 23:06:16, sclittle wrote: > nit: either consistently use int32_t, or SessionID::id_type, here and in the > rest of these. Done. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:54: base::WeakPtrFactory<TestDataUseTabModel> weak_factory_; On 2015/11/23 23:06:16, sclittle wrote: > I don't think you need this weak factory, since you could just use the > GetWeakPtr() from DataUseTabModel. Done. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:99: const base::WeakPtr<DataUseTabModel>& data_use_tab_model_weak_ptr() const { On 2015/11/23 23:06:16, sclittle wrote: > nit: Why can't this just be as it was before? Or make this a non-const method > and name it data_use_tab_model() and return data_use_tab_model_->GetWeakPtr()? > You'd want to move the implementation to the .cc file and add a thread_checker_ > check. weak ptr has to be generated on IO thread, stored somewhere where it is accessible on UI thread for consumption by profile_impl_io_data*. The weakptr can't be stored in IOThread::Globals() because globals are not accessible on UI thread. Also, it has to be generated before it is consumed by profile_impl_io_data* because profile_impl_io_data* runs on UI thread. If I change it to "return data_use_tab_model_->GetWeakPtr()?", it is not clear where this function would be called, and where the weak ptr would be stored. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:242: data_use_ui_tab_model->SetIOThread(io_thread); On 2015/11/23 23:06:16, sclittle wrote: > You have an extra hop to the IO thread and back just to get the > WeakPtr<DataUseTabModel>, which seems suboptimal, and you're passing a raw > IOThread pointer all the way down just to make this hop, which is weird for > layering. > > I think your original design of having a WeakPtr<DataUseTabModel> member in > IOThread was nicer, unless you know of a nicer way to do this than that. The original design does not work anymore because IOThread::Globals can't be accessed on UI thread.
https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:21: #include "url/gurl.h" On 2015/11/24 00:42:02, tbansal1 wrote: > On 2015/11/23 23:06:16, sclittle wrote: > > nit: Remove this include and make it a forward declaration instead > > Not sure how to forward declare a typedef without repeating the definition. GURL isn't a typedef, so this shouldn't be a problem? https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:99: const base::WeakPtr<DataUseTabModel>& data_use_tab_model_weak_ptr() const { On 2015/11/24 00:42:03, tbansal1 wrote: > On 2015/11/23 23:06:16, sclittle wrote: > > nit: Why can't this just be as it was before? Or make this a non-const method > > and name it data_use_tab_model() and return data_use_tab_model_->GetWeakPtr()? > > You'd want to move the implementation to the .cc file and add a > thread_checker_ > > check. > > weak ptr has to be generated on IO thread, stored somewhere where it is > accessible on UI thread for consumption by profile_impl_io_data*. The weakptr > can't be stored in IOThread::Globals() because globals are not accessible on UI > thread. Also, it has to be generated before it is consumed by > profile_impl_io_data* because profile_impl_io_data* runs on UI thread. > > If I change it to "return data_use_tab_model_->GetWeakPtr()?", it is not clear > where this function would be called, and where the weak ptr would be stored. I'm confused - this method is run on the IO thread, right? If so, then data_use_tab_model_->GetWeakPtr() is just as safe to run, right? https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:165: const base::WeakPtr<TabDataUseObserver> weak_ptr_observer) { There's no point in making this const, since WeakPtr::get() is const. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:167: observers_.insert(ObserverMap::value_type(observer, weak_ptr_observer)); DCHECK that the observer isn't already in the map. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:173: observers_.erase(observer); DCHECK that the observer is in the map. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:6: You still need <stdint.h> for the int32_t's used in this file https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:31: return io_thread->globals() Could it be possible for io_thread->globals() to have already been cleaned up at this point? Perhaps you could check that io_thread->globals() isn't NULL. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:238: // Pass the DataUseTabModel weak pointer to DataUseUITabModel. CreateMainRequestContextGetter seems like the wrong place for this code, since it has nothing to do with creating a request context getter, and if for some reason this method is called multiple times, this would register the TabModels together again. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:242: data_use_ui_tab_model->SetIOThread(io_thread); Could you move the anonymous function to get the WeakPtr<DataUseTabModel> into this file, or wherever the DataUseUITabModel is initialized? It would be nice for layering to avoid having DataUseUITabModel depend on IOThread.
Patchset #11 (id:460001) has been deleted
ptal. thanks. https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:21: #include "url/gurl.h" On 2015/11/24 04:11:50, sclittle wrote: > On 2015/11/24 00:42:02, tbansal1 wrote: > > On 2015/11/23 23:06:16, sclittle wrote: > > > nit: Remove this include and make it a forward declaration instead > > > > Not sure how to forward declare a typedef without repeating the definition. > > GURL isn't a typedef, so this shouldn't be a problem? That's required because some functions pass GURL by const reference (not by pointer). ../../chrome/browser/android/data_usage/data_use_tab_model.h:93:37: error: 'GURL' in namespace 'net' does not name a type const net::GURL& url, https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:165: const base::WeakPtr<TabDataUseObserver> weak_ptr_observer) { On 2015/11/24 04:11:50, sclittle wrote: > There's no point in making this const, since WeakPtr::get() is const. const qualifier prevents something like weak_ptr_observer = some_other_weak_ptr_observer; at compile-time. Also, it seems to be used enough in net/ and outside: https://code.google.com/p/chromium/codesearch#search/&q=%22const%20base::Weak... https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:167: observers_.insert(ObserverMap::value_type(observer, weak_ptr_observer)); On 2015/11/24 04:11:50, sclittle wrote: > DCHECK that the observer isn't already in the map. Good point. Done similar to https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list... https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:173: observers_.erase(observer); On 2015/11/24 04:11:50, sclittle wrote: > DCHECK that the observer is in the map. Done. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:6: On 2015/11/24 04:11:51, sclittle wrote: > You still need <stdint.h> for the int32_t's used in this file Done. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:31: return io_thread->globals() On 2015/11/24 04:11:51, sclittle wrote: > Could it be possible for io_thread->globals() to have already been cleaned up at > this point? Perhaps you could check that io_thread->globals() isn't NULL. Obsolete. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:238: // Pass the DataUseTabModel weak pointer to DataUseUITabModel. On 2015/11/24 04:11:51, sclittle wrote: > CreateMainRequestContextGetter seems like the wrong place for this code, since > it has nothing to do with creating a request context getter, and if for some > reason this method is called multiple times, this would register the TabModels > together again. Good point. Moved to Init(). Also, added checks in DataUseTabModel to make sure that UI Tab Model only registers at most once. https://codereview.chromium.org/1443683002/diff/440001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:242: data_use_ui_tab_model->SetIOThread(io_thread); On 2015/11/24 04:11:51, sclittle wrote: > Could you move the anonymous function to get the WeakPtr<DataUseTabModel> into > this file, or wherever the DataUseUITabModel is initialized? It would be nice > for layering to avoid having DataUseUITabModel depend on IOThread. Now, storing the weak ptr in IO thread to reduce thread hops.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:21: #include "url/gurl.h" On 2015/11/24 20:01:12, tbansal1 wrote: > On 2015/11/24 04:11:50, sclittle wrote: > > On 2015/11/24 00:42:02, tbansal1 wrote: > > > On 2015/11/23 23:06:16, sclittle wrote: > > > > nit: Remove this include and make it a forward declaration instead > > > > > > Not sure how to forward declare a typedef without repeating the definition. > > > > GURL isn't a typedef, so this shouldn't be a problem? > > That's required because some functions pass GURL by const reference (not by > pointer). > > ../../chrome/browser/android/data_usage/data_use_tab_model.h:93:37: error: > 'GURL' in namespace 'net' does not name a type > const net::GURL& url, You can forward declare classes you pass by reference (const or not), since it is basically the same thing as a pointer.
https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:21: #include "url/gurl.h" On 2015/11/24 20:44:28, mmenke wrote: > On 2015/11/24 20:01:12, tbansal1 wrote: > > On 2015/11/24 04:11:50, sclittle wrote: > > > On 2015/11/24 00:42:02, tbansal1 wrote: > > > > On 2015/11/23 23:06:16, sclittle wrote: > > > > > nit: Remove this include and make it a forward declaration instead > > > > > > > > Not sure how to forward declare a typedef without repeating the > definition. > > > > > > GURL isn't a typedef, so this shouldn't be a problem? > > > > That's required because some functions pass GURL by const reference (not by > > pointer). > > > > ../../chrome/browser/android/data_usage/data_use_tab_model.h:93:37: error: > > 'GURL' in namespace 'net' does not name a type > > const net::GURL& url, > > You can forward declare classes you pass by reference (const or not), since it > is basically the same thing as a pointer. Done.
https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/400001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:21: #include "url/gurl.h" On 2015/11/24 20:01:12, tbansal1 wrote: > On 2015/11/24 04:11:50, sclittle wrote: > > On 2015/11/24 00:42:02, tbansal1 wrote: > > > On 2015/11/23 23:06:16, sclittle wrote: > > > > nit: Remove this include and make it a forward declaration instead > > > > > > Not sure how to forward declare a typedef without repeating the definition. > > > > GURL isn't a typedef, so this shouldn't be a problem? > > That's required because some functions pass GURL by const reference (not by > pointer). > > ../../chrome/browser/android/data_usage/data_use_tab_model.h:93:37: error: > 'GURL' in namespace 'net' does not name a type > const net::GURL& url, Const ref works fine with forward declarations, I think that error is caused by you trying to forward declare GURL under the "net" namespace, which is incorrect. GURL is in the global namespace. See https://code.google.com/p/chromium/codesearch#search/&q=%22class%20GURL;%22&s... https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:162: const int32_t mask = 0xFFFFFFFF ^ ui::PAGE_TRANSITION_QUALIFIER_MASK; nit: Could you get rid of this variable and just use PAGE_TRANSITION_CORE_MASK? https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/page_trans... https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:49: class DataUseUITabModelTest : public ChromeRenderViewHostTestHarness { Do you really need a full render view test harness here, or could you just make do with simulating the different tab events and using a test browser context? Then, you could use a TestBrowserThreadBundle with IO_MAINLOOP (which can't be used with RenderViewHostTestHarness), which would make any cross-thread testing here MUCH easier since then the IO thread and the UI thread would use the same MessageLoop, meaning that you could just call RunUntilIdle() and that would cover both IO and UI threads. https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:55: base::RunLoop().RunUntilIdle(); Why is this here? This shouldn't need to be here. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:80: content::BrowserThread::IO); None of these objects actually live on the IO thread, they all live here on the UI thread. This doesn't test any of the different thread stuff between the objects. You could either switch over to using an IO_MAINLOOP test setup like my comment above (probably easier), or actually simulate this with a real IO thread (probably harder). For example, the TabIdAnnotator test uses a real IO thread by passing around a base::RunLoop for the UI thread, and posting a Quit task to it later. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dat... https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/io_thre... chrome/browser/io_thread.h:548: // WeakPtr to DataUseTabModel to be used on IO thread. Please be more specific - data_use_tab_model_ should only be dereferenced on the IO thread, but should only be accessed on the UI thread after IOThread::Init ran on the IO thread. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:72: #include "chrome/browser/android/data_usage/external_data_use_observer.h" nit: unnecessary include https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:216: g_browser_process->io_thread()->data_use_tab_model()); This is only safe if you can be certain that io_thread()->data_use_tab_model is always set before this line here is run, otherwise you have a race condition on io_thread()->data_use_tab_model, or you'll end up copying the WeakPtr before it's set. Is this guaranteed to be true? If so, could you add a comment describing that?
I'll take another look at this after sclittle@ is done. As for setting up the connections between UI and IO objects, I wonder if we can't do what we did with the d_r_p. See: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... for the construction of the IO object and https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... for the initialization of the UI object. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:116: protected: On 2015/11/23 19:58:08, tbansal1 wrote: > On 2015/11/23 18:26:25, bengr wrote: > > Since you've already friended a bunch of stuff, why not make these private? > > Friending is used by tests in data_use_tab_model_unittest.cc. > "Protecting" makes it easier for derived test classes in > data_use_ui_tab_model_unittest.cc Acknowledged. https://codereview.chromium.org/1443683002/diff/380001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:171: // notifications. Map is keyed by raw pointers, and notifications are posted On 2015/11/23 19:58:08, tbansal1 wrote: > On 2015/11/23 18:26:25, bengr wrote: > > This comment is very confusing. Why do you need the raw pointers at all? Why > not > > just have a list of WeakPtrs? > > Raw pointers are needed to remove observers. It is not possible to compare weak > pointers. Acknowledged.
Patchset #13 (id:520001) has been deleted
PTAL. Thanks. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:162: const int32_t mask = 0xFFFFFFFF ^ ui::PAGE_TRANSITION_QUALIFIER_MASK; On 2015/11/24 23:07:37, sclittle wrote: > nit: Could you get rid of this variable and just use PAGE_TRANSITION_CORE_MASK? > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/page_trans... Done. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:49: class DataUseUITabModelTest : public ChromeRenderViewHostTestHarness { On 2015/11/24 23:07:37, sclittle wrote: > Do you really need a full render view test harness here, Yes, because it uses ChromeRenderViewHostTestHarness::web_contents > or could you just make > do with simulating the different tab events and using a test browser context? > > Then, you could use a TestBrowserThreadBundle with IO_MAINLOOP (which can't be > used with RenderViewHostTestHarness), which would make any cross-thread testing > here MUCH easier since then the IO thread and the UI thread would use the same > MessageLoop, meaning that you could just call RunUntilIdle() and that would > cover both IO and UI threads. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:55: base::RunLoop().RunUntilIdle(); On 2015/11/24 23:07:37, sclittle wrote: > Why is this here? This shouldn't need to be here. Done. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:80: content::BrowserThread::IO); On 2015/11/24 23:07:37, sclittle wrote: > None of these objects actually live on the IO thread, they all live here on the > UI thread. This doesn't test any of the different thread stuff between the > objects. > > You could either switch over to using an IO_MAINLOOP test setup like my comment > above (probably easier), or actually simulate this with a real IO thread > (probably harder). > > For example, the TabIdAnnotator test uses a real IO thread by passing around a > base::RunLoop for the UI thread, and posting a Quit task to it later. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dat... I need the ability to use web_contents, that;s why ChromeRenderViewHostTestHarness. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/io_thre... chrome/browser/io_thread.h:548: // WeakPtr to DataUseTabModel to be used on IO thread. On 2015/11/24 23:07:37, sclittle wrote: > Please be more specific - data_use_tab_model_ should only be dereferenced on the > IO thread, but should only be accessed on the UI thread after IOThread::Init ran > on the IO thread. Obsolete. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:72: #include "chrome/browser/android/data_usage/external_data_use_observer.h" On 2015/11/24 23:07:37, sclittle wrote: > nit: unnecessary include Obsolete. https://codereview.chromium.org/1443683002/diff/480001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:216: g_browser_process->io_thread()->data_use_tab_model()); On 2015/11/24 23:07:37, sclittle wrote: > This is only safe if you can be certain that io_thread()->data_use_tab_model is > always set before this line here is run, otherwise you have a race condition on > io_thread()->data_use_tab_model, or you'll end up copying the WeakPtr before > it's set. > > Is this guaranteed to be true? If so, could you add a comment describing that? Obsolete.
https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:168: DCHECK_LT(0U, observers_.size()); Remove this DCHECK. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:179: for (const auto& it : observers_) { use |observer| or something instead of |it| here, since |it| implies an iterator. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:180: ui_task_runner_->PostTask( Instead of posting a task for each of these, you could just post one task with a copy of the |observers_| list, which then calls NotifyTrackingStarting on each of those. Please either do this or add a TODO to do this. Same below with NotifyTrackingEnding. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:113: // TODO(tbansal): Remove observers that have been destroyed. FYI: Please either do this now or in a followup CL soon; otherwise there's nothing preventing this from growing unbounded. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:146: typedef std::vector<const base::WeakPtr<TabDataUseObserver>> Observers; Use an std::list so that iterators are stable, and remove the const qualifier on the base::WeakPtr, since it doesn't actually prevent anyone from dereferencing it, since base::WeakPtr::get() is const. Also, does this really need to be a typedef, or could you just use it inline below? https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:58: class DataUseTabModelNowTest : public DataUseTabModel { nit: Could you move this class into the anonymous namespace above? You can also move that anonymous namespace into the chrome::android namespace here, so that you don't have to qualify all the names. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:67: void AdvanceTime(base::TimeDelta now_offset) { now_offset_ = now_offset; } nit: Change this test class to use a SimpleTestTickClock instead, calling Advance() and NowTicks() on the clock. https://code.google.com/p/chromium/codesearch#chromium/src/base/test/simple_t... https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:196: class MockTabDataUseObserver : public DataUseTabModel::TabDataUseObserver { nit: Could you move this into the anonymous namespace? https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:49: class DataUseUITabModelTest : public ChromeRenderViewHostTestHarness { As per our discussion offline, you don't actually need RenderViewHostTestHarness, you can just create a TestBrowserContext and get the DataUseUITabModel from that. https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:105: std::string foo_label("foo_label"); nit: Make these and the other variables like these into constants. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:130: SessionID::id_type foo_tab_id = 0; nit: use a non-zero tab ID, e.g. 100, so that this test doesn't accidentally pass if someone later accidentally initializes a tab ID to 0 or something. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:174: ++foo_tab_id; Use a different variable for this, e.g. bar_tab_id https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:252: base::Bind(&SetDataUseTabModelOnUIThread, You could just bind &chrome::android::DataUseUITabModel::SetDataUseTabModel here directly instead of using an anonymous function.
ptal. thanks. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:168: DCHECK_LT(0U, observers_.size()); On 2015/11/25 22:49:11, sclittle wrote: > Remove this DCHECK. Done. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:179: for (const auto& it : observers_) { On 2015/11/25 22:49:11, sclittle wrote: > use |observer| or something instead of |it| here, since |it| implies an > iterator. Done. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:180: ui_task_runner_->PostTask( On 2015/11/25 22:49:11, sclittle wrote: > Instead of posting a task for each of these, you could just post one task with a > copy of the |observers_| list, which then calls NotifyTrackingStarting on each > of those. > > Please either do this or add a TODO to do this. Same below with > NotifyTrackingEnding. Does it not depend on the size of the list? If we expect the size of the list to be ~1, then running a for loop over 1 entry should be better. Right? https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:113: // TODO(tbansal): Remove observers that have been destroyed. On 2015/11/25 22:49:11, sclittle wrote: > FYI: Please either do this now or in a followup CL soon; otherwise there's > nothing preventing this from growing unbounded. Done. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.h:146: typedef std::vector<const base::WeakPtr<TabDataUseObserver>> Observers; On 2015/11/25 22:49:11, sclittle wrote: > Use an std::list so that iterators are stable, and remove the const qualifier on > the base::WeakPtr, since it doesn't actually prevent anyone from dereferencing > it, since base::WeakPtr::get() is const. > I was going to do it when adding RemoveObserver but done for now. > Also, does this really need to be a typedef, or could you just use it inline > below? I do not feel strongly about it but AFAIK it is okay to typedef containers. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:58: class DataUseTabModelNowTest : public DataUseTabModel { On 2015/11/25 22:49:11, sclittle wrote: > nit: Could you move this class into the anonymous namespace above? You can also > move that anonymous namespace into the chrome::android namespace here, so that > you don't have to qualify all the names. Added TODO for Raj. Also, I don't want to pollute this CL with changes that are not related to this CL. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:67: void AdvanceTime(base::TimeDelta now_offset) { now_offset_ = now_offset; } On 2015/11/25 22:49:11, sclittle wrote: > nit: Change this test class to use a SimpleTestTickClock instead, calling > Advance() and NowTicks() on the clock. > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/simple_t... TODO for Raj. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:196: class MockTabDataUseObserver : public DataUseTabModel::TabDataUseObserver { On 2015/11/25 22:49:11, sclittle wrote: > nit: Could you move this into the anonymous namespace? TODO for Raj. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:49: class DataUseUITabModelTest : public ChromeRenderViewHostTestHarness { On 2015/11/25 22:49:11, sclittle wrote: > As per our discussion offline, you don't actually need > RenderViewHostTestHarness, you can just create a TestBrowserContext and get the > DataUseUITabModel from that. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... Creating DataUseUITabModel object directly, although I still do not understand why ChromeRenderViewHostTestHarness is prohibited here. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_uGtLS0atUY https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:105: std::string foo_label("foo_label"); On 2015/11/25 22:49:11, sclittle wrote: > nit: Make these and the other variables like these into constants. Done. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:130: SessionID::id_type foo_tab_id = 0; On 2015/11/25 22:49:11, sclittle wrote: > nit: use a non-zero tab ID, e.g. 100, so that this test doesn't accidentally > pass if someone later accidentally initializes a tab ID to 0 or something. Done, but 0 is a valid tab id. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:174: ++foo_tab_id; On 2015/11/25 22:49:11, sclittle wrote: > Use a different variable for this, e.g. bar_tab_id Done. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:252: base::Bind(&SetDataUseTabModelOnUIThread, On 2015/11/25 22:49:11, sclittle wrote: > You could just bind &chrome::android::DataUseUITabModel::SetDataUseTabModel here > directly instead of using an anonymous function. Done.
Patchset #15 (id:580001) has been deleted
lgtm % nits Thanks for doing this, sorry for the lengthy review. https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_model.cc:180: ui_task_runner_->PostTask( On 2015/11/26 00:31:21, tbansal1 wrote: > On 2015/11/25 22:49:11, sclittle wrote: > > Instead of posting a task for each of these, you could just post one task with > a > > copy of the |observers_| list, which then calls NotifyTrackingStarting on each > > of those. > > > > Please either do this or add a TODO to do this. Same below with > > NotifyTrackingEnding. > > Does it not depend on the size of the list? If we expect the size of the list to > be ~1, then running a for loop over 1 entry should be better. Right? Sure, that should be fine. Could you add a comment that the expected size of this list is 1, so there's no need to batch these together? https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1443683002/diff/540001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:49: class DataUseUITabModelTest : public ChromeRenderViewHostTestHarness { On 2015/11/26 00:31:21, tbansal1 wrote: > On 2015/11/25 22:49:11, sclittle wrote: > > As per our discussion offline, you don't actually need > > RenderViewHostTestHarness, you can just create a TestBrowserContext and get > the > > DataUseUITabModel from that. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > Creating DataUseUITabModel object directly, although I still do not understand > why ChromeRenderViewHostTestHarness is prohibited here. > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/_uGtLS0atUY OK, then can you use a TestingProfile instead, like that google groups post suggests? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/t... Alternatively, you could just keep it like you have it now, which should be OK. https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc (right): https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc:28: static_cast<SessionID::id_type>(tab_id)); nit: Could you DCHECK that |tab_id| is a valid >=0 tab ID here? https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:105: AtMostOneDataUseSubmitRequest); nit: I don't think this test exists anymore https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:116: FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, ReportTabEventsTest); nit: This should alphabetically come before the ExternalDataUseObserverTest macros, since it's DataUseUITabModelTest. https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:109: if (!io_thread || !io_thread->globals()) nit: It looks like |io_thread| and |io_thread->globals()| are guaranteed to exist here, so you can remove this if statement.
mmenke: PTAL. Thanks. https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc (right): https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... chrome/browser/android/data_usage/data_use_tab_ui_manager_android.cc:28: static_cast<SessionID::id_type>(tab_id)); On 2015/11/30 22:33:38, sclittle wrote: > nit: Could you DCHECK that |tab_id| is a valid >=0 tab ID here? Done. https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:105: AtMostOneDataUseSubmitRequest); On 2015/11/30 22:33:38, sclittle wrote: > nit: I don't think this test exists anymore Done. https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/android... chrome/browser/android/data_usage/external_data_use_observer.h:116: FRIEND_TEST_ALL_PREFIXES(DataUseUITabModelTest, ReportTabEventsTest); On 2015/11/30 22:33:38, sclittle wrote: > nit: This should alphabetically come before the ExternalDataUseObserverTest > macros, since it's DataUseUITabModelTest. Done. https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/600001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:109: if (!io_thread || !io_thread->globals()) On 2015/11/30 22:33:38, sclittle wrote: > nit: It looks like |io_thread| and |io_thread->globals()| are guaranteed to > exist here, so you can remove this if statement. Done.
https://codereview.chromium.org/1443683002/diff/620001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/620001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:237: if (data_use_ui_tab_model && !io_data_->IsOffTheRecord()) { The off the record check isn't actually needed - I meant to point out that this file is only for on the record events, in case you thought you were gathering everything. https://codereview.chromium.org/1443683002/diff/620001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:244: data_use_ui_tab_model->GetWeakPtr())); Seems like we're micromanaging DataUseUITabModel's internal state and setup here. None of this code even cares about ProfileIOData. Could we just add a call to DataUseUITabModelFactory::GetInstance() in chrome_browser_main_extra_parts_profiles.cc, have it grab the IOThread pointer there, and do all this setup itself in its constructor? Instantiating the service should guarantee it gets created. Also means we wouldn't have to expose weak pointers to the world, which is generally a good thing to not do. You'll need to be careful about handling off the record correctly.
PTAL. Thanks. https://codereview.chromium.org/1443683002/diff/620001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1443683002/diff/620001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:237: if (data_use_ui_tab_model && !io_data_->IsOffTheRecord()) { On 2015/12/01 16:45:49, mmenke wrote: > The off the record check isn't actually needed - I meant to point out that this > file is only for on the record events, in case you thought you were gathering > everything. Got it. I previously verified that ProfileImplIOData was constructed with REGULAR_PROFILE. So, I thought you just want me to add an additional check. https://codereview.chromium.org/1443683002/diff/620001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:244: data_use_ui_tab_model->GetWeakPtr())); On 2015/12/01 16:45:49, mmenke wrote: > Seems like we're micromanaging DataUseUITabModel's internal state and setup > here. None of this code even cares about ProfileIOData. > > Could we just add a call to DataUseUITabModelFactory::GetInstance() in > chrome_browser_main_extra_parts_profiles.cc, have it grab the IOThread pointer > there, and do all this setup itself in its constructor? Instantiating the > service should guarantee it gets created. > > Also means we wouldn't have to expose weak pointers to the world, which is > generally a good thing to not do. > > You'll need to be careful about handling off the record correctly. Done.
LGTM https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:234: #else nit: A not insignificant subset of these are in alphabetical order (Some by namespace, some by class name), while others are randomly strewn about. Maybe move this up with the other D's (DomDistillerServiceFactory and friends) or with the C's, adding a new ifdef block?
https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:81: base::WeakPtr<DataUseUITabModel> GetWeakPtr(); Oh, can we get rid of this? Best not to expose these to the world.
https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:81: base::WeakPtr<DataUseUITabModel> GetWeakPtr(); On 2015/12/01 20:27:31, mmenke wrote: > Oh, can we get rid of this? Best not to expose these to the world. It is still used in data_use_ui_tab_model_factory.cc. https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/1443683002/diff/640001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:234: #else On 2015/12/01 20:21:45, mmenke wrote: > nit: A not insignificant subset of these are in alphabetical order (Some by > namespace, some by class name), while others are randomly strewn about. Maybe > move this up with the other D's (DomDistillerServiceFactory and friends) or with > the C's, adding a new ifdef block? Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rajendrant@chromium.org, sclittle@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1443683002/#ps660001 (title: "Addressed mmenke comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1443683002/660001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1443683002/660001
Patchset #19 (id:680001) has been deleted
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rajendrant@chromium.org, mmenke@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/1443683002/#ps700001 (title: "Remove unnecessary thread checks in the factory class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1443683002/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1443683002/700001
Message was sent while issue was closed.
Description was changed from ========== Notify DataUseTabModel of navigations and tab closures. package name will be later plumbed through a separate function call. BUG=552131, 552133 ========== to ========== Notify DataUseTabModel of navigations and tab closures. package name will be later plumbed through a separate function call. BUG=552131, 552133 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:700001)
Message was sent while issue was closed.
Description was changed from ========== Notify DataUseTabModel of navigations and tab closures. package name will be later plumbed through a separate function call. BUG=552131, 552133 ========== to ========== Notify DataUseTabModel of navigations and tab closures. package name will be later plumbed through a separate function call. BUG=552131, 552133 Committed: https://crrev.com/224e4f0ba66c17fe3776976f476b87fdb85adaf3 Cr-Commit-Position: refs/heads/master@{#362582} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/224e4f0ba66c17fe3776976f476b87fdb85adaf3 Cr-Commit-Position: refs/heads/master@{#362582} |
