Changed NavigationController access to through tab in Java code

Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
NavigationHandler interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080
Committed: https://crrev.com/8b75c19b428805a0465624cd6047b705c3b9a1b5
Cr-Commit-Position: refs/heads/master@{#405899}

[shaktisahu, 2016-06-20 18:58:39]

Description was changed from

==========
Changed NavigationController access to through tab in Java code

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

to

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence needs a
different way to access NavigationController. In this CL, a
BaseNavigationController interface was written that can be directly accessed
from tab. This would be used throughout clank UI. Chrome and blimp versions
would implement this interface.   
BUG=612080
==========

[shaktisahu, 2016-06-20 18:58:39]

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

[shaktisahu, 2016-06-21 17:36:20]

Description was changed from

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence needs a
different way to access NavigationController. In this CL, a
BaseNavigationController interface was written that can be directly accessed
from tab. This would be used throughout clank UI. Chrome and blimp versions
would implement this interface.   
BUG=612080
==========

to

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
BaseNavigationController interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080
==========

[David Trainor- moved to gerrit, 2016-06-21 23:32:01]

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java
(right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:29:
import org.chromium.chrome.browser.navigation.BaseNavigationController;
Should this be ChromeNavigationController?

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java
(right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java:14:
* An interface that is responsible for managing back-forward navigation for a
tab.
Update this a bit to explain more why it's here (in an abstract way).  To allow
the Chrome layer to interact with navigation controllers from multiple types of
underlying source content.

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java:16:
public interface BaseNavigationController {
All of these methods are used by the Chrome Java layer?

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/navigation/ChromeNavigationControllerWrapper.java
(right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/ChromeNavigationControllerWrapper.java:13:
* Provides navigation functionalities for Chrome.
Maybe expand on the javadoc a bit.  Explain how it wraps the content layer
NavigationController and delegates all calls to it?

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/ChromeNavigationControllerWrapper.java:15:
public class ChromeNavigationControllerWrapper implements
BaseNavigationController {
ContentNavigationControllerWrapper implements ChromeNavigationController?

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:401:
mBaseNavigationController =
We should build and destroy this at setContentViewCore and
destroyContentViewCore.

[shaktisahu, 2016-06-23 22:26:05]

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java
(right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:29:
import org.chromium.chrome.browser.navigation.BaseNavigationController;
On 2016/06/21 23:32:01, David Trainor wrote:
> Should this be ChromeNavigationController?

No, this should be the generic one.

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java
(right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java:14:
* An interface that is responsible for managing back-forward navigation for a
tab.
On 2016/06/21 23:32:01, David Trainor wrote:
> Update this a bit to explain more why it's here (in an abstract way).  To
allow
> the Chrome layer to interact with navigation controllers from multiple types
of
> underlying source content.

Done.

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java:16:
public interface BaseNavigationController {
On 2016/06/21 23:32:01, David Trainor wrote:
> All of these methods are used by the Chrome Java layer?

Yes, I removed 3 more methods that weren't used.

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/navigation/ChromeNavigationControllerWrapper.java
(right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/ChromeNavigationControllerWrapper.java:13:
* Provides navigation functionalities for Chrome.
On 2016/06/21 23:32:01, David Trainor wrote:
> Maybe expand on the javadoc a bit.  Explain how it wraps the content layer
> NavigationController and delegates all calls to it?

Done.

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/ChromeNavigationControllerWrapper.java:15:
public class ChromeNavigationControllerWrapper implements
BaseNavigationController {
On 2016/06/21 23:32:01, David Trainor wrote:
> ContentNavigationControllerWrapper implements ChromeNavigationController?

Why do we need ContentNavigationControllerWrapper?

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2087543002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:401:
mBaseNavigationController =
On 2016/06/21 23:32:01, David Trainor wrote:
> We should build and destroy this at setContentViewCore and
> destroyContentViewCore.

Done.

[Ted C, 2016-06-24 20:06:19]

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

[Ted C, 2016-06-24 20:06:20]

https://codereview.chromium.org/2087543002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java
(right):

https://codereview.chromium.org/2087543002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java:17:
public interface BaseNavigationController {
Don't call this BaseNavigationController or really .*NavigationController.

If we do want to have this exist only in chrome, call it something like
NavigationHandler or NavigationDelegate (potentially with a "Tab" prefix).

We should never have a "base" class that exists in Chrome while the
implementation is in content/.  That does not fit the layering model at all.

But in this model, I think you'd have a

TabWebContentsNavigationHandler implements NavigationHandler,
NavigationController {
     private final NavigationController mNavigationController;

    public TabWebContentsNavigationHandler(WebContents webContents) {
        mNavigationController = webContents.getNavigationController();
    }

    .... override all the methods ....
}

[shaktisahu, 2016-06-28 22:23:23]

Description was changed from

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
BaseNavigationController interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080
==========

to

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
NavigationHandler interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080
==========

[shaktisahu, 2016-06-28 22:26:29]

On 2016/06/24 20:06:20, Ted C wrote:
>
https://codereview.chromium.org/2087543002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java
> (right):
> 
>
https://codereview.chromium.org/2087543002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/navigation/BaseNavigationController.java:17:
> public interface BaseNavigationController {
> Don't call this BaseNavigationController or really .*NavigationController.
> 
> If we do want to have this exist only in chrome, call it something like
> NavigationHandler or NavigationDelegate (potentially with a "Tab" prefix).
> 
> We should never have a "base" class that exists in Chrome while the
> implementation is in content/.  That does not fit the layering model at all.
> 
> But in this model, I think you'd have a
> 
> TabWebContentsNavigationHandler implements NavigationHandler,
> NavigationController {
>      private final NavigationController mNavigationController;
> 
>     public TabWebContentsNavigationHandler(WebContents webContents) {
>         mNavigationController = webContents.getNavigationController();
>     }
> 
>     .... override all the methods ....
> }

Thanks. NavigationHandler sounds much better. I have renamed this interface. I
am not clear if TabWebContentsNavigationHandler should implement
NavigationController though. Please look at the changes and let me know.

[David Trainor- moved to gerrit, 2016-07-12 17:29:29]

lgtm

[shaktisahu, 2016-07-15 22:49:11]

The CQ bit was checked by shaktisahu@chromium.org

[commit-bot: I haz the power, 2016-07-15 22:49:38]

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-07-15 23:38:27]

Description was changed from

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
NavigationHandler interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080
==========

to

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
NavigationHandler interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080
==========

[commit-bot: I haz the power, 2016-07-15 23:38:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-15 23:38:30]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 23:40:14]

Description was changed from

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
NavigationHandler interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080
==========

to

==========
Changed NavigationController access to through tab in Java code

Currently most of the chrome UI code access NavigationController through
WebContents. However for Blimp mode there is no WebContents and hence
needs a different way to access NavigationController. In this CL, a
NavigationHandler interface was written that can be directly
accessed from tab. This would be used throughout clank UI. Chrome and
blimp versions would implement this interface.
BUG=612080

Committed: https://crrev.com/8b75c19b428805a0465624cd6047b705c3b9a1b5
Cr-Commit-Position: refs/heads/master@{#405899}
==========

[commit-bot: I haz the power, 2016-07-15 23:40:15]

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