Send Navigation notifications to clients.

Send Navigation notifications to clients.

This teaches clients about Navigation entries & should eventually allow them to add a client lib which replicates the in-service NavigationController.

Adds back/forward dropdown menus to the browser app.

R=sky@chromium.org

Committed: https://crrev.com/70fb56fb89b2e92a24567dc61fb7a257b6e7ae6a
Cr-Commit-Position: refs/heads/master@{#398747}

[sky, 2016-06-08 21:31:21]

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc
File mash/browser/browser.cc (right):

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:57: int offset;
What is offset? At least document it.

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:122: int multiplier_;
remove as not used.

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:126: int cursor_;
Not used?

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:154: bool OnMousePressed(const ui::MouseEvent& event)
override {
Prefix with // views::LabelButton: (or something similar)

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:181: 
nit: remove extra whitespace.

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:514:
navigation_list_.push_back(std::move(pending_nav_));
Don't you want to add at current_index?

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:519: cursor_ = current_index;
How come you don't replace the entry at current_index here?

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:529: if (from_front) {
Did you mean to have !from_front here as the first branch removes from the end?

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:559: view_->NavigateToOffset(offset);
What happens if navigation_list_ changed while the menu was displayed?

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:592: int cursor_ = 0;
Document what this is. I would be inclined to name it more clearly, e.g.
current_navigation_entry_offset_ or something.

[Ben Goodger (Google), 2016-06-08 21:55:04]

Updated PTAL

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc
File mash/browser/browser.cc (right):

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:514:
navigation_list_.push_back(std::move(pending_nav_));
On 2016/06/08 21:31:20, sky wrote:
> Don't you want to add at current_index?

At this point we should have trimmed the navigation_list_ to size ==
current_index. Perhaps worth DCHECKing that fact.

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:519: cursor_ = current_index;
On 2016/06/08 21:31:20, sky wrote:
> How come you don't replace the entry at current_index here?

That appears to lose data. Sometimes the pending entry doesn't have e.g. title
data.

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:529: if (from_front) {
On 2016/06/08 21:31:20, sky wrote:
> Did you mean to have !from_front here as the first branch removes from the
end?

For navigations, the list reversed. The "front" of the controller I think are
the items at the back of the vector.

https://codereview.chromium.org/2055553002/diff/20001/mash/browser/browser.cc...
mash/browser/browser.cc:559: view_->NavigateToOffset(offset);
On 2016/06/08 21:31:20, sky wrote:
> What happens if navigation_list_ changed while the menu was displayed?

These are great questions for an implementation of this feature significantly
less cheesy than this. I think once I have a client lib & develop a BFList for
it, we can figure all this out & have unit tests.

[Ben Goodger (Google), 2016-06-08 22:23:05]

On 2016/06/08 21:55:04, Ben Goodger (Google) wrote:
> At this point we should have trimmed the navigation_list_ to size ==
> current_index. Perhaps worth DCHECKing that fact.

OK it turns out the service helps you maintain your client-side list via the
prune call. I thought that was for something else. But it seems like in this
case you can always just add it to the end of the list.

> For navigations, the list reversed. The "front" of the controller I think are
> the items at the back of the vector.

I somehow figured this was the case but I was wrong and you were right :-P

[sky, 2016-06-08 22:56:14]

LGTM

[Ben Goodger (Google), 2016-06-09 00:12:31]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2016-06-09 00:12:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055553002/140001

[commit-bot: I haz the power, 2016-06-09 00:18:23]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-09 00:19:25]

Description was changed from

==========
Send Navigation notifications to clients.

This teaches clients about Navigation entries & should eventually allow them to
add a client lib which replicates the in-service NavigationController.

Adds back/forward dropdown menus to the browser app.

R=sky@chromium.org
==========

to

==========
Send Navigation notifications to clients.

This teaches clients about Navigation entries & should eventually allow them to
add a client lib which replicates the in-service NavigationController.

Adds back/forward dropdown menus to the browser app.

R=sky@chromium.org

Committed: https://crrev.com/70fb56fb89b2e92a24567dc61fb7a257b6e7ae6a
Cr-Commit-Position: refs/heads/master@{#398747}
==========

[commit-bot: I haz the power, 2016-06-09 00:19:26]

Patchset 8 (id:??) landed as
https://crrev.com/70fb56fb89b2e92a24567dc61fb7a257b6e7ae6a
Cr-Commit-Position: refs/heads/master@{#398747}