[ios] Wait for sync query to return before displaying history

[ios] Wait for sync query to return before displaying history

If sync history is on, wait for WebHistoryService to complete its query
before displaying history. This prevents the history panel from
displaying no history or only local history just after the user switches
sync accounts.

BUG=589277
TEST=Have multiple sync accounts with a large number of synced entries.
       Switch sync accounts and immediately open history. A loading
       indicator should be displayed until sync history is presented.
     Turn off network connection and open history. Only local history
       should be displayed.
     Re-enable network connectivity and turn off history sync while
       leaving other forms of sync on. Open history. Only local history
       should be displayed.
     Re-enable history sync. Synced history should be displayed.

Committed: https://crrev.com/4bbf08e203ec86ba88247bbc9392a895ded82939
Cr-Commit-Position: refs/heads/master@{#439946}

[Jackie Quinn, 2016-12-20 19:16:38]

jyquinn@chromium.org changed reviewers:
+ lpromero@chromium.org

[sczs, 2016-12-20 22:49:48]

sczs@chromium.org changed reviewers:
+ sczs@chromium.org

[sczs, 2016-12-20 22:49:48]

https://codereview.chromium.org/2589313002/diff/20001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:327:
!result.web_history_returned) {
Should this be !result.sync_returned ?!

https://codereview.chromium.org/2589313002/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:658:
NSIndexPath* indexPath = [NSIndexPath indexPathForItem:0 inSection:0];
Could self.loading be used here instead of checking the cell subclass?

[Jackie Quinn, 2016-12-20 23:12:04]

https://codereview.chromium.org/2589313002/diff/20001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:327:
!result.web_history_returned) {
On 2016/12/20 22:49:48, sczs wrote:
> Should this be !result.sync_returned ?!

Ah yes thanks. Last minute variable name change 🙃

https://codereview.chromium.org/2589313002/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:658:
NSIndexPath* indexPath = [NSIndexPath indexPathForItem:0 inSection:0];
On 2016/12/20 22:49:48, sczs wrote:
> Could self.loading be used here instead of checking the cell subclass?

Not necessarily. Presence/absence of the loading indicator right now is not tied
to self.loading, so I think it's better to check for the actual item.

[lpromero, 2016-12-20 23:13:42]

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:664: // Do
not add indicator a sectond time.
second*

Can't you look instead in the model then ?

[Jackie Quinn, 2016-12-20 23:18:10]

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:664: // Do
not add indicator a sectond time.
On 2016/12/20 23:13:42, lpromero wrote:
> second*
> 
> Can't you look instead in the model then ?

Done. There is no ActivityIndicatorItem, only the cell.

[lpromero, 2016-12-20 23:26:23]

lgtm

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:664: // Do
not add indicator a sectond time.
On 2016/12/20 23:18:10, Jackie Quinn wrote:
> On 2016/12/20 23:13:42, lpromero wrote:
> > second*
> > 
> > Can't you look instead in the model then ?
> 
> Done. There is no ActivityIndicatorItem, only the cell.

That's where giving a specific type to the item can come in handy I guess. Would
it be worth it here?

[sczs, 2016-12-20 23:27:42]

lgtm

[Jackie Quinn, 2016-12-20 23:33:21]

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:664: // Do
not add indicator a sectond time.
On 2016/12/20 23:26:23, lpromero wrote:
> On 2016/12/20 23:18:10, Jackie Quinn wrote:
> > On 2016/12/20 23:13:42, lpromero wrote:
> > > second*
> > > 
> > > Can't you look instead in the model then ?
> > 
> > Done. There is no ActivityIndicatorItem, only the cell.
> 
> That's where giving a specific type to the item can come in handy I guess.
Would
> it be worth it here?

If the sole purpose is checking for the type of item, I feel like an extra class
is overkill here when you can just check the cellClass. If you feel differently
let me know.

[lpromero, 2016-12-20 23:38:51]

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:664: // Do
not add indicator a sectond time.
On 2016/12/20 23:33:21, Jackie Quinn wrote:
> On 2016/12/20 23:26:23, lpromero wrote:
> > On 2016/12/20 23:18:10, Jackie Quinn wrote:
> > > On 2016/12/20 23:13:42, lpromero wrote:
> > > > second*
> > > > 
> > > > Can't you look instead in the model then ?
> > > 
> > > Done. There is no ActivityIndicatorItem, only the cell.
> > 
> > That's where giving a specific type to the item can come in handy I guess.
> Would
> > it be worth it here?
> 
> If the sole purpose is checking for the type of item, I feel like an extra
class
> is overkill here when you can just check the cellClass. If you feel
differently
> let me know.

I meant just the enum value you pass to the initializer -initWithType:, not the
type in the sense of compiler. I agree that we shouldn't need a subclass.

[Jackie Quinn, 2016-12-21 00:02:05]

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/history_collection_view_controller.mm
(right):

https://codereview.chromium.org/2589313002/diff/40001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/history_collection_view_controller.mm:664: // Do
not add indicator a sectond time.
On 2016/12/20 23:38:51, lpromero wrote:
> On 2016/12/20 23:33:21, Jackie Quinn wrote:
> > On 2016/12/20 23:26:23, lpromero wrote:
> > > On 2016/12/20 23:18:10, Jackie Quinn wrote:
> > > > On 2016/12/20 23:13:42, lpromero wrote:
> > > > > second*
> > > > > 
> > > > > Can't you look instead in the model then ?
> > > > 
> > > > Done. There is no ActivityIndicatorItem, only the cell.
> > > 
> > > That's where giving a specific type to the item can come in handy I guess.
> > Would
> > > it be worth it here?
> > 
> > If the sole purpose is checking for the type of item, I feel like an extra
> class
> > is overkill here when you can just check the cellClass. If you feel
> differently
> > let me know.
> 
> I meant just the enum value you pass to the initializer -initWithType:, not
the
> type in the sense of compiler. I agree that we shouldn't need a subclass.

Oh, I see. Yeah that sounds reasonable. Updated!

[Jackie Quinn, 2016-12-21 00:02:17]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2016-12-21 00:02:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sczs@chromium.org,
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2589313002/#ps80001
(title: "Use item types!")

[commit-bot: I haz the power, 2016-12-21 00:03:17]

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-21 00:16:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482278537008300,
"parent_rev": "51f36e9353888eeecc9da06a87810e531dd7bc0f", "commit_rev":
"9467bfc6839828d8096cb4cec35c5b8ced41fcc8"}

