Stop/reload button should hide if url bar is focussed.

Stop/reload button should hide if url bar is focussed.

These changes are to hide the Stop/reload button if url
bar is focussed when we switch from tab manager to main screen.

BUG=457540
Committed: https://crrev.com/f75f91bf4b302a0101b63a2be9f9e899ffd9d88b
Cr-Commit-Position: refs/heads/master@{#315963}

[divya.bansal, 2015-02-11 06:06:56]

divya.bansal@samsung.com changed reviewers:
+ bauerb@chromium.org

[divya.bansal, 2015-02-11 06:07:08]

On 2015/02/11 06:06:56, divya.bansal wrote:
> mailto:divya.bansal@samsung.com changed reviewers:
> + mailto:bauerb@chromium.org

@Bernhard PTAL

[Bernhard Bauer, 2015-02-11 17:08:56]

Instead of adding more hacks to fix the old hacks, this should be done properly.

Our invariant is "the stop/reload button is visible if the URL field is not
focused and the tab switcher is not shown". What we should do is move that logic
to a single method that updates the visibility of the stop/reload button, and
call that any time any of its inputs changes. Same for the "add tab" button,
where the invariant is that it is visible if the tab switcher is shown.

[divya.bansal, 2015-02-12 09:29:43]

On 2015/02/11 17:08:56, Bernhard Bauer wrote:
> Instead of adding more hacks to fix the old hacks, this should be done
properly.
> 
> Our invariant is "the stop/reload button is visible if the URL field is not
> focused and the tab switcher is not shown". What we should do is move that
logic
> to a single method that updates the visibility of the stop/reload button, and
> call that any time any of its inputs changes. Same for the "add tab" button,
> where the invariant is that it is visible if the tab switcher is shown.

@Bernhard PTAL

[Bernhard Bauer, 2015-02-12 09:55:09]

https://codereview.chromium.org/914933002/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/914933002/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:271:
* @param visibility The visibility status of the stop/reload button.
This parameter doesn't really control the visibility of the button. For one, if
it's set to true, the button is *hidden*, and if it's set to false, the button
is hidden only if |mFocus| is false. In terms of reducing the number of hacks,
this is not going in the right direction.

Let me repeat the invariants:
* The stop/reload button is visible if the URL field is not focused and the tab
switcher is not shown.
* The "add tab" button is visible if the tab switcher is shown.

You can translate this straight into code, with one method that updates the
status of each button, and which is called whenever any of its dependencies
changes.

[divya.bansal, 2015-02-12 11:36:51]

On 2015/02/12 09:55:09, Bernhard Bauer wrote:
>
https://codereview.chromium.org/914933002/diff/20001/chrome/android/shell/jav...
> File
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
> (right):
> 
>
https://codereview.chromium.org/914933002/diff/20001/chrome/android/shell/jav...
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:271:
> * @param visibility The visibility status of the stop/reload button.
> This parameter doesn't really control the visibility of the button. For one,
if
> it's set to true, the button is *hidden*, and if it's set to false, the button
> is hidden only if |mFocus| is false. In terms of reducing the number of hacks,
> this is not going in the right direction.
> 
> Let me repeat the invariants:
> * The stop/reload button is visible if the URL field is not focused and the
tab
> switcher is not shown.
> * The "add tab" button is visible if the tab switcher is shown.
> 
> You can translate this straight into code, with one method that updates the
> status of each button, and which is called whenever any of its dependencies
> changes.

PTAL

[Bernhard Bauer, 2015-02-12 11:49:12]

https://codereview.chromium.org/914933002/diff/40001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/914933002/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:261:
* Shows or hides the add and the stop/reload button .
Nit: remove the space before the period.

https://codereview.chromium.org/914933002/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:262:
* @param visibility The visibility status of the add button.
Javadoc comment is now outdated.

[divya.bansal, 2015-02-12 12:03:26]

On 2015/02/12 11:49:12, Bernhard Bauer wrote:
>
https://codereview.chromium.org/914933002/diff/40001/chrome/android/shell/jav...
> File
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
> (right):
> 
>
https://codereview.chromium.org/914933002/diff/40001/chrome/android/shell/jav...
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:261:
> * Shows or hides the add and the stop/reload button .
> Nit: remove the space before the period.
> 
>
https://codereview.chromium.org/914933002/diff/40001/chrome/android/shell/jav...
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:262:
> * @param visibility The visibility status of the add button.
> Javadoc comment is now outdated.

PTAL

[Bernhard Bauer, 2015-02-12 12:13:31]

lgtm

[divya.bansal, 2015-02-12 12:13:48]

The CQ bit was checked by divya.bansal@samsung.com

[commit-bot: I haz the power, 2015-02-12 12:14:49]

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

[divya.bansal, 2015-02-12 12:15:06]

On 2015/02/12 12:13:31, Bernhard Bauer wrote:
> lgtm

@Bernhard Thanks for the review.

[commit-bot: I haz the power, 2015-02-12 12:34:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-12 12:34:54]

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