Tied up BlimpNavigationController to NavigationFeature

Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100
Committed: https://crrev.com/07de74f02ced93bc52625ee8f50f9d9a3f40a841
Cr-Commit-Position: refs/heads/master@{#409689}

[shaktisahu, 2016-06-13 21:58:51]

shaktisahu@chromium.org changed reviewers:
+ nyquist@chromium.org

[shaktisahu, 2016-06-23 20:44:34]

Description was changed from

==========
Added network components to BlimpNavigationController

BUG=
==========

to

==========
Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100
==========

[shaktisahu, 2016-06-23 21:14:21]

Description was changed from

==========
Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100
==========

to

==========
Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100
==========

[shaktisahu, 2016-06-23 21:14:21]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-06-29 17:27:15]

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl.h:50: void OnUrlChanged(const GURL& url)
override;
How are these generally exposed through the normal NavigationController flow? 
Does it make sense for us to have our delegate and feature have the same general
API?

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:44: void OnUrlChanged(const
GURL& url) override {}
// BlimpNavigationControllerDelegate implementation.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:49:
DISALLOW_COPY_AND_ASSIGN(TestBlimpNavigationControllerDelegate);
New line before this?

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:16: const int kDummyTabId
= 0;
Can we add a comment about this?  We'll need to solve this shortly.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.h:46: // TODO(shaktisahu):
Remove this.
How will this be set during the normal flow?  In the constructor?  Add a bug
number here.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl_unittest.cc:43:
TEST(BlimpNavigationControllerImplTest, Basic) {
Should we add a separate test for these extra calls?

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/publi...
File
blimp/client/core/public/android/java/src/org/chromium/blimp/core_public/BlimpNavigationController.java
(right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/publi...
blimp/client/core/public/android/java/src/org/chromium/blimp/core_public/BlimpNavigationController.java:51:
* Reload the current page.
Add @param checkForRepost and explain what it is.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.h:62: NavigationFeatureDelegate*
FindDelegate(const int tab_id);
comment. explain whatitdoes and why it's protected.

[shaktisahu, 2016-07-19 21:45:30]

Hi David, 
PTAL

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl.h:50: void OnUrlChanged(const GURL& url)
override;
On 2016/06/29 17:27:15, David Trainor wrote:
> How are these generally exposed through the normal NavigationController flow? 
> Does it make sense for us to have our delegate and feature have the same
general
> API?

Changed it to OnNavigationStateChanged as we discussed. May be we shuold remove
these calls from NavigationFeature too and send an enum and the required params
later.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:44: void OnUrlChanged(const
GURL& url) override {}
On 2016/06/29 17:27:15, David Trainor (OOO until 7-11) wrote:
> // BlimpNavigationControllerDelegate implementation.

Done.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:49:
DISALLOW_COPY_AND_ASSIGN(TestBlimpNavigationControllerDelegate);
On 2016/06/29 17:27:15, David Trainor (OOO until 7-11) wrote:
> New line before this?

Done.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:16: const int kDummyTabId
= 0;
On 2016/06/29 17:27:15, David Trainor (OOO until 7-11) wrote:
> Can we add a comment about this?  We'll need to solve this shortly.

Done.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.h:46: // TODO(shaktisahu):
Remove this.
On 2016/06/29 17:27:15, David Trainor (OOO until 7-11) wrote:
> How will this be set during the normal flow?  In the constructor?  Add a bug
> number here.

Yes. This should be set from the constructor. I added a TODO in constructor.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/publi...
File
blimp/client/core/public/android/java/src/org/chromium/blimp/core_public/BlimpNavigationController.java
(right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/core/publi...
blimp/client/core/public/android/java/src/org/chromium/blimp/core_public/BlimpNavigationController.java:51:
* Reload the current page.
On 2016/06/29 17:27:15, David Trainor wrote:
> Add @param checkForRepost and explain what it is.

Done.

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.h:62: NavigationFeatureDelegate*
FindDelegate(const int tab_id);
On 2016/06/29 17:27:15, David Trainor (OOO until 7-11) wrote:
> comment. explain whatitdoes and why it's protected.

Done.

[David Trainor- moved to gerrit, 2016-07-25 15:57:32]

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl.cc:63:
OnURLUpdated(navigation_controller_.GetURL()));
Shouldn't this be OnNavigationStateChanged()?  Should we remove OnURLUpdated?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:42: void
OnNavigationStateChanged() override{};
space after override

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:59:
BlimpNavigationControllerImpl* nav_controller_impl =
Can we just convert it to an impl when we query it from the BlimpContents?  Or
better yet, make BlimpContentsImpl return BlimpNavigationControllerImpl (see
WebContents/WebContentsImpl).

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:17: // later when it is
fully integrated with BlimpClientSession.
BlimpClientSession -> BlimpClientContext

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:18: const int kDummyTabId
= 0;
namespace {} around this?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:22: :  //
TODO(shaktisahu): Get the feature from BlimpClientSession.
BlimpClientSession -> BlimpClientContext?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.h:32: explicit
BlimpNavigationControllerImpl(
Why can't we pass one in now?  Can we just pass nullptr from the current
callsite if it's not ready yet?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/mo...
File blimp/client/feature/mock_navigation_feature.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/mo...
blimp/client/feature/mock_navigation_feature.cc:19:
delegate->OnUrlChanged(kDummyTabId, GURL(url_text));
Why do we need kDummyTabId if we're passing tab_id in?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.h:51: // virtual for testing.
Do we want to test this to/from the proto level like we did on 0.5?  If not this
is ok.  Just curious.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/public/bli...
File blimp/client/public/blimp_navigation_controller.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/public/bli...
blimp/client/public/blimp_navigation_controller.h:29: virtual void Reload() = 0;
Do these require documentation?

[nyquist, 2016-07-25 22:59:06]

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:173: "feature/mock_navigation_feature.cc",
Could we move these two new files to //blimp/client/core/contents?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/BUILD.gn
File blimp/client/core/BUILD.gn (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/BUILD...
blimp/client/core/BUILD.gn:108: "//blimp/client:test_support",
Does this have to live here? Could we have a new test_support under
//blimp/client/core?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.h:32: explicit
BlimpNavigationControllerImpl(
On 2016/07/25 15:57:31, David Trainor wrote:
> Why can't we pass one in now?  Can we just pass nullptr from the current
> callsite if it's not ready yet?

The feature doesn't live in //blimp/client/core yet, but I think this CL could
move it to //blimp/client/core/contents.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.h:5: #ifndef
BLIMP_CLIENT_FEATURE_NAVIGATION_FEATURE_H_
Can this be moved to //blimp/client/core/contents?

[shaktisahu, 2016-07-26 01:25:27]

Hi David,
PTAL.

Haven't addressed @nyquist's comments related to gn.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl.cc:63:
OnURLUpdated(navigation_controller_.GetURL()));
On 2016/07/25 15:57:31, David Trainor wrote:
> Shouldn't this be OnNavigationStateChanged()?  Should we remove OnURLUpdated?

I think so. But then should BlimpContentsObserver have a pointer to
BlimpContents?

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:42: void
OnNavigationStateChanged() override{};
On 2016/07/25 15:57:31, David Trainor wrote:
> space after override

Done.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:59:
BlimpNavigationControllerImpl* nav_controller_impl =
On 2016/07/25 15:57:31, David Trainor wrote:
> Can we just convert it to an impl when we query it from the BlimpContents?  Or
> better yet, make BlimpContentsImpl return BlimpNavigationControllerImpl (see
> WebContents/WebContentsImpl).

Done.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:17: // later when it is
fully integrated with BlimpClientSession.
On 2016/07/25 15:57:31, David Trainor wrote:
> BlimpClientSession -> BlimpClientContext

Done.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:18: const int kDummyTabId
= 0;
On 2016/07/25 15:57:31, David Trainor wrote:
> namespace {} around this?

Done.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:22: :  //
TODO(shaktisahu): Get the feature from BlimpClientSession.
On 2016/07/25 15:57:31, David Trainor wrote:
> BlimpClientSession -> BlimpClientContext?

Done.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.h:32: explicit
BlimpNavigationControllerImpl(
On 2016/07/25 15:57:31, David Trainor wrote:
> Why can't we pass one in now?  Can we just pass nullptr from the current
> callsite if it's not ready yet?

Done. Passing a nullptr now at the call site (BlimpContentsImpl constructor)

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/mo...
File blimp/client/feature/mock_navigation_feature.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/mo...
blimp/client/feature/mock_navigation_feature.cc:19:
delegate->OnUrlChanged(kDummyTabId, GURL(url_text));
On 2016/07/25 15:57:32, David Trainor wrote:
> Why do we need kDummyTabId if we're passing tab_id in?

Done.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.h:51: // virtual for testing.
On 2016/07/25 15:57:32, David Trainor wrote:
> Do we want to test this to/from the proto level like we did on 0.5?  If not
this
> is ok.  Just curious.

I wanted to test that the feature is getting called. Also mocking out this class
gives a way to test the path from BlimpContents to its observers. I think the
proto layer is adequately tested in the navigation_feature_unittest.cc.

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/public/bli...
File blimp/client/public/blimp_navigation_controller.h (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/public/bli...
blimp/client/public/blimp_navigation_controller.h:29: virtual void Reload() = 0;
On 2016/07/25 15:57:32, David Trainor wrote:
> Do these require documentation?

Done.

[David Trainor- moved to gerrit, 2016-07-27 16:55:01]

Mostly nits.  lgtm % the changes I've requested below.  Thanks!

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/40001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl.cc:63:
OnURLUpdated(navigation_controller_.GetURL()));
On 2016/07/26 01:25:27, shaktisahu wrote:
> On 2016/07/25 15:57:31, David Trainor wrote:
> > Shouldn't this be OnNavigationStateChanged()?  Should we remove
OnURLUpdated?
> 
> I think so. But then should BlimpContentsObserver have a pointer to
> BlimpContents?

Why?  Does it need one?

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:43: void
OnNavigationStateChanged() override{};
Still don't see a space.  Might be the code review tool.

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:68: // EXPECT_EQ(kExampleURL,
observer1.GetLastURL());
?

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:14: // TODO(shaktisahu):
NavigationFeautre currently needs a tab_id. Remove this
NavigationFeautre -> NavigationFeature.

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/public/bli...
File blimp/client/public/blimp_navigation_controller.h (right):

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/public/bli...
blimp/client/public/blimp_navigation_controller.h:32: // Navigation relative to
the "current entry"
. at end

[nyquist, 2016-07-27 17:19:00]

kmarshall: FYI

[nyquist, 2016-07-27 22:36:48]

https://codereview.chromium.org/2058263002/diff/100001/blimp/client/feature/m...
File blimp/client/feature/mock_navigation_feature.cc (right):

https://codereview.chromium.org/2058263002/diff/100001/blimp/client/feature/m...
blimp/client/feature/mock_navigation_feature.cc:17:
delegate->OnUrlChanged(tab_id, GURL(url_text));
This is a little bit weird. It directly calls the delegate instead of waiting.
Could this post instead, similarly to what the fake
blimp_navigation_controller_impl did before?

[shaktisahu, 2016-07-27 23:13:48]

On 2016/07/27 22:36:48, nyquist wrote:
>
https://codereview.chromium.org/2058263002/diff/100001/blimp/client/feature/m...
> File blimp/client/feature/mock_navigation_feature.cc (right):
> 
>
https://codereview.chromium.org/2058263002/diff/100001/blimp/client/feature/m...
> blimp/client/feature/mock_navigation_feature.cc:17:
> delegate->OnUrlChanged(tab_id, GURL(url_text));
> This is a little bit weird. It directly calls the delegate instead of waiting.
> Could this post instead, similarly to what the fake
> blimp_navigation_controller_impl did before?

Done. Also moved the NavigationFeature files to the blimp/client/core/contents.
Tommy, can you please take a look?

[Kevin M, 2016-07-27 23:35:15]

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

[Kevin M, 2016-07-27 23:35:17]

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:43:
BlimpNavigationControllerImpl& GetNavigationController() override;
This should be a pointer, not a reference - reference return types are almost
always const.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:22: //
BlimpContentsImpl is the implementation of the core class in
BlimpContentsImpl isn't defined here; is this comment applicable?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:31: //
TODO(shaktisahu): Provide NavigationFeature* in the constructor.
Obsolete comment?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:34:
NavigationFeature* feature);
Document the lifetime requirements of |feature| ? Does the need for WeakPtr
still exist?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:37: //
BlimpNavigationController implementation.
Overrides go below non-overrides.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:47: // For testing
only.
Is this method necessary given that the ctor takes a NavigationFeature*?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:21:
class TestBlimpNavigationControllerDelegate
s/Test/Mock/

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:30: GURL
last_loaded_url_;
Not used?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:48:
EXPECT_CALL(feature, GoBack(_)).Times(1);
Times(1) is redundant; use StrictMock<>

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:17: class
FakeNavigationFeature : public NavigationFeature {
s/Fake/Mock/

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:31:
base::WeakPtrFactory<FakeNavigationFeature> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGNED

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:31:
base::WeakPtrFactory<FakeNavigationFeature> weak_ptr_factory_;
Is this ever used? I don't see a test case where FakeNavigationFeature is
deleted.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/navigation_feature.h:51: // virtual for testing.
Capitalize Virtual

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/navigation_feature.h:67: typedef
base::SmallMap<std::map<int, NavigationFeatureDelegate*>> DelegateMap;
IMO these private typedefs do more harm than good for readability. Recommend
removing the typedef and using the definition directly.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/public/an...
File
blimp/client/public/android/java/src/org/chromium/blimp_public/contents/BlimpContentsObserver.java
(right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/public/an...
blimp/client/public/android/java/src/org/chromium/blimp_public/contents/BlimpContentsObserver.java:15:
* Invoked when the URL of the BlimpContents is changes.
Comment is obsolete (and has a typo: "changes")

[David Trainor- moved to gerrit, 2016-07-28 16:43:25]

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:43:
BlimpNavigationControllerImpl& GetNavigationController() override;
On 2016/07/27 23:35:17, Kevin M wrote:
> This should be a pointer, not a reference - reference return types are almost
> always const.

We were following the pattern used by WebContents and WebContentsImpl in
Chromium.  Although after looking, it seems like the IOS versions from WebState
return a pointer and say 'cannot be null.'

I was under the impression that a reference is a clean way to state that this
object can never be null, but I'm fine with us changing it to a pointer if you
feel strongly.  I'm not sure if the Chromium style guide prefers one way over
another, but Shakti should look!

[nyquist, 2016-07-28 19:23:06]

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:43:
BlimpNavigationControllerImpl& GetNavigationController() override;
On 2016/07/28 16:43:24, David Trainor wrote:
> On 2016/07/27 23:35:17, Kevin M wrote:
> > This should be a pointer, not a reference - reference return types are
almost
> > always const.
> 
> We were following the pattern used by WebContents and WebContentsImpl in
> Chromium.  Although after looking, it seems like the IOS versions from
WebState
> return a pointer and say 'cannot be null.'
> 
> I was under the impression that a reference is a clean way to state that this
> object can never be null, but I'm fine with us changing it to a pointer if you
> feel strongly.  I'm not sure if the Chromium style guide prefers one way over
> another, but Shakti should look!

Yeah, when I added this I was following the exact same pattern as for the public
content API with the added bonus that it clearly shows it's never null.
Otherwise you have to look at the base class documentation in the header to see
if it can be null or not.

I don't feel strongly, but I didn't quite get what the benefit of what returning
a pointer here would give us.

[shaktisahu, 2016-07-28 19:32:52]

Kevin,
PTAL

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
File blimp/client/core/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:43: void
OnNavigationStateChanged() override{};
On 2016/07/27 16:55:00, David Trainor wrote:
> Still don't see a space.  Might be the code review tool.

Strange! git cl format was removing the space. But any way, the class is removed
now :)

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
blimp/client/core/blimp_contents_impl_unittest.cc:68: // EXPECT_EQ(kExampleURL,
observer1.GetLastURL());
On 2016/07/27 16:55:00, David Trainor wrote:
> ?

Modified the test to check the calls.

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
File blimp/client/core/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/core/blimp...
blimp/client/core/blimp_navigation_controller_impl.cc:14: // TODO(shaktisahu):
NavigationFeautre currently needs a tab_id. Remove this
On 2016/07/27 16:55:00, David Trainor wrote:
> NavigationFeautre -> NavigationFeature.

Done.

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/public/bli...
File blimp/client/public/blimp_navigation_controller.h (right):

https://codereview.chromium.org/2058263002/diff/80001/blimp/client/public/bli...
blimp/client/public/blimp_navigation_controller.h:32: // Navigation relative to
the "current entry"
On 2016/07/27 16:55:00, David Trainor wrote:
> . at end

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:22: //
BlimpContentsImpl is the implementation of the core class in
On 2016/07/27 23:35:17, Kevin M wrote:
> BlimpContentsImpl isn't defined here; is this comment applicable?

Removed the comment since it doesn't convey more than the super class comments.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:31: //
TODO(shaktisahu): Provide NavigationFeature* in the constructor.
On 2016/07/27 23:35:17, Kevin M wrote:
> Obsolete comment?

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:34:
NavigationFeature* feature);
On 2016/07/27 23:35:17, Kevin M wrote:
> Document the lifetime requirements of |feature| ? Does the need for WeakPtr
> still exist?

Added comments in the variables section. Regarding the WeakPtr, please clarify?
If you are talking about the test class, please see my comments there.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:37: //
BlimpNavigationController implementation.
On 2016/07/27 23:35:17, Kevin M wrote:
> Overrides go below non-overrides.

Since the only other non-override method is a forTesting method, I think it
should be deprioritized and go below the other methods? Let me know what you
think.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:47: // For testing
only.
On 2016/07/27 23:35:17, Kevin M wrote:
> Is this method necessary given that the ctor takes a NavigationFeature*?

Yes. It is used in blimp_contents_impl_unittest.cc to set a
FakeNavigationFeature to the navigation controller obtained from the
BlimpContents.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:21:
class TestBlimpNavigationControllerDelegate
On 2016/07/27 23:35:17, Kevin M wrote:
> s/Test/Mock/

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:30: GURL
last_loaded_url_;
On 2016/07/27 23:35:17, Kevin M wrote:
> Not used?

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:48:
EXPECT_CALL(feature, GoBack(_)).Times(1);
On 2016/07/27 23:35:17, Kevin M wrote:
> Times(1) is redundant; use StrictMock<>

Okay, didn't know much about StrictMock. There are some occasions where I want
to verify the exact Times(n), not in this file though. (check
blimp_contents_impl_unittest.cc). Suggestions?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:17: class
FakeNavigationFeature : public NavigationFeature {
On 2016/07/27 23:35:17, Kevin M wrote:
> s/Fake/Mock/

The NavigateToUrlText method emulates that URL has been loaded and notifies the
delegate. So I think calling Fake is more appropriate?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:31:
base::WeakPtrFactory<FakeNavigationFeature> weak_ptr_factory_;
On 2016/07/27 23:35:17, Kevin M wrote:
> Is this ever used? I don't see a test case where FakeNavigationFeature is
> deleted.

Yes, this is used in the C++ file to post a callback.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:31:
base::WeakPtrFactory<FakeNavigationFeature> weak_ptr_factory_;
On 2016/07/27 23:35:17, Kevin M wrote:
> DISALLOW_COPY_AND_ASSIGNED

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/navigation_feature.h:51: // virtual for testing.
On 2016/07/27 23:35:17, Kevin M wrote:
> Capitalize Virtual

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/navigation_feature.h:67: typedef
base::SmallMap<std::map<int, NavigationFeatureDelegate*>> DelegateMap;
On 2016/07/27 23:35:17, Kevin M wrote:
> IMO these private typedefs do more harm than good for readability. Recommend
> removing the typedef and using the definition directly.

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/public/an...
File
blimp/client/public/android/java/src/org/chromium/blimp_public/contents/BlimpContentsObserver.java
(right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/public/an...
blimp/client/public/android/java/src/org/chromium/blimp_public/contents/BlimpContentsObserver.java:15:
* Invoked when the URL of the BlimpContents is changes.
On 2016/07/27 23:35:17, Kevin M wrote:
> Comment is obsolete (and has a typo: "changes")

Done.

[shaktisahu, 2016-07-28 19:32:56]

Kevin,
PTAL

[Kevin M, 2016-07-29 21:05:33]

A couple general comments:
* This CL is really big in terms of the number of different files that it
touches. It would be nice if it were chopped up a bit. Broadly-scoped CLs are
harder for reviewers in two ways: touching a lot of files adds to the cognitive
load and taxes the working memory of the reviewer; and it is simply more
difficult for a reviewer to find a sufficiently large contiguous chunk of time
to perform the review.
* The CL description does not accurately reflect the amount of stuff that was
changed in this CL.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:32: explicit
BlimpNavigationControllerImpl(
"explicit" not used for >1 param ctor

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:34:
NavigationFeature* feature);
On 2016/07/28 19:32:51, shaktisahu wrote:
> On 2016/07/27 23:35:17, Kevin M wrote:
> > Document the lifetime requirements of |feature| ? Does the need for WeakPtr
> > still exist?
> 
> Added comments in the variables section. Regarding the WeakPtr, please
clarify?
> If you are talking about the test class, please see my comments there.

I meant, is it possible that the NavigationController might get navigation
events from the UI after the feature is deleted? If no, then disregard.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:37: //
BlimpNavigationController implementation.
On 2016/07/28 19:32:51, shaktisahu wrote:
> On 2016/07/27 23:35:17, Kevin M wrote:
> > Overrides go below non-overrides.
> 
> Since the only other non-override method is a forTesting method, I think it
> should be deprioritized and go below the other methods? Let me know what you
> think.

Even though it might be a less-important method, the style guide is pretty
categorical about the declaration order - static methods > class methods >
overrides.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:47: // For testing
only.
On 2016/07/28 19:32:51, shaktisahu wrote:
> On 2016/07/27 23:35:17, Kevin M wrote:
> > Is this method necessary given that the ctor takes a NavigationFeature*?
> 
> Yes. It is used in blimp_contents_impl_unittest.cc to set a
> FakeNavigationFeature to the navigation controller obtained from the
> BlimpContents.

Right, but the constructor already takes |feature|, so why do we need a testing
setter?

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:17: class
FakeNavigationFeature : public NavigationFeature {
On 2016/07/28 19:32:52, shaktisahu wrote:
> On 2016/07/27 23:35:17, Kevin M wrote:
> > s/Fake/Mock/
> 
> The NavigateToUrlText method emulates that URL has been loaded and notifies
the
> delegate. So I think calling Fake is more appropriate?

We don't need to emulate that behavior, because it's not really providing better
coverage of product code. We just want to ensure that NavigateToUrlText is
called. Invoking the delegate with a fake method is only really exercising
GetDelegate(), which you can test through more direct means.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:31:
base::WeakPtrFactory<FakeNavigationFeature> weak_ptr_factory_;
On 2016/07/28 19:32:52, shaktisahu wrote:
> On 2016/07/27 23:35:17, Kevin M wrote:
> > Is this ever used? I don't see a test case where FakeNavigationFeature is
> > deleted.
> 
> Yes, this is used in the C++ file to post a callback.

WeakPtr isn't useful though. Just use base::Unretained().

You might want to read weak_ptr.h to get an understanding of why WeakPtr is
useful, since this has come up elsewhere in this code review.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:45:
feature.SetDelegate(1, &navigation_controller);
Can you move these right after "navigation_controller", so that the declaration
is directly proximate to the initialization?

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_delegate.h (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_delegate.h:20: // Informs
the delegate that navigation state has been changed.
was/has changed

no need to use the passive voice with "been"

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.cc:16: const int
kDummyTabId = 0;
We should use a shared constant instead of redefining this for every feature.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.cc:26: if
(navigation_feature_)
Why would this be non-null?

If there's no reason, then DCHECK the ptr.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.cc:48: //
TODO(shaktisahu): Implement the client-side logic.
Replace these TODOs with NOTIMPLEMENTED()

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:38: 
Remove newline to keep these overrides together as a single semantic block.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:54: //
|navigation_feature_| must remain valid throughout |this|.
Lifetime requirement comments should be documented in the constructor comments.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:5:
#include <blimp/client/core/contents/fake_navigation_feature.h>
Don't use <> for non-system includes

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:47:
EXPECT_CALL(feature, GoForward(_)).Times(1);
Use StrictMock; remove Times(1)

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/navigation_feature.h:62: // Finds the
NavigationFeatureDelegate for a |tab_id|, protected for unit
A better practice is to break out the feature delegate map into a standalone
manager object, which can be injected by test code. (favoring composition over
inheritance)

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/public/co...
File blimp/client/public/contents/blimp_navigation_controller.h (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/public/co...
blimp/client/public/contents/blimp_navigation_controller.h:27: virtual void
LoadURL(const GURL& url) = 0;
Should we add parameters for tabs here too?

[shaktisahu, 2016-07-29 22:57:31]

@kmarshall, PTAL

The CL was started out very small. But as a few files were moved to
blimp/client/core/contents, it became big and cumbersome to review.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:32: explicit
BlimpNavigationControllerImpl(
On 2016/07/29 21:05:32, Kevin M wrote:
> "explicit" not used for >1 param ctor

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:34:
NavigationFeature* feature);
On 2016/07/29 21:05:32, Kevin M wrote:
> On 2016/07/28 19:32:51, shaktisahu wrote:
> > On 2016/07/27 23:35:17, Kevin M wrote:
> > > Document the lifetime requirements of |feature| ? Does the need for
WeakPtr
> > > still exist?
> > 
> > Added comments in the variables section. Regarding the WeakPtr, please
> clarify?
> > If you are talking about the test class, please see my comments there.
> 
> I meant, is it possible that the NavigationController might get navigation
> events from the UI after the feature is deleted? If no, then disregard.

Possibly not. The |feature| lives in BlimpClientSession and is managed on main
thread.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:37: //
BlimpNavigationController implementation.
On 2016/07/29 21:05:32, Kevin M wrote:
> On 2016/07/28 19:32:51, shaktisahu wrote:
> > On 2016/07/27 23:35:17, Kevin M wrote:
> > > Overrides go below non-overrides.
> > 
> > Since the only other non-override method is a forTesting method, I think it
> > should be deprioritized and go below the other methods? Let me know what you
> > think.
> 
> Even though it might be a less-important method, the style guide is pretty
> categorical about the declaration order - static methods > class methods >
> overrides.

Done.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:47: // For testing
only.
On 2016/07/29 21:05:32, Kevin M wrote:
> On 2016/07/28 19:32:51, shaktisahu wrote:
> > On 2016/07/27 23:35:17, Kevin M wrote:
> > > Is this method necessary given that the ctor takes a NavigationFeature*?
> > 
> > Yes. It is used in blimp_contents_impl_unittest.cc to set a
> > FakeNavigationFeature to the navigation controller obtained from the
> > BlimpContents.
> 
> Right, but the constructor already takes |feature|, so why do we need a
testing
> setter?

Because this object is created inside BlimpContentsImpl constructor which takes
no argument and there is no way to pass this |feature|.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:17: class
FakeNavigationFeature : public NavigationFeature {
On 2016/07/29 21:05:32, Kevin M wrote:
> On 2016/07/28 19:32:52, shaktisahu wrote:
> > On 2016/07/27 23:35:17, Kevin M wrote:
> > > s/Fake/Mock/
> > 
> > The NavigateToUrlText method emulates that URL has been loaded and notifies
> the
> > delegate. So I think calling Fake is more appropriate?
> 
> We don't need to emulate that behavior, because it's not really providing
better
> coverage of product code. We just want to ensure that NavigateToUrlText is
> called. Invoking the delegate with a fake method is only really exercising
> GetDelegate(), which you can test through more direct means.

hmm.. Emulating this method provides a nice way to verify the call path all the
way from BlimpContents, BlimpNavigationController,
BlimpNavigationControllerDelegate, BlimpContentsObserver. For instance, we could
verify if we are loading the correct URL and check the observers etc. In future,
we might also need to verify end-to-end from Java.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:31:
base::WeakPtrFactory<FakeNavigationFeature> weak_ptr_factory_;
On 2016/07/29 21:05:32, Kevin M wrote:
> On 2016/07/28 19:32:52, shaktisahu wrote:
> > On 2016/07/27 23:35:17, Kevin M wrote:
> > > Is this ever used? I don't see a test case where FakeNavigationFeature is
> > > deleted.
> > 
> > Yes, this is used in the C++ file to post a callback.
> 
> WeakPtr isn't useful though. Just use base::Unretained().
> 
> You might want to read weak_ptr.h to get an understanding of why WeakPtr is
> useful, since this has come up elsewhere in this code review.

Done. Thanks!

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_delegate.h (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_delegate.h:20: // Informs
the delegate that navigation state has been changed.
On 2016/07/29 21:05:32, Kevin M wrote:
> was/has changed
> 
> no need to use the passive voice with "been"

Done.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.cc:16: const int
kDummyTabId = 0;
On 2016/07/29 21:05:32, Kevin M wrote:
> We should use a shared constant instead of redefining this for every feature.

Acknowledged.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.cc:26: if
(navigation_feature_)
On 2016/07/29 21:05:32, Kevin M wrote:
> Why would this be non-null?
> 
> If there's no reason, then DCHECK the ptr.

Yea, I should DCHECK. But currently the hookup is not complete till
BlimpClientSession integration and I am passing null for |feature|.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.cc:48: //
TODO(shaktisahu): Implement the client-side logic.
On 2016/07/29 21:05:32, Kevin M wrote:
> Replace these TODOs with NOTIMPLEMENTED()

Done.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc:47:
EXPECT_CALL(feature, GoForward(_)).Times(1);
On 2016/07/29 21:05:32, Kevin M wrote:
> Use StrictMock; remove Times(1)

Done.

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/public/co...
File blimp/client/public/contents/blimp_navigation_controller.h (right):

https://codereview.chromium.org/2058263002/diff/140001/blimp/client/public/co...
blimp/client/public/contents/blimp_navigation_controller.h:27: virtual void
LoadURL(const GURL& url) = 0;
On 2016/07/29 21:05:33, Kevin M wrote:
> Should we add parameters for tabs here too?

No, the plan is that navigation controller will not know about the tab id. The
params passed to this function will change in future as more  details about the
implementation is worked out.

[Kevin M, 2016-08-01 23:41:58]

Yeah, that's how these things usually go. Still, though, there are ways you can
chop up the CLs by managing multiple Git branches. For example, you can use
upstream branches in Git to create a sequence of dependent CLs that must be
submitted in order.

Otherwise there are too many parallel discussions going on at once, hence my
review latency (gotta cross-reference a lot of files before I write comments).

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:43:
BlimpNavigationControllerImpl& GetNavigationController() override;
On 2016/07/28 19:23:06, nyquist wrote:
> On 2016/07/28 16:43:24, David Trainor wrote:
> > On 2016/07/27 23:35:17, Kevin M wrote:
> > > This should be a pointer, not a reference - reference return types are
> almost
> > > always const.
> > 
> > We were following the pattern used by WebContents and WebContentsImpl in
> > Chromium.  Although after looking, it seems like the IOS versions from
> WebState
> > return a pointer and say 'cannot be null.'
> > 
> > I was under the impression that a reference is a clean way to state that
this
> > object can never be null, but I'm fine with us changing it to a pointer if
you
> > feel strongly.  I'm not sure if the Chromium style guide prefers one way
over
> > another, but Shakti should look!
> 
> Yeah, when I added this I was following the exact same pattern as for the
public
> content API with the added bonus that it clearly shows it's never null.
> Otherwise you have to look at the base class documentation in the header to
see
> if it can be null or not.
> 
> I don't feel strongly, but I didn't quite get what the benefit of what
returning
> a pointer here would give us.

The convention that Wez and I have become very accustomed to is that pointers
imply immutability and a potentially long lifetime (you can depend on the
pointer living as long as the comment implies) whereas references imply
constness and a potentially shorter lifetime.

ISTM that the style guide has implicitly declared a preference for pointered
return values by specifying that mutable parameters must be passed in via
pointers. Pointer parameters are also susceptible to being passed
surprise-NULLs, but we accept that risk--mitigated with DCHECK--in exchange for
the readability benefit of strongly associating mutability with pointers.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:47: // For testing
only.
On 2016/07/29 22:57:30, shaktisahu wrote:
> On 2016/07/29 21:05:32, Kevin M wrote:
> > On 2016/07/28 19:32:51, shaktisahu wrote:
> > > On 2016/07/27 23:35:17, Kevin M wrote:
> > > > Is this method necessary given that the ctor takes a NavigationFeature*?
> > > 
> > > Yes. It is used in blimp_contents_impl_unittest.cc to set a
> > > FakeNavigationFeature to the navigation controller obtained from the
> > > BlimpContents.
> > 
> > Right, but the constructor already takes |feature|, so why do we need a
> testing
> > setter?
> 
> Because this object is created inside BlimpContentsImpl constructor which
takes
> no argument and there is no way to pass this |feature|.

That's true right now, but BlimpContentsImpl can easily provide an alternate
constructor that takes a NavigationController*. The default no-args ctor can
then delegate to that ctor.

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:17: class
FakeNavigationFeature : public NavigationFeature {
On 2016/07/29 22:57:30, shaktisahu wrote:
> On 2016/07/29 21:05:32, Kevin M wrote:
> > On 2016/07/28 19:32:52, shaktisahu wrote:
> > > On 2016/07/27 23:35:17, Kevin M wrote:
> > > > s/Fake/Mock/
> > > 
> > > The NavigateToUrlText method emulates that URL has been loaded and
notifies
> > the
> > > delegate. So I think calling Fake is more appropriate?
> > 
> > We don't need to emulate that behavior, because it's not really providing
> better
> > coverage of product code. We just want to ensure that NavigateToUrlText is
> > called. Invoking the delegate with a fake method is only really exercising
> > GetDelegate(), which you can test through more direct means.
> 
> hmm.. Emulating this method provides a nice way to verify the call path all
the
> way from BlimpContents, BlimpNavigationController,
> BlimpNavigationControllerDelegate, BlimpContentsObserver. For instance, we
could
> verify if we are loading the correct URL and check the observers etc. In
future,
> we might also need to verify end-to-end from Java.

Yeah, but right now a lot of the functionality of this Fake that is being tested
is... test code. What's the point of testing that?

My gut feeling is that we'll probably want to avoid the use of mocks in
end-to-end testing anyhow, since we'll want as little "fakeness" as possible?

We can demote FindDelegate() to class-private afterward.

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:29: // BlimpContentsImpl is the
implementation of the core class in
The code makes this all clear - can you provide a product-oriented description
of this class' reason for existing & behavior?

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:5: #include
<blimp/client/core/contents/fake_navigation_feature.h>
Should use "" instead of <>, move into the second include block.

Eclipse refactoring? :\

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:18: const GURL
kExampleURL = GURL("https://www.example.com/");
Globals must be POD types e.g. const char[]

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:32:
TEST(BlimpContentsImplTest, Basic) {
Test name is too basic ;)

RouteNavigationStateChanged?

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.cc (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.cc:21:
base::ThreadTaskRunnerHandle::Get()->PostTask(
(See comment in .h)

Why are we posting a task instead of just invoking the method directly?

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/navigation_feature.cc (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/navigation_feature.cc:38:
base::SmallMap<std::map<int, NavigationFeatureDelegate*>>::iterator it =
You can use "auto it"

[shaktisahu, 2016-08-02 17:47:53]

Hi Kevin, PTAL

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/120001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:47: // For testing
only.
On 2016/08/01 23:41:57, Kevin M wrote:
> On 2016/07/29 22:57:30, shaktisahu wrote:
> > On 2016/07/29 21:05:32, Kevin M wrote:
> > > On 2016/07/28 19:32:51, shaktisahu wrote:
> > > > On 2016/07/27 23:35:17, Kevin M wrote:
> > > > > Is this method necessary given that the ctor takes a
NavigationFeature*?
> > > > 
> > > > Yes. It is used in blimp_contents_impl_unittest.cc to set a
> > > > FakeNavigationFeature to the navigation controller obtained from the
> > > > BlimpContents.
> > > 
> > > Right, but the constructor already takes |feature|, so why do we need a
> > testing
> > > setter?
> > 
> > Because this object is created inside BlimpContentsImpl constructor which
> takes
> > no argument and there is no way to pass this |feature|.
> 
> That's true right now, but BlimpContentsImpl can easily provide an alternate
> constructor that takes a NavigationController*. The default no-args ctor can
> then delegate to that ctor.

Sure, that's a good idea. In that case, we are providing an extra constructor
that originally we didn't want to provide to the users. Not strongly feeling
about it though. Let me know if you want me to make this change. There are lots
of examples following "**ForTesting" pattern. So I decided to use.

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl.h (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl.h:29: // BlimpContentsImpl is the
implementation of the core class in
On 2016/08/01 23:41:57, Kevin M wrote:
> The code makes this all clear - can you provide a product-oriented description
> of this class' reason for existing & behavior?

Right. I improved the description in BlimpContents interface. Please have a look
and let me know if we still need description here.

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_contents_impl_unittest.cc (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:5: #include
<blimp/client/core/contents/fake_navigation_feature.h>
On 2016/08/01 23:41:58, Kevin M wrote:
> Should use "" instead of <>, move into the second include block.
> 
> Eclipse refactoring? :\

Yes.. Eclipse refactoring.

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:18: const GURL
kExampleURL = GURL("https://www.example.com/");
On 2016/08/01 23:41:58, Kevin M wrote:
> Globals must be POD types e.g. const char[]

Done.

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/blimp_contents_impl_unittest.cc:32:
TEST(BlimpContentsImplTest, Basic) {
On 2016/08/01 23:41:58, Kevin M wrote:
> Test name is too basic ;)
> 
> RouteNavigationStateChanged?

Done.

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.cc (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.cc:21:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/08/01 23:41:58, Kevin M wrote:
> (See comment in .h)
> 
> Why are we posting a task instead of just invoking the method directly?

In real world scenarios, the delegates/observers are usually invoked
asynchronously. Posting a task here kind of mimics that.

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
File blimp/client/core/contents/navigation_feature.cc (right):

https://codereview.chromium.org/2058263002/diff/160001/blimp/client/core/cont...
blimp/client/core/contents/navigation_feature.cc:38:
base::SmallMap<std::map<int, NavigationFeatureDelegate*>>::iterator it =
On 2016/08/01 23:41:58, Kevin M wrote:
> You can use "auto it"

Done.

[shaktisahu, 2016-08-02 17:47:57]

Hi Kevin, PTAL

[Kevin M, 2016-08-03 16:55:16]

lgtm % nits

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
File blimp/client/core/contents/android/blimp_contents_observer_proxy.cc
(right):

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
blimp/client/core/contents/android/blimp_contents_observer_proxy.cc:53:
java_obj_.obj());
This looks like Java-style formatting? Run "git cl format".

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.cc (right):

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.cc:22: //
TODO(shaktisahu): DCHECK(navigation_feature_).
Doesn't look like this comment suggests a future course of action? Recommend
acting on this comment or removing it now.

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
File blimp/client/core/contents/blimp_navigation_controller_impl.h (right):

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
blimp/client/core/contents/blimp_navigation_controller_impl.h:50: // navigation
state to the server over the proto channel. The life time of
Move comments on lifetime to constructor comments.

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
File blimp/client/core/contents/fake_navigation_feature.h (right):

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/core/cont...
blimp/client/core/contents/fake_navigation_feature.h:28: void
NotifyDelegateURLLoaded(int tab_id, const std::string& url_text);
Add a comment specifying that this function is designed to mimic asynchronous
invocation from NavigateToUrlText.

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/public/an...
File
blimp/client/public/android/java/src/org/chromium/blimp_public/contents/BlimpNavigationController.java
(right):

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/public/an...
blimp/client/public/android/java/src/org/chromium/blimp_public/contents/BlimpNavigationController.java:51:
* Reload the current page. If |checkForRepost| is true and the current entry has
POST data the
Shouldn't checkForRepost be documented via @param ?

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/public/co...
File blimp/client/public/contents/blimp_contents.h (right):

https://codereview.chromium.org/2058263002/diff/180001/blimp/client/public/co...
blimp/client/public/contents/blimp_contents.h:18: // BlimpContents is the core
class in //blimp/client which is responsible for
This is nice!

You can remove "in //blimp/client" because that detail is well-established by
the file path.

[shaktisahu, 2016-08-03 18:38:08]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-08-03 18:38:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2058263002/#ps200001
(title: "nits and merge origin/master")

[commit-bot: I haz the power, 2016-08-03 18:38:59]

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-08-03 18:43:35]

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

[commit-bot: I haz the power, 2016-08-03 18:43:37]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shaktisahu, 2016-08-03 20:02:01]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-08-03 20:02:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2058263002/#ps220001
(title: "Fixed DEPS")

[commit-bot: I haz the power, 2016-08-03 20:03:00]

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-08-03 20:07:44]

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

[commit-bot: I haz the power, 2016-08-03 20:07:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shaktisahu, 2016-08-03 21:01:26]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-08-03 21:01:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2058263002/#ps240001
(title: "DEPS for linux")

[commit-bot: I haz the power, 2016-08-03 21:02: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, 2016-08-03 21:08:05]

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

[commit-bot: I haz the power, 2016-08-03 21:08:07]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shaktisahu, 2016-08-03 21:26:06]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-08-03 21:26:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2058263002/#ps260001
(title: "More deps")

[commit-bot: I haz the power, 2016-08-03 21:26:43]

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-08-03 22:44:49]

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

[commit-bot: I haz the power, 2016-08-03 22:44:50]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[shaktisahu, 2016-08-03 23:26:05]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-08-03 23:26:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2058263002/#ps280001
(title: "Removed BlimpContentsTest.java")

[commit-bot: I haz the power, 2016-08-03 23:26:37]

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-08-04 01:07:24]

Description was changed from

==========
Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100
==========

to

==========
Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100
==========

[commit-bot: I haz the power, 2016-08-04 01:07:27]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-08-04 01:09:34]

Description was changed from

==========
Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100
==========

to

==========
Tied up BlimpNavigationController to NavigationFeature

Added some necessary navigation methods to BlimpNavigationController
Java interface and hooked it up all the way to navigation_feature.cc.
Added C++ unit tests for the same.

BUG=611100

Committed: https://crrev.com/07de74f02ced93bc52625ee8f50f9d9a3f40a841
Cr-Commit-Position: refs/heads/master@{#409689}
==========

[commit-bot: I haz the power, 2016-08-04 01:09:35]

Patchset 15 (id:??) landed as
https://crrev.com/07de74f02ced93bc52625ee8f50f9d9a3f40a841
Cr-Commit-Position: refs/heads/master@{#409689}