[commit-bot: I haz the power, 2016-12-21 00:16:30]

Description was changed from

==========
[ios] Wait for sync query to return before displaying history

If sync history is on, wait for WebHistoryService to complete its query
before displaying history. This prevents the history panel from
displaying no history or only local history just after the user switches
sync accounts.

BUG=589277
TEST=Have multiple sync accounts with a large number of synced entries.
       Switch sync accounts and immediately open history. A loading
       indicator should be displayed until sync history is presented.
     Turn off network connection and open history. Only local history
       should be displayed.
     Re-enable network connectivity and turn off history sync while
       leaving other forms of sync on. Open history. Only local history
       should be displayed.
     Re-enable history sync. Synced history should be displayed.
==========

to

==========
[ios] Wait for sync query to return before displaying history

If sync history is on, wait for WebHistoryService to complete its query
before displaying history. This prevents the history panel from
displaying no history or only local history just after the user switches
sync accounts.

BUG=589277
TEST=Have multiple sync accounts with a large number of synced entries.
       Switch sync accounts and immediately open history. A loading
       indicator should be displayed until sync history is presented.
     Turn off network connection and open history. Only local history
       should be displayed.
     Re-enable network connectivity and turn off history sync while
       leaving other forms of sync on. Open history. Only local history
       should be displayed.
     Re-enable history sync. Synced history should be displayed.

Review-Url: https://codereview.chromium.org/2589313002
==========

[commit-bot: I haz the power, 2016-12-21 00:16:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-21 00:20:29]

Description was changed from

==========
[ios] Wait for sync query to return before displaying history

If sync history is on, wait for WebHistoryService to complete its query
before displaying history. This prevents the history panel from
displaying no history or only local history just after the user switches
sync accounts.

BUG=589277
TEST=Have multiple sync accounts with a large number of synced entries.
       Switch sync accounts and immediately open history. A loading
       indicator should be displayed until sync history is presented.
     Turn off network connection and open history. Only local history
       should be displayed.
     Re-enable network connectivity and turn off history sync while
       leaving other forms of sync on. Open history. Only local history
       should be displayed.
     Re-enable history sync. Synced history should be displayed.

Review-Url: https://codereview.chromium.org/2589313002
==========

to

==========
[ios] Wait for sync query to return before displaying history

If sync history is on, wait for WebHistoryService to complete its query
before displaying history. This prevents the history panel from
displaying no history or only local history just after the user switches
sync accounts.

BUG=589277
TEST=Have multiple sync accounts with a large number of synced entries.
       Switch sync accounts and immediately open history. A loading
       indicator should be displayed until sync history is presented.
     Turn off network connection and open history. Only local history
       should be displayed.
     Re-enable network connectivity and turn off history sync while
       leaving other forms of sync on. Open history. Only local history
       should be displayed.
     Re-enable history sync. Synced history should be displayed.

Committed: https://crrev.com/4bbf08e203ec86ba88247bbc9392a895ded82939
Cr-Commit-Position: refs/heads/master@{#439946}
==========

[commit-bot: I haz the power, 2016-12-21 00:20:30]

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