Remove one thread hop while fetching matching rules

Remove one thread hop while fetching matching rules

When matching rules are fetched in DataUseTabModel, it involves one
thread hop from UI thread to ExternalDataUseObserver in the IO thread.
Remove this thread hop by DataUseTabModel directly having a pointer to
the ExternalDataUseObserverBridge.

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

[Raj, 2016-03-08 06:33:07]

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

[Raj, 2016-03-08 06:33:07]

PTAL.
I will add new unittests specific to new changes.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.cc:28: const
ExternalDataUseObserverBridge* external_data_use_observer_bridge,
ExternalDataUseObserverBridge lives in UI thread, and avoids one UI-IO thread
hop while fetching rules. I measured that the thread hop for the initial rule
fetch accounted for ~50ms.

[tbansal1, 2016-03-08 18:15:01]

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.cc:28: const
ExternalDataUseObserverBridge* external_data_use_observer_bridge,
On 2016/03/08 06:33:07, Raj wrote:
> ExternalDataUseObserverBridge lives in UI thread, and avoids one UI-IO thread
> hop while fetching rules. I measured that the thread hop for the initial rule
> fetch accounted for ~50ms.

I am confused. Is this related to the buffering of navigation events? For my own
convenience :), I would suggest splitting into 3 CLs:
1) Buffering + Processing of UI events.
2) Fetching the matching rules when the control app is installed.
3) Removing one thread-hop by passing Bridge instead of EDUO. Also, can you not
skip this thread hop by unconditionally fetching the matching rules in
Bridge::Init() method.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/data_use_matcher.h (left):

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.h:16: #include
"base/threading/thread_checker.h"
Why was this include removed?

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.h:139: //
ExternalDataUseObserver. DataUseTabModel(owner of |this|) and
nit: space before (

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:175: bool
is_package_match;
May be declare these variables after the if-block since they are not used in the
if-block.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:179:
DataUseUINavigationBuffer ui_event;
If you want to force callers to fill all the fields of the struct, I would
suggest adding a constructor to the struct.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:186: if
(ui_navigation_buffer->size() >= kDefaultMaxNavigationEventsBuffered) {
If the matching rules have not been fetched, what would be effect of this? It
seems that since there are no matching rules, none of the tabs will enter
tracking, and so processing would not have any effect.

May be add a comment to say what is expected to happen here.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:291: if
(!is_control_app_installed_) {  // Clear rules.
nit: move the comment on next line?

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:295:
data_use_matcher_->FetchMatchingRules();
Can we do this in a separate CL and merge it to M-50?

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:482: if
(!ui_navigation_buffer)
DCHECK (thread_checker_...)

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:484:
scoped_ptr<std::vector<DataUseUINavigationBuffer>> tmp_ui_navigation_buffer =
Why move it to a tmp vector? Can we not iterate over the original vector, and
then empty/delete it afterwards?

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:88: // pointer to
ExternalDataUseObserverBridge object.
say more about ownership and lifetime: It seems the caller owns the bridge
object, but I am not sure if the bridge can get destroyed before |this|?

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:198: struct
DataUseUINavigationBuffer {
This name seems incorrect. This is not a buffer/collection, more like a single
event.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:200: TransitionType
transition;
s/transition/transition_type/

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:202: std::string package;
nit: may be add comments for the fields. e.g., is the package name of the
package that started this navigation?

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:273:
scoped_ptr<std::vector<DataUseUINavigationBuffer>> ui_navigation_buffer;
variable name should end with underscore.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:273:
scoped_ptr<std::vector<DataUseUINavigationBuffer>> ui_navigation_buffer;
s/ui_navigation_buffer/data_use_ui_navigations_/?

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:273:
scoped_ptr<std::vector<DataUseUINavigationBuffer>> ui_navigation_buffer;
say more about this buffer: what is the order in which events are stored. It
seems they are stored in FIFO?

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:273:
scoped_ptr<std::vector<DataUseUINavigationBuffer>> ui_navigation_buffer;
Also, why does this need to be a scoped_ptr, and not a regular variable
std::vector<...> ui_navigation_buffer_;

May be add another variable
bool buffer_navigations_
instead of overriding this variable.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:95: new
TestExternalDataUseObserverBridge());
Would this not cause memory leak in the test? It seems that the Bridge object is
never destroyed?

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:97: friend class
DataUseUITabModelTest;
alphabetical order.

[Raj, 2016-03-09 02:22:19]

Description was changed from

==========
Race condition with matching rule fetching and Data use tracking start

Initial UI navigation events are buffered until one of the following
happens:
 * rule fetch completes successfully or unsuccessfully.
 * control app not installed callback was received.
 * maximum limit is reached for the buffer.

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

to

==========
Remove one thread hop while fetching matching rules

When matching rules are fetched in DataUseTabModel, it involves one
thread hop from UI thread to ExternalDataUseObserver in the IO thread.
Remove this thread hop by DataUseTabModel directly having a pointer to
the ExternalDataUseObserverBridge.

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

[Raj, 2016-03-09 02:27:36]

Thanks for taking a look.

This CL is for removing one thread hop while fetching matching rules.
Will create separate CL for buffering UI navigations.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.cc:28: const
ExternalDataUseObserverBridge* external_data_use_observer_bridge,
On 2016/03/08 18:15:00, tbansal1 wrote:
> On 2016/03/08 06:33:07, Raj wrote:
> > ExternalDataUseObserverBridge lives in UI thread, and avoids one UI-IO
thread
> > hop while fetching rules. I measured that the thread hop for the initial
rule
> > fetch accounted for ~50ms.
> 
> I am confused. Is this related to the buffering of navigation events? For my
own
> convenience :), I would suggest splitting into 3 CLs:
> 1) Buffering + Processing of UI events.
> 2) Fetching the matching rules when the control app is installed.
> 3) Removing one thread-hop by passing Bridge instead of EDUO. Also, can you
not
> skip this thread hop by unconditionally fetching the matching rules in
> Bridge::Init() method.

