Updated tab history classes to use NavigationItemLists.

Updated tab history classes to use NavigationItemLists.

This CL also updates these classes to use modern Objective-C syntax
and to follow the style guide (i.e. no property accessors in init).

BUG=454984, 546355
Review-Url: https://codereview.chromium.org/2693013005
Cr-Commit-Position: refs/heads/master@{#452241}
Committed: https://chromium.googlesource.com/chromium/src/+/0d966a54af810d7328692553e9bfc21bda33a990

[kkhorimoto, 2017-02-14 22:46:50]

kkhorimoto@chromium.org changed reviewers:
+ eugenebut@chromium.org

[kkhorimoto, 2017-02-14 22:46:50]

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/web_toolbar_controller.mm (right):

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/web_toolbar_controller.mm:833: CGRect
containerFrame = [[_tabHistoryPopupController popupContainer] frame];
This was moved from TabHistoryPopupController's initializer to avoid accessing
|-popupContainer| in its initializer.  Also, it doesn't really make sense for
the initializer to call the fade in code.

[Eugene But (OOO till 7-30), 2017-02-14 23:08:08]

lgtm

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

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:108:
std::string longURL =
s/longURL/long_url

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/tab_history_view_controller.h (right):

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_view_controller.h:16: -
(instancetype)initWithItems:(const web::NavigationItemList&)items;
nit: NS_DESIGNATED_INITIALIZER ?

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

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_view_controller.mm:245:
TabHistoryViewControllerLayout* layout =
Do you want to move this inside |if ((self =| block?

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_view_controller.mm:249: [self
partitionItemsByHost:items];
We should not call instance methods inside init. The reason is the same why we
should not access properties.

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_view_controller.mm:319: size_t
sectionIdx = static_cast<size_t>(section);
s/sectionIdx/sectionIndex 

Objective-C Style Guide discourages abbreviations like this.

[kkhorimoto, 2017-02-14 23:23:52]

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

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:108:
std::string longURL =
On 2017/02/14 23:08:08, Eugene But wrote:
> s/longURL/long_url

Done.

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/tab_history_view_controller.h (right):

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_view_controller.h:16: -
(instancetype)initWithItems:(const web::NavigationItemList&)items;
On 2017/02/14 23:08:08, Eugene But wrote:
> nit: NS_DESIGNATED_INITIALIZER ?

Done.

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

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_view_controller.mm:245:
TabHistoryViewControllerLayout* layout =
On 2017/02/14 23:08:08, Eugene But wrote:
> Do you want to move this inside |if ((self =| block?

It's passed to |initWithCollectionViewLayout|.

https://codereview.chromium.org/2693013005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_view_controller.mm:249: [self
partitionItemsByHost:items];
On 2017/02/14 23:08:08, Eugene But wrote:
> We should not call instance methods inside init. The reason is the same why we
> should not access properties.

Changed to C function.

[kkhorimoto, 2017-02-15 00:10:16]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-15 00:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2693013005/#ps60001
(title: "Eugene's comments")

[commit-bot: I haz the power, 2017-02-15 00:11:41]

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

[kkhorimoto, 2017-02-15 00:21:14]

The CQ bit was unchecked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-15 00:24:34]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-15 00:24:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2693013005/#ps80001
(title: "comment fix")

[commit-bot: I haz the power, 2017-02-15 00:25:52]

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

[kkhorimoto, 2017-02-15 01:05:19]

The CQ bit was unchecked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-15 23:27:58]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-15 23:27:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2693013005/#ps100001
(title: "fixed egtest")

[commit-bot: I haz the power, 2017-02-15 23:28:53]

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, 2017-02-15 23:38:10]

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

[commit-bot: I haz the power, 2017-02-15 23:38:11]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kkhorimoto, 2017-02-16 00:23:05]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-16 00:23:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2693013005/#ps120001
(title: "update DEPS, include url_formatter in BUILD.gn")

[commit-bot: I haz the power, 2017-02-16 00:24:15]

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, 2017-02-16 01:46:36]

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

[commit-bot: I haz the power, 2017-02-16 01:46:38]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kkhorimoto, 2017-02-17 19:46:41]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-17 19:48:10]

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, 2017-02-17 20:34:36]

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

[commit-bot: I haz the power, 2017-02-17 20:34:38]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[kkhorimoto, 2017-02-17 22:41:47]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-17 22:42:47]

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, 2017-02-18 00:45:43]

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

[commit-bot: I haz the power, 2017-02-18 00:45:45]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[kkhorimoto, 2017-02-21 19:49:23]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-21 19:50:13]

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, 2017-02-21 21:53:02]

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

[commit-bot: I haz the power, 2017-02-21 21:53:03]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[kkhorimoto, 2017-02-21 23:13:50]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:14:52]

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, 2017-02-22 01:19:08]

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

[commit-bot: I haz the power, 2017-02-22 01:19:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[kkhorimoto, 2017-02-22 19:18:36]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-22 19:19:34]

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, 2017-02-22 22:38:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487791116329240,
"parent_rev": "a77b53baca5f2a4ff102592c17c8ea3f53dd5307", "commit_rev":
"0d966a54af810d7328692553e9bfc21bda33a990"}

[commit-bot: I haz the power, 2017-02-22 22:39:40]

Description was changed from

==========
Updated tab history classes to use NavigationItemLists.

This CL also updates these classes to use modern Objective-C syntax
and to follow the style guide (i.e. no property accessors in init).

BUG=454984, 546355
==========

to

==========
Updated tab history classes to use NavigationItemLists.

This CL also updates these classes to use modern Objective-C syntax
and to follow the style guide (i.e. no property accessors in init).

BUG=454984, 546355

Review-Url: https://codereview.chromium.org/2693013005
Cr-Commit-Position: refs/heads/master@{#452241}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d966a54af810d7328692553e9bf...
==========

[commit-bot: I haz the power, 2017-02-22 22:39:41]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/0d966a54af810d7328692553e9bf...

[lody, 2017-02-23 12:39:54]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2706403009/ by lod@chromium.org.

The reason for reverting is: Tests VisibleURLTestCase.testHistoryNavigation and
FormsTestCase.testRepostFormAfterIndexNavigation are broken ( see
https://bugs.chromium.org/p/chromium/issues/detail?id=695413 )..