Updated CRWSessionController interface to use NavigationItems.

Updated CRWSessionController interface to use NavigationItems.

This CL introduces NavigationItem verions of CRWSessionEntry properties
and rewrites selectors to take NavigationItems.  The underlying logic
is still the same (i.e. NavigationItems are still owned by
CRWSessionEntries), and will be updated in a subsequent CL.

BUG=454984
Review-Url: https://codereview.chromium.org/2671773005
Cr-Commit-Position: refs/heads/master@{#448564}
Committed: https://chromium.googlesource.com/chromium/src/+/df9b48363d2759ccc531c354f7e57d9eea7a2935

[kkhorimoto, 2017-02-03 20:17:23]

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

[kkhorimoto, 2017-02-03 20:17:24]

Contains the naming changes from https://codereview.chromium.org/2672723003/

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

Thank you for splitting CL.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:748: web::NavigationItemList list;
nit: pass entries.count into constructor?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:750:
list.push_back(entry.navigationItem);
emplace_back ?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_entry_unittest.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_entry_unittest.mm:48:
base::scoped_nsobject<CRWSessionEntry> _sessionEntry;
If you want to fix this ivar then it should be session_entry_.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl_list.h (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl_list.h:24: ScopedNavigationItemImplList
CreateScopedNavigationItemImplList(
Is this used anywhere?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl_list.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl_list.mm:13: ScopedNavigationItemImplList
list;
Construct with size?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl_list.mm:17:
list.push_back(std::move(scoped_item_impl));
emplace_back ?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:89: [session_controller_
setBrowserState:browser_state];
Does it change the logic? Should this be in a separate CL?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_storage_builder.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:69:
web::ScopedNavigationItemList items;
Construct with size?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:75:
items.push_back(std::move(item_impl));
emplace_back?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:77: NSUInteger
currentIndex =
s/currentIndex/current_index

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
File ios/web/public/navigation_item_list.h (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.h:16: typedef std::vector<NavigationItem*>
NavigationItemList;
Is there a value in having this typedef? It does not make code much shorter or
clearer. To me it is misleading, because NavigationItemList is not a list and
when I see types like this I have to actually look into the header to see the
actual type.

Typedefs like this make code less clear and |auto| is a good way to address long
types for variables where necessary.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.h:19: typedef
std::vector<std::unique_ptr<NavigationItem>> ScopedNavigationItemList;
ditto

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
File ios/web/public/navigation_item_list.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.mm:11: NavigationItemList list;
Construct with size?

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.mm:13:
list.push_back(scoped_list[index].get());
emplace_back ?

[kkhorimoto, 2017-02-04 02:45:31]

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:748: web::NavigationItemList list;
On 2017/02/03 23:33:33, Eugene But wrote:
> nit: pass entries.count into constructor?

Done.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:750:
list.push_back(entry.navigationItem);
On 2017/02/03 23:33:33, Eugene But wrote:
> emplace_back ?

|emplace_back| passes its input parameters to the constructor of the vector item
class.  |list| contains raw pointers, so there is no constructor to use; this
simply copies the pointer value.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_entry_unittest.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_entry_unittest.mm:48:
base::scoped_nsobject<CRWSessionEntry> _sessionEntry;
On 2017/02/03 23:33:33, Eugene But wrote:
> If you want to fix this ivar then it should be session_entry_. 

Done.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl_list.h (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl_list.h:24: ScopedNavigationItemImplList
CreateScopedNavigationItemImplList(
On 2017/02/03 23:33:33, Eugene But wrote:
> Is this used anywhere?

It's used in my subsequent CL to convert the ScopedNavigationItemList to its
impl version in CRWSessionController's constructor.  I just added the entire
files in this CL for simplicity.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_item_impl_list.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl_list.mm:13: ScopedNavigationItemImplList
list;
On 2017/02/03 23:33:33, Eugene But wrote:
> Construct with size?

Done.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_item_impl_list.mm:17:
list.push_back(std::move(scoped_item_impl));
On 2017/02/03 23:33:33, Eugene But wrote:
> emplace_back ?

Judging by C++ documentation, it seems that using std::move() is more
performant.  Using emplace_back() would require calling release() on
|scoped_item_impl| and calling the constructor for the inserted item, whereas
std::move() uses some C++ rvalue reference magic to avoid constructing a new
object.  Additionally, from a self-documenting memory management perspective,
personally I feel that it's easier to think about "moving" a unique_ptr rather
than releasing one unique_ptr's value and creating a new unique_ptr with the
unowned pointer value.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.mm:89: [session_controller_
setBrowserState:browser_state];
On 2017/02/03 23:33:33, Eugene But wrote:
> Does it change the logic? Should this be in a separate CL?

Whoops, yes.  I'll move it to a separate CL.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_storage_builder.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:69:
web::ScopedNavigationItemList items;
On 2017/02/03 23:33:33, Eugene But wrote:
> Construct with size?

Done.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:75:
items.push_back(std::move(item_impl));
On 2017/02/03 23:33:33, Eugene But wrote:
> emplace_back?

It makes sense for BuildNavigationItemImpl() to return a unique_ptr so that it's
clear that the caller assumes ownership of the returned value, and for the same
reasons as in my other comment, I'm leaving this as a move rather than emplacing
a constructed unique_ptr from the released value.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_storage_builder.mm:77: NSUInteger
currentIndex =
On 2017/02/03 23:33:33, Eugene But wrote:
> s/currentIndex/current_index

Done.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
File ios/web/public/navigation_item_list.h (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.h:16: typedef std::vector<NavigationItem*>
NavigationItemList;
On 2017/02/03 23:33:33, Eugene But wrote:
> Is there a value in having this typedef? It does not make code much shorter or
> clearer. To me it is misleading, because NavigationItemList is not a list and
> when I see types like this I have to actually look into the header to see the
> actual type.
> 
> Typedefs like this make code less clear and |auto| is a good way to address
long
> types for variables where necessary.

This is mostly for convenience because the actual type is unwieldy to write all
the time (especially std::vector<std::unique_ptr<web::NavigationItemImpl>>). 
It's long enough that it makes formatting a nightmare.  I named it
NavigationItemList since it's a list (not an std::list, but a list in terms of
the software engineering concept
https://en.wikipedia.org/wiki/List_(abstract_data_type) ).  I suppose I can see
how it can be a little ambiguous since it isn't obvious from the name that it's
a list of NavigationItem pointers instead of the objects themselves, but I feel
that the slight ambiguity is outweighed by the code readability.  Moreover,
using auto (which would be necessary to have reasonable formatting in many
places) would introduce far more ambiguity than the one-time cost of learning
what the typedef means.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.h:16: typedef std::vector<NavigationItem*>
NavigationItemList;
Additionally, I modeled this typedef after RedirectList, which is a
std::vector<GURL> used all over the place in the history component.  I don't
think it's unreasonable to expect people writing code in a particular module, be
it ios/web/ or components/history, to familiarize themselves with associated
typedefs.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
File ios/web/public/navigation_item_list.mm (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.mm:11: NavigationItemList list;
On 2017/02/03 23:33:33, Eugene But wrote:
> Construct with size?

Done.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.mm:13:
list.push_back(scoped_list[index].get());
On 2017/02/03 23:33:33, Eugene But wrote:
> emplace_back ?

Acknowledged.

[Eugene But (OOO till 7-30), 2017-02-04 03:34:51]

Thanks! Everything look good, but I think those typedefs hurt readability. If
you disagree with my arguments then it would be great to have an offline
conversation so I can understand the benefits. But if you don't have a strong
opining and fine with removing typedefs, then lgtm.

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
File ios/web/public/navigation_item_list.h (right):

https://codereview.chromium.org/2671773005/diff/20001/ios/web/public/navigati...
ios/web/public/navigation_item_list.h:16: typedef std::vector<NavigationItem*>
NavigationItemList;
On 2017/02/04 02:45:31, kkhorimoto_ wrote:
> On 2017/02/03 23:33:33, Eugene But wrote:
> > Is there a value in having this typedef? It does not make code much shorter
or
> > clearer. To me it is misleading, because NavigationItemList is not a list
and
> > when I see types like this I have to actually look into the header to see
the
> > actual type.
> > 
> > Typedefs like this make code less clear and |auto| is a good way to address
> long
> > types for variables where necessary.
> 
> This is mostly for convenience because the actual type is unwieldy to write
all
> the time (especially std::vector<std::unique_ptr<web::NavigationItemImpl>>). 
> It's long enough that it makes formatting a nightmare.  I named it
> NavigationItemList since it's a list (not an std::list, but a list in terms of
> the software engineering concept
> https://en.wikipedia.org/wiki/List_(abstract_data_type) ).  I suppose I can
see
> how it can be a little ambiguous since it isn't obvious from the name that
it's
> a list of NavigationItem pointers instead of the objects themselves, but I
feel
> that the slight ambiguity is outweighed by the code readability.  Moreover,
> using auto (which would be necessary to have reasonable formatting in many
> places) would introduce far more ambiguity than the one-time cost of learning
> what the typedef means.
Shouldn't we optimize code for reading and not for writing? I believe this
typedef actually hurts readability. It is quite clear what
|std::vector<NavigationItem*| means while I had to look up what
NavigationItemList is.

std::vector<std::unique_ptr<web::NavigationItemImpl>> is really bad, but is it
the case that formatting can not be improved by using auto?
I could not find a place where ScopedNavigationItemImplList really improves
formatting, except CRWSessionController. But at this point should we really care
how CRWSessionController looks? It will be removed anyways.

https://codereview.chromium.org/2671773005/diff/40001/ios/web/public/navigati...
File ios/web/public/navigation_item_list.h (right):

https://codereview.chromium.org/2671773005/diff/40001/ios/web/public/navigati...
ios/web/public/navigation_item_list.h:23: NavigationItemList
CreateUnscopedNavigationItemList(
nit: How about CreateRawNavigationItemList?

[kkhorimoto, 2017-02-07 05:07:23]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-07 05:07:23]

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/2671773005/#ps60001
(title: "todo, unscoped => raw")

[commit-bot: I haz the power, 2017-02-07 05:07:44]

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-07 06:48:42]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486444043379720,
"parent_rev": "ff9c66f0c118f2cacfdaeda9e384eb2e47b33cdc", "commit_rev":
"df9b48363d2759ccc531c354f7e57d9eea7a2935"}

[commit-bot: I haz the power, 2017-02-07 06:50:03]

Description was changed from

==========
Updated CRWSessionController interface to use NavigationItems.

This CL introduces NavigationItem verions of CRWSessionEntry properties
and rewrites selectors to take NavigationItems.  The underlying logic
is still the same (i.e. NavigationItems are still owned by
CRWSessionEntries), and will be updated in a subsequent CL.

BUG=454984
==========

to

==========
Updated CRWSessionController interface to use NavigationItems.

This CL introduces NavigationItem verions of CRWSessionEntry properties
and rewrites selectors to take NavigationItems.  The underlying logic
is still the same (i.e. NavigationItems are still owned by
CRWSessionEntries), and will be updated in a subsequent CL.

BUG=454984

Review-Url: https://codereview.chromium.org/2671773005
Cr-Commit-Position: refs/heads/master@{#448564}
Committed:
https://chromium.googlesource.com/chromium/src/+/df9b48363d2759ccc531c354f7e5...
==========

[commit-bot: I haz the power, 2017-02-07 06:50:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/df9b48363d2759ccc531c354f7e5...