Sorry. Will split into 3 CLs.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/data_use_matcher.h (left):

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.h:16: #include
"base/threading/thread_checker.h"
On 2016/03/08 18:15:00, tbansal1 wrote:
> Why was this include removed?

Done.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.h:139: //
ExternalDataUseObserver. DataUseTabModel(owner of |this|) and
On 2016/03/08 18:15:00, tbansal1 wrote:
> nit: space before (

Done.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:291: if
(!is_control_app_installed_) {  // Clear rules.
On 2016/03/08 18:15:00, tbansal1 wrote:
> nit: move the comment on next line?

Done.

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.cc:295:
data_use_matcher_->FetchMatchingRules();
On 2016/03/08 18:15:00, tbansal1 wrote:
> Can we do this in a separate CL and merge it to M-50?

Done.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model.h:88: // pointer to
ExternalDataUseObserverBridge object.
On 2016/03/08 18:15:01, tbansal1 wrote:
> say more about ownership and lifetime: It seems the caller owns the bridge
> object, but I am not sure if the bridge can get destroyed before |this|?

Done.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:95: new
TestExternalDataUseObserverBridge());
On 2016/03/08 18:15:01, tbansal1 wrote:
> Would this not cause memory leak in the test? It seems that the Bridge object
is
> never destroyed?

Done.

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

https://codereview.chromium.org/1772273002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:97: friend class
DataUseUITabModelTest;
On 2016/03/08 18:15:01, tbansal1 wrote:
> alphabetical order.

Done.

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.cc:284: void
ExternalDataUseObserver::OnFetchMatchingRulesDone() {
I am more inclined to move the 15 minute rule fetch logic to DataUseTabModel.
Then:
 * last_matching_rules_fetch_time_ will be directly updated in tab model.
 * Fetch can happen based on whether control app is installed or not.
 * all the fetch logic will be in one place.
WDYT.

[tbansal1, 2016-03-10 01:00:58]

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher.h:142: // destroyed in the
order.
s/the/that/

May be add a explicit comment that this is guaranteed to be non-null because....

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:16: #include
"base/memory/ref_counted.h"
Is this include still needed?

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.cc:142:
external_data_use_observer_bridge_(new ExternalDataUseObserverBridge()),
change the order so that bridge is constructed before tab model.

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.cc:284: void
ExternalDataUseObserver::OnFetchMatchingRulesDone() {
On 2016/03/09 02:27:36, Raj wrote:
> I am more inclined to move the 15 minute rule fetch logic to DataUseTabModel.
> Then:
>  * last_matching_rules_fetch_time_ will be directly updated in tab model.
>  * Fetch can happen based on whether control app is installed or not.
>  * all the fetch logic will be in one place.
> WDYT.

As discussed offline, I would suggest thinking of how you can re-architecture
all the classes so that it is easier to handle different kinds of data use
tracking. If DataUseTabModel is going to contain one model of data use tracking,
then probably it should not be doing general stuff.

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:133:
FROM_HERE, base::Bind(&ExternalDataUseObserver::OnFetchMatchingRulesDone,
Why is this callback needed?  This is not preventing 2 parallel calls to the
Java class because not all FetchMatchingRules() come from EDUO.

[Raj, 2016-03-10 02:13:34]

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher.h:142: // destroyed in the
order.
On 2016/03/10 01:00:57, tbansal1 wrote:
> s/the/that/
> 
> May be add a explicit comment that this is guaranteed to be non-null
because....

Done.

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:16: #include
"base/memory/ref_counted.h"
On 2016/03/10 01:00:57, tbansal1 wrote:
> Is this include still needed?

Done.

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.cc:142:
external_data_use_observer_bridge_(new ExternalDataUseObserverBridge()),
On 2016/03/10 01:00:58, tbansal1 wrote:
> change the order so that bridge is constructed before tab model.

Done.

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.cc:284: void
ExternalDataUseObserver::OnFetchMatchingRulesDone() {
On 2016/03/10 01:00:58, tbansal1 wrote:
> On 2016/03/09 02:27:36, Raj wrote:
> > I am more inclined to move the 15 minute rule fetch logic to
DataUseTabModel.
> > Then:
> >  * last_matching_rules_fetch_time_ will be directly updated in tab model.
> >  * Fetch can happen based on whether control app is installed or not.
> >  * all the fetch logic will be in one place.
> > WDYT.
> 
> As discussed offline, I would suggest thinking of how you can re-architecture
> all the classes so that it is easier to handle different kinds of data use
> tracking. If DataUseTabModel is going to contain one model of data use
tracking,
> then probably it should not be doing general stuff.

Acknowledged.

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

https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:133:
FROM_HERE, base::Bind(&ExternalDataUseObserver::OnFetchMatchingRulesDone,
On 2016/03/10 01:00:58, tbansal1 wrote:
> Why is this callback needed?  This is not preventing 2 parallel calls to the
> Java class because not all FetchMatchingRules() come from EDUO.

This callback updates the last_matching_rules_fetch_time_ in EDUO, which is
relied on in test MatchingRuleFetchOnControlAppInstall to identify whether rule
fetch is invoked.
I have changed the test to look at UMA to determine rule fetch was invoked.

[tbansal1, 2016-03-10 17:32:04]

On 2016/03/10 02:13:34, Raj wrote:
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/data_usage/data_use_matcher.h (right):
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> chrome/browser/android/data_usage/data_use_matcher.h:142: // destroyed in the
> order.
> On 2016/03/10 01:00:57, tbansal1 wrote:
> > s/the/that/
> > 
> > May be add a explicit comment that this is guaranteed to be non-null
> because....
> 
> Done.
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/data_usage/data_use_tab_model.h (right):
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> chrome/browser/android/data_usage/data_use_tab_model.h:16: #include
> "base/memory/ref_counted.h"
> On 2016/03/10 01:00:57, tbansal1 wrote:
> > Is this include still needed?
> 
> Done.
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/data_usage/external_data_use_observer.cc (right):
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> chrome/browser/android/data_usage/external_data_use_observer.cc:142:
> external_data_use_observer_bridge_(new ExternalDataUseObserverBridge()),
> On 2016/03/10 01:00:58, tbansal1 wrote:
> > change the order so that bridge is constructed before tab model.
> 
> Done.
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> chrome/browser/android/data_usage/external_data_use_observer.cc:284: void
> ExternalDataUseObserver::OnFetchMatchingRulesDone() {
> On 2016/03/10 01:00:58, tbansal1 wrote:
> > On 2016/03/09 02:27:36, Raj wrote:
> > > I am more inclined to move the 15 minute rule fetch logic to
> DataUseTabModel.
> > > Then:
> > >  * last_matching_rules_fetch_time_ will be directly updated in tab model.
> > >  * Fetch can happen based on whether control app is installed or not.
> > >  * all the fetch logic will be in one place.
> > > WDYT.
> > 
> > As discussed offline, I would suggest thinking of how you can
re-architecture
> > all the classes so that it is easier to handle different kinds of data use
> > tracking. If DataUseTabModel is going to contain one model of data use
> tracking,
> > then probably it should not be doing general stuff.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/data_usage/external_data_use_observer_bridge.cc
> (right):
> 
>
https://codereview.chromium.org/1772273002/diff/20001/chrome/browser/android/...
> chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:133:
> FROM_HERE, base::Bind(&ExternalDataUseObserver::OnFetchMatchingRulesDone,
> On 2016/03/10 01:00:58, tbansal1 wrote:
> > Why is this callback needed?  This is not preventing 2 parallel calls to the
> > Java class because not all FetchMatchingRules() come from EDUO.
> 
> This callback updates the last_matching_rules_fetch_time_ in EDUO, which is
> relied on in test MatchingRuleFetchOnControlAppInstall to identify whether
rule
> fetch is invoked.
> I have changed the test to look at UMA to determine rule fetch was invoked.

Forgot to say in the last round, the CL title needs to be updated.

[tbansal1, 2016-03-10 17:44:55]

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_bridge.cc
(right):

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:175:
data_use_tab_model_->OnControlAppInstallStateChange(is_control_app_installed);
Add:
if(!data_use_tab_model_)
   return;

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_bridge.h
(right):

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_bridge.h:109: //
|data_use_tab_model_| is notified of the matching rules on UI thread.
nit: Add a comment that |data_use_tab_model_| may be null.

[Raj, 2016-03-10 18:30:11]

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_bridge.cc
(right):

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:175:
data_use_tab_model_->OnControlAppInstallStateChange(is_control_app_installed);
On 2016/03/10 17:44:55, tbansal1 wrote:
> Add:
> if(!data_use_tab_model_)
>    return;

Done.

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_bridge.h
(right):

https://codereview.chromium.org/1772273002/diff/40001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_bridge.h:109: //
|data_use_tab_model_| is notified of the matching rules on UI thread.
On 2016/03/10 17:44:55, tbansal1 wrote:
> nit: Add a comment that |data_use_tab_model_| may be null.

Done.

[tbansal1, 2016-03-10 19:37:58]

lgtm % two nits. One below. And, second: The CL title needs to be updated to
"Remove one thread hop while fetching matching rules"

https://codereview.chromium.org/1772273002/diff/60001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_bridge.h
(right):

https://codereview.chromium.org/1772273002/diff/60001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_bridge.h:110: //
|data_use_tab_model_| may be null if Init has not been called yet.
|data_use_tab_model_| could also be null because it was destroyed (since it gets
destroyed before |this|...right?).

I think it is simpler to just  say that |data_use_tab_model_| may be null.

[Raj, 2016-03-10 19:50:07]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-03-10 19:50:08]

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/1772273002/#ps80001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-03-10 19:50:35]

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

[commit-bot: I haz the power, 2016-03-10 20:56:32]

Description was changed from

==========
Remove one thread hop while fetching matching rules

When matching rules are fetched in DataUseTabModel, it involves one
thread hop from UI thread to ExternalDataUseObserver in the IO thread.
Remove this thread hop by DataUseTabModel directly having a pointer to
the ExternalDataUseObserverBridge.

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

to

==========
Remove one thread hop while fetching matching rules

When matching rules are fetched in DataUseTabModel, it involves one
thread hop from UI thread to ExternalDataUseObserver in the IO thread.
Remove this thread hop by DataUseTabModel directly having a pointer to
the ExternalDataUseObserverBridge.

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

[commit-bot: I haz the power, 2016-03-10 20:56:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-10 20:58:41]

Description was changed from

==========
Remove one thread hop while fetching matching rules

When matching rules are fetched in DataUseTabModel, it involves one
thread hop from UI thread to ExternalDataUseObserver in the IO thread.
Remove this thread hop by DataUseTabModel directly having a pointer to
the ExternalDataUseObserverBridge.

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

to

==========
Remove one thread hop while fetching matching rules

When matching rules are fetched in DataUseTabModel, it involves one
thread hop from UI thread to ExternalDataUseObserver in the IO thread.
Remove this thread hop by DataUseTabModel directly having a pointer to
the ExternalDataUseObserverBridge.

BUG=586235

Committed: https://crrev.com/f2143600e425715839c76794abcf5021f4ca8e1b
Cr-Commit-Position: refs/heads/master@{#380460}
==========

[commit-bot: I haz the power, 2016-03-10 20:58:42]

Patchset 5 (id:??) landed as
https://crrev.com/f2143600e425715839c76794abcf5021f4ca8e1b
Cr-Commit-Position: refs/heads/master@{#380460}