[OfflinePages] Restarts immediate processing if stopped due to no net

[OfflinePages] Restarts immediate processing if stopped due to no net

Adds a ConnectionNotifier helper class to be able to learn when we get
connected and uses it in the Coordinator to try immediate start when
we get connected after stopping because we had no connection.

This is a memory-only network observer so only in effect if chrome stays
resident in memory.  Otherwise, we still have the GcmNetworkManager
scheduler as a fallback.

BUG=670119
Committed: https://crrev.com/7103c6cef4c8ed86a090e605e9817d1852452dc2
Cr-Commit-Position: refs/heads/master@{#436040}

[dougarnett, 2016-12-01 00:29:54]

The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 00:30:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2016-12-01 00:32:32]

dougarnett@chromium.org changed reviewers:
+ petewil@chromium.org

[dougarnett, 2016-12-01 00:32:33]

Here's the one-shot network observer approach I'm looking at.
Limited to immediate processing for now. No tests added yet.

[Pete Williamson, 2016-12-01 01:34:08]

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/connection_notifier.cc (right):

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
components/offline_pages/background/connection_notifier.cc:21: if (type !=
net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)
Might the RC also want to know about going offline?

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/connection_notifier.h (right):

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
components/offline_pages/background/connection_notifier.h:5: #ifndef
COMPONENTS_OFFLINE_PAGES_BACKGROUND_CONNECTION_NOTIFIER_H_
I think I recall you saying something about NQE also having a notifier.  If I
remember correctly, why chose this one over that one?  (Either is fine, I just
want to make sure we thought about it.)

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:563:
RequestConnectedEventForStarting();
Should we remove ourselves from the notifier when we no longer need the event?

[commit-bot: I haz the power, 2016-12-01 02:01:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 02:01:02]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-12-01 19:25:22]

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/connection_notifier.cc (right):

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
components/offline_pages/background/connection_notifier.cc:21: if (type !=
net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)
On 2016/12/01 01:34:08, Pete Williamson wrote:
> Might the RC also want to know about going offline?

I don't have a use for that yet. One idea is to know to stop processing when
connection goes away, but it may come back and the request can continue (without
killing its work in progress). So I think it may be a bit more complicated to
figure out how to leverage the offline transition.

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/connection_notifier.h (right):

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
components/offline_pages/background/connection_notifier.h:5: #ifndef
COMPONENTS_OFFLINE_PAGES_BACKGROUND_CONNECTION_NOTIFIER_H_
On 2016/12/01 01:34:08, Pete Williamson wrote:
> I think I recall you saying something about NQE also having a notifier.  If I
> remember correctly, why chose this one over that one?  (Either is fine, I just
> want to make sure we thought about it.)

I think there is no difference for what this class does currently - callback 
on next connection type seen. I did not see any difference yet in chattiness
between disconnecting and reconnecting (which was what I was wondering about).
The NQE can generate more events when you are connected and the connection
quality is determined to have changed. If/when we want to differentiate between
poor and non-poor connections, then we will want to use NQE here.

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2546433004/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:563:
RequestConnectedEventForStarting();
On 2016/12/01 01:34:08, Pete Williamson wrote:
> Should we remove ourselves from the notifier when we no longer need the event?

Yeah, I'll add as an else here - in case another path (like GcmNetworkManager)
found out about the network connection first. 

Otherwise, I do remove it in the RequestConnectedEventForStarting callback when
it fires.

[Pete Williamson, 2016-12-01 23:25:05]

lgtm

[dougarnett, 2016-12-01 23:41:48]

Added unit test coverage for both paths of starting a ConnectionNotifier

[dougarnett, 2016-12-01 23:42:45]

The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 23:43:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Pete Williamson, 2016-12-02 01:42:57]

https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:355: if
(connection_notifier_)
Why add this?  It will fire an event, right?  In theory, I would expect that we
shouldn't need to fire an event when setting up network conditions because we
have already set the network to be up or down.

[commit-bot: I haz the power, 2016-12-02 02:11:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 02:11:08]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-12-02 02:24:54]

https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:355: if
(connection_notifier_)
On 2016/12/02 01:42:57, Pete Williamson wrote:
> Why add this?  It will fire an event, right?  In theory, I would expect that
we
> shouldn't need to fire an event when setting up network conditions because we
> have already set the network to be up or down.

