Buffer UI navigation events in DataUseTabModel until rule fetch

Buffer UI navigation events in DataUseTabModel until rule fetch

Matching rules are fetched when Chrome starts and can take in the order
of 100-200ms. UI navigations that happen before these rules are fetched
will not start the data use tracking. This race condition is fixed by
buffering the navigation events until rule fetch is complete.

BUG=586235
Committed: https://crrev.com/557c920a034b0226b1edd307ae63995ee358644e
Cr-Commit-Position: refs/heads/master@{#383817}

[Raj, 2016-03-29 16:05:01]

rajendrant@chromium.org changed reviewers:
+ tbansal@chromium.org

[Raj, 2016-03-29 16:05:01]

ptal

[tbansal1, 2016-03-29 16:20:41]

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/data_use_tab_model.cc (right):

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:178:
data_use_ui_navigations_->push_back(ui_event);
Just push directly without creating temp. variable |ui_event|?

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/data_use_tab_model.h (right):

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:202: struct
DataUseUINavigationEvent {
Methods should come before fields.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:203: SessionID::id_type
tab_id;
can these fields be all const?

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:252: void
ProcessBufferedNavigationEvents();
function comments please.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:281: // Buffers the list
of UI navigation events that occurred until the first rule
s/Buffers the list of/Buffer of/?

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
(right):

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:415: //
Matching rules are fetched  when control app is installed.
extra space?

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:655:
TEST_F(ExternalDataUseObserverTest, ProcessBufferedNavigationEvents) {
 Seems like this is essentially 3 different tests. Can this be split into 3
different tests?

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:655:
TEST_F(ExternalDataUseObserverTest, ProcessBufferedNavigationEvents) {
This test does not really check if ProcessBufferedNavigations did something
useful or not. May be instead of checking the size(), you can see if
ProcessBufferedNavigations actually changed the state of the tab or not.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:699:
for (size_t count = 1; count < kDefaultMaxNavigationEventsBuffered;
this is flaky. If somebody changes value at one place, this test will fail. If
you want to know the value of this outside the class, pass this as a parameter
in the constructor of DataUseTabModel.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:700:
count++) {
++count

[Raj, 2016-03-29 18:03:21]

Thanks for reviewing.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/data_use_tab_model.cc (right):

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:178:
data_use_ui_navigations_->push_back(ui_event);
On 2016/03/29 16:20:41, tbansal1 wrote:
> Just push directly without creating temp. variable |ui_event|?

Done.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/data_use_tab_model.h (right):

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:202: struct
DataUseUINavigationEvent {
On 2016/03/29 16:20:41, tbansal1 wrote:
> Methods should come before fields.

Done.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:203: SessionID::id_type
tab_id;
On 2016/03/29 16:20:41, tbansal1 wrote:
> can these fields be all const?

Done.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:252: void
ProcessBufferedNavigationEvents();
On 2016/03/29 16:20:41, tbansal1 wrote:
> function comments please.

Done.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:281: // Buffers the list
of UI navigation events that occurred until the first rule
On 2016/03/29 16:20:41, tbansal1 wrote:
> s/Buffers the list of/Buffer of/?

Done.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
(right):

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:415: //
Matching rules are fetched  when control app is installed.
On 2016/03/29 16:20:41, tbansal1 wrote:
> extra space?

Done.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:655:
TEST_F(ExternalDataUseObserverTest, ProcessBufferedNavigationEvents) {
On 2016/03/29 16:20:41, tbansal1 wrote:
>  Seems like this is essentially 3 different tests. Can this be split into 3
> different tests?

Done.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:655:
TEST_F(ExternalDataUseObserverTest, ProcessBufferedNavigationEvents) {
On 2016/03/29 16:20:41, tbansal1 wrote:
> This test does not really check if ProcessBufferedNavigations did something
> useful or not. May be instead of checking the size(), you can see if
> ProcessBufferedNavigations actually changed the state of the tab or not.

I have added a unittest in data_use_tab_model_unittest.cc to check if tracking
starts for the buffered events.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:699:
for (size_t count = 1; count < kDefaultMaxNavigationEventsBuffered;
On 2016/03/29 16:20:41, tbansal1 wrote:
> this is flaky. If somebody changes value at one place, this test will fail. If
> you want to know the value of this outside the class, pass this as a parameter
> in the constructor of DataUseTabModel.

I do not want to add this limit as another parameter to DataUseTabModel. I have
used a higher number in the testfile and expecting that the buffer will get
cleared eventually at some point (not at an exact size limit). WDYT.

https://codereview.chromium.org/1837223002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:700:
count++) {
On 2016/03/29 16:20:41, tbansal1 wrote:
> ++count

Done.

[tbansal1, 2016-03-29 18:19:04]

lgtm % nits.

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_tab_model.h (right):

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:261: // and releases the
vector in |data_use_ui_navigations_| so that navigation
s/releases/deletes/

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:292: // Buffers of UI
navigation events that occurred until the first rule fetch is
s/Buffers/Buffer/
IIUC, this is a single buffer that contains multiple entries.

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
(right):

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:697:
for (size_t count = 1; count < kDefaultMaxNavigationEventsBuffered; ++count) {
start at 0?

[Raj, 2016-03-29 18:44:23]

Patchset #3 (id:40001) has been deleted

[Raj, 2016-03-29 19:01:19]

Patchset #3 (id:60001) has been deleted

[Raj, 2016-03-29 19:45:42]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-03-29 19:45:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/1837223002/#ps80001
(title: "Addressed nits")

[Raj, 2016-03-29 19:45:54]

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/data_use_tab_model.h (right):

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:261: // and releases the
vector in |data_use_ui_navigations_| so that navigation
On 2016/03/29 18:19:04, tbansal1 wrote:
> s/releases/deletes/

Done.

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:292: // Buffers of UI
navigation events that occurred until the first rule fetch is
On 2016/03/29 18:19:03, tbansal1 wrote:
> s/Buffers/Buffer/
> IIUC, this is a single buffer that contains multiple entries. 

Done.

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
(right):

https://codereview.chromium.org/1837223002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:697:
for (size_t count = 1; count < kDefaultMaxNavigationEventsBuffered; ++count) {
On 2016/03/29 18:19:04, tbansal1 wrote:
> start at 0?

Done.

[commit-bot: I haz the power, 2016-03-29 19:45:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837223002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837223002/80001

[Raj, 2016-03-29 20:46:33]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-03-29 20:46:33]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/1837223002/#ps40002
(title: "Addressed unittest fail")

[commit-bot: I haz the power, 2016-03-29 20:47:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1837223002/40002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1837223002/40002

[commit-bot: I haz the power, 2016-03-29 20:54:25]

Committed patchset #4 (id:40002)

[commit-bot: I haz the power, 2016-03-29 20:56:57]

Description was changed from

==========
Buffer UI navigation events in DataUseTabModel until rule fetch

Matching rules are fetched when Chrome starts and can take in the order
of 100-200ms. UI navigations that happen before these rules are fetched
will not start the data use tracking. This race condition is fixed by
buffering the navigation events until rule fetch is complete.

BUG=586235
==========

to

==========
Buffer UI navigation events in DataUseTabModel until rule fetch

Matching rules are fetched when Chrome starts and can take in the order
of 100-200ms. UI navigations that happen before these rules are fetched
will not start the data use tracking. This race condition is fixed by
buffering the navigation events until rule fetch is complete.

BUG=586235

Committed: https://crrev.com/557c920a034b0226b1edd307ae63995ee358644e
Cr-Commit-Position: refs/heads/master@{#383817}
==========

[commit-bot: I haz the power, 2016-03-29 20:56:58]

Patchset 4 (id:??) landed as
https://crrev.com/557c920a034b0226b1edd307ae63995ee358644e
Cr-Commit-Position: refs/heads/master@{#383817}