Move ToolbarManager and AppMenuHandler creation to ChromeActivity

Move ToolbarManager and AppMenuHandler creation to ChromeActivity

Defines an API for confirming that a toolbar exists within the layout
of a ChromeActivity and uses that to construct the ToolbarManager and
AppMenuHandler and AppMenuPropertiesDelegate. This is a first CL
that moves essentials only removing duplicated code and fixing some
missing functionality in CustomTabActivity. Follow ups will include
initialization for ToolbarManager and getting the FindToolbarManager
to be owned by ToolbarManager.

BUG=501509
Committed: https://crrev.com/8b3c633165edb0311674751ef86e08c86b53277e
Cr-Commit-Position: refs/heads/master@{#336607}

[Yusuf, 2015-06-26 00:17:35]

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

[Ted C, 2015-06-26 23:03:12]

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:195:
protected ChromeAppMenuPropertiesDelegate mAppMenuPropertiesDelegate;
I'd rather just have protected accessors for these.

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:272: if
(containsToolbar()) {
Instead of containsToolbar(), I wonder if this would be more consistent to have
a initializeToolbar method that classes can override and do nothing.

Then this class would internally just check mToolbarManager != null.

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:542:
return containsToolbar() ? mToolbarManager.hasDoneFirstDraw() : true;
I would return super.hasDoneFirstDraw() here instead of true in case that wants
to track it in a more meaningful way someday.

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:573: if
(containsToolbar()) {
For all these, I would just check mToolbarManager != null instead of
containsToolbar.

[Yusuf, 2015-06-27 00:20:42]

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:195:
protected ChromeAppMenuPropertiesDelegate mAppMenuPropertiesDelegate;
On 2015/06/26 23:03:12, Ted C wrote:
> I'd rather just have protected accessors for these.

Done.

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:272: if
(containsToolbar()) {
On 2015/06/26 23:03:12, Ted C wrote:
> Instead of containsToolbar(), I wonder if this would be more consistent to
have
> a initializeToolbar method that classes can override and do nothing.
> 
> Then this class would internally just check mToolbarManager != null.

Done.

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:542:
return containsToolbar() ? mToolbarManager.hasDoneFirstDraw() : true;
On 2015/06/26 23:03:12, Ted C wrote:
> I would return super.hasDoneFirstDraw() here instead of true in case that
wants
> to track it in a more meaningful way someday.

Done.

https://codereview.chromium.org/1211673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:573: if
(containsToolbar()) {
On 2015/06/26 23:03:12, Ted C wrote:
> For all these, I would just check mToolbarManager != null instead of
> containsToolbar.

Done.

[Ted C, 2015-06-29 16:51:13]

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

https://codereview.chromium.org/1211673002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:314:
protected void initializeToolbar() {
This doesn't seem to be called anywhere.

And the subclasses still have containsToolbar(), so it looks like it's not fully
up to date.

It is also kind of sad that child classes could not create their own subclass of
ToolbarManager with this approach.  I guess in the future if that is necessary,
we could provide a method

createToolbarManager(ControlContainer, AppMenuHandler, etc...) that child
classes could extend and we could still keep this logic the same.

So I guess this is fine for now actually.

[Yusuf, 2015-06-29 17:46:00]

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

https://codereview.chromium.org/1211673002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:314:
protected void initializeToolbar() {
On 2015/06/29 16:51:13, Ted C wrote:
> This doesn't seem to be called anywhere.
> 
> And the subclasses still have containsToolbar(), so it looks like it's not
fully
> up to date.
> 
> It is also kind of sad that child classes could not create their own subclass
of
> ToolbarManager with this approach.  I guess in the future if that is
necessary,
> we could provide a method
> 
> createToolbarManager(ControlContainer, AppMenuHandler, etc...) that child
> classes could extend and we could still keep this logic the same.
> 
> So I guess this is fine for now actually.

Done.

[Ted C, 2015-06-29 17:51:35]

lgtm

[Yusuf, 2015-06-29 17:51:58]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2015-06-29 17:53:01]

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

[commit-bot: I haz the power, 2015-06-29 17:56:44]

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

[commit-bot: I haz the power, 2015-06-29 17:56:44]

Exceeded global retry quota

[Yusuf, 2015-06-29 18:16:55]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2015-06-29 18:16:55]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1211673002/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2015-06-29 18:19:29]

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

[commit-bot: I haz the power, 2015-06-29 19:08:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-29 19:09:20]

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