Yes, I want to be able to fire an event if there is an active notifier which
won't happen from our local use_test_connection_type_ and test_conneciton_type
members here that only work with GetConnectionType.

I could create another method to just do the event but I would like the test
state to also be consistent. I suppose I can call both then (if there are 2
methods) but this seemed attractive initially for changing the network state in
a test as a unit (firing an event if appropriate). Perhaps this is more of an
UpdateNetworkConditionsForTest()

Thoughts on how to proceed?

[Pete Williamson, 2016-12-02 17:50:14]

On 2016/12/02 02:24:54, dougarnett wrote:
>
https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
> File components/offline_pages/background/request_coordinator.h (right):
> 
>
https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
> components/offline_pages/background/request_coordinator.h:355: if
> (connection_notifier_)
> On 2016/12/02 01:42:57, Pete Williamson wrote:
> > Why add this?  It will fire an event, right?  In theory, I would expect that
> we
> > shouldn't need to fire an event when setting up network conditions because
we
> > have already set the network to be up or down.
> 
> Yes, I want to be able to fire an event if there is an active notifier which
> won't happen from our local use_test_connection_type_ and test_conneciton_type
> members here that only work with GetConnectionType.
> 
> I could create another method to just do the event but I would like the test
> state to also be consistent. I suppose I can call both then (if there are 2
> methods) but this seemed attractive initially for changing the network state
in
> a test as a unit (firing an event if appropriate). Perhaps this is more of an
> UpdateNetworkConditionsForTest()
> 
> Thoughts on how to proceed?

Since we don't need the event every time, I'd prefer a second method for the
event, and just call both when we need the event, but only one for the normal
case where we don't need an event.

[dougarnett, 2016-12-02 18:41:57]

The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 18:42:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2016-12-02 19:08:46]

https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2546433004/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:355: if
(connection_notifier_)
On 2016/12/02 02:24:54, dougarnett wrote:
> On 2016/12/02 01:42:57, Pete Williamson wrote:
> > Why add this?  It will fire an event, right?  In theory, I would expect that
> we
> > shouldn't need to fire an event when setting up network conditions because
we
> > have already set the network to be up or down.
> 
> Yes, I want to be able to fire an event if there is an active notifier which
> won't happen from our local use_test_connection_type_ and test_conneciton_type
> members here that only work with GetConnectionType.
> 
> I could create another method to just do the event but I would like the test
> state to also be consistent. I suppose I can call both then (if there are 2
> methods) but this seemed attractive initially for changing the network state
in
> a test as a unit (firing an event if appropriate). Perhaps this is more of an
> UpdateNetworkConditionsForTest()
> 
> Thoughts on how to proceed?

Added separate method in test class

[commit-bot: I haz the power, 2016-12-02 19:40:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 19:40:11]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-12-02 21:43:05]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-12-02 21:43:05]

The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2546433004/#ps60001
(title: "New test method: CallConnectionTypeObserver()")

[commit-bot: I haz the power, 2016-12-02 21:43:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 21:51:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480714985241640,
"parent_rev": "9f55fb4e31eab6a7e3157935d0dee3bc9b217d5d", "commit_rev":
"f7195f771ed53ec9a461ddc06ca0f417d3f24d0b"}

[commit-bot: I haz the power, 2016-12-02 21:52:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-02 21:53:57]

Description was changed from

==========
[OfflinePages] Restarts immediate processing if stopped due to no net

Adds a ConnectionNotifier helper class to be able to learn when we get
connected and uses it in the Coordinator to try immediate start when
we get connected after stopping because we had no connection.

This is a memory-only network observer so only in effect if chrome stays
resident in memory.  Otherwise, we still have the GcmNetworkManager
scheduler as a fallback.

BUG=670119
==========

to

==========
[OfflinePages] Restarts immediate processing if stopped due to no net

Adds a ConnectionNotifier helper class to be able to learn when we get
connected and uses it in the Coordinator to try immediate start when
we get connected after stopping because we had no connection.

This is a memory-only network observer so only in effect if chrome stays
resident in memory.  Otherwise, we still have the GcmNetworkManager
scheduler as a fallback.

BUG=670119

Committed: https://crrev.com/7103c6cef4c8ed86a090e605e9817d1852452dc2
Cr-Commit-Position: refs/heads/master@{#436040}
==========

[commit-bot: I haz the power, 2016-12-02 21:53:58]

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