Adding option for Stop/Reload in location bar for chrome_shell.

Adding option for Stop/Reload in location bar for chrome_shell.

Currently reload/stop button is not present in chrome shell. If user
has to do reload operation then only option to do that is select
the url bar and click go button. Similarly stop button is not present.
To stop loading of page only option is to press back button.
Added code to support stop/reload option.

BUG=

Committed: https://crrev.com/ea300714c42b5653189b49910f39f33202cea774
Cr-Commit-Position: refs/heads/master@{#293421}

[ankit, 2014-09-04 12:52:04]

ankit2.kumar@samsung.com changed reviewers:
+ bauerb@chromium.org, tedchoc@chromium.org

[ankit, 2014-09-04 12:52:05]

PTAL

[Bernhard Bauer, 2014-09-04 13:17:35]

Where did you get those images from?

Do we fall back to lower-res images if high-res images aren't available? If yes,
then I'd just leave the upscaled versions out.

https://codereview.chromium.org/541713002/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
(right):

https://codereview.chromium.org/541713002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:94:
mLoading = progress == 100 ? false : true;
Don't use false and true in a ?: expression. Just invert the condition.

https://codereview.chromium.org/541713002/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:190:
if (mLoading) mTab.getContentViewCore().stopLoading();
Is the loading check actually necessary? The button shouldn't be visible if
we're not loading the page.

[ankit, 2014-09-04 13:38:31]

@Bernhard
PTAL new patch

> Where did you get those images from?
For stop I am using button provided by android.
For reload I am using button similar to content_shell reload button.

> Do we fall back to lower-res images if high-res images aren't available? If
yes,
> then I'd just leave the upscaled versions out.
I am not sure about we fall back to lower-res images if high-res images aren't
available.
But removing high-res images are not causing any build error. 
Please suggest if we can remove high-res images.

>
https://codereview.chromium.org/541713002/diff/1/chrome/android/shell/java/sr...
> File
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java
> (right):
> 
>
https://codereview.chromium.org/541713002/diff/1/chrome/android/shell/java/sr...
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:94:
> mLoading = progress == 100 ? false : true;
> Don't use false and true in a ?: expression. Just invert the condition.
Done

>
https://codereview.chromium.org/541713002/diff/1/chrome/android/shell/java/sr...
>
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:190:
> if (mLoading) mTab.getContentViewCore().stopLoading();
> Is the loading check actually necessary? The button shouldn't be visible if
> we're not loading the page.
Removed mLoading check as button will not be visible.

[Bernhard Bauer, 2014-09-04 13:59:41]

OK, if I understand
http://developer.android.com/guide/practices/screens_support.html correctly, we
will use the best available image for the screen pixel density. So, I would
remove any images that are simply upscaled versions of lower-res images (leaving
the scaling to Android).

https://codereview.chromium.org/541713002/diff/20001/chrome/android/java/res/...
File chrome/android/java/res/drawable/btn_reload.xml (right):

https://codereview.chromium.org/541713002/diff/20001/chrome/android/java/res/...
chrome/android/java/res/drawable/btn_reload.xml:10: <item
android:drawable="@drawable/btn_reload_normal" />
Is there a reason why this is not in c/a/shell/? Also, could we get rid of the
XML file and just directly use the image as a drawable?

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

https://codereview.chromium.org/541713002/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:94:
mLoading = progress != 100;
And now this doesn't need to be a member variable anymore.

https://codereview.chromium.org/541713002/diff/20001/chrome/android/shell/res...
File chrome/android/shell/res/layout/chrome_shell_activity.xml (right):

https://codereview.chromium.org/541713002/diff/20001/chrome/android/shell/res...
chrome/android/shell/res/layout/chrome_shell_activity.xml:29:
android:visibility="gone"/>
Space before closing the tag.

[ankit, 2014-09-04 14:15:54]

@Bernhard
PTAL new patch.
Thanks for guidance.

https://codereview.chromium.org/541713002/diff/20001/chrome/android/java/res/...
File chrome/android/java/res/drawable/btn_reload.xml (right):

https://codereview.chromium.org/541713002/diff/20001/chrome/android/java/res/...
chrome/android/java/res/drawable/btn_reload.xml:10: <item
android:drawable="@drawable/btn_reload_normal" />
On 2014/09/04 13:59:41, Bernhard Bauer wrote:
> Is there a reason why this is not in c/a/shell/? Also, could we get rid of the
> XML file and just directly use the image as a drawable?

Done.
Removed file and used image directly.

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

https://codereview.chromium.org/541713002/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellToolbar.java:94:
mLoading = progress != 100;
On 2014/09/04 13:59:41, Bernhard Bauer wrote:
> And now this doesn't need to be a member variable anymore.

Done.

https://codereview.chromium.org/541713002/diff/20001/chrome/android/shell/res...
File chrome/android/shell/res/layout/chrome_shell_activity.xml (right):

https://codereview.chromium.org/541713002/diff/20001/chrome/android/shell/res...
chrome/android/shell/res/layout/chrome_shell_activity.xml:29:
android:visibility="gone"/>
On 2014/09/04 13:59:41, Bernhard Bauer wrote:
> Space before closing the tag.

Done.

[Bernhard Bauer, 2014-09-04 15:16:46]

LGTM!

[ankit, 2014-09-04 15:38:10]

On 2014/09/04 15:16:46, Bernhard Bauer wrote:
> LGTM!

Thanks

[ankit, 2014-09-04 15:38:14]

The CQ bit was checked by ankit2.kumar@samsung.com

[commit-bot: I haz the power, 2014-09-04 15:39:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ankit2.kumar@samsung.com/541713002/40001

[commit-bot: I haz the power, 2014-09-04 16:30:33]

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

[commit-bot: I haz the power, 2014-09-04 16:30:34]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[ankit, 2014-09-04 17:37:07]

The CQ bit was checked by ankit2.kumar@samsung.com

[commit-bot: I haz the power, 2014-09-04 17:50:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ankit2.kumar@samsung.com/541713002/40001

[commit-bot: I haz the power, 2014-09-04 18:50:32]

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

[commit-bot: I haz the power, 2014-09-04 18:50:32]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[ankit, 2014-09-05 03:27:36]

The CQ bit was checked by ankit2.kumar@samsung.com

[commit-bot: I haz the power, 2014-09-05 03:30:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ankit2.kumar@samsung.com/541713002/40001

[commit-bot: I haz the power, 2014-09-05 03:33:09]

Committed patchset #3 (id:40001) as bc6aa3661113e8e0e51450e175d2e603371d528d

[aurimas (slooooooooow), 2014-09-05 17:01:34]

On 2014/09/05 03:33:09, I haz the power (commit-bot) wrote:
> Committed patchset #3 (id:40001) as bc6aa3661113e8e0e51450e175d2e603371d528d

This is not OK.

1. Why are we including these resources in chrome/android/java/res/ even thought
it is only used for ChromeShell ? This makes it so that they get included for
everyone who builds chrome target. These png resources should live in
chrome/android/shell/res/
2. chrome_shell_activity.xml has tabs on lines 19 and 24.
3. Why are we using two buttons if only one of them is visible at a time? Just
use one button and just change the drawable that is shown.

Please make a followup CL that fixes the issues above.

[ankit, 2014-09-06 04:07:16]

On 2014/09/05 17:01:34, aurimas wrote:
> On 2014/09/05 03:33:09, I haz the power (commit-bot) wrote:
> > Committed patchset #3 (id:40001) as bc6aa3661113e8e0e51450e175d2e603371d528d
> 
> This is not OK.
> 
> 1. Why are we including these resources in chrome/android/java/res/ even
thought
> it is only used for ChromeShell ? This makes it so that they get included for
> everyone who builds chrome target. These png resources should live in
> chrome/android/shell/res/
> 2. chrome_shell_activity.xml has tabs on lines 19 and 24.
> 3. Why are we using two buttons if only one of them is visible at a time? Just
> use one button and just change the drawable that is shown.
> 
> Please make a followup CL that fixes the issues above.

@aurimas
Thanks for suggestion.
I will make a followup CL fixing above issues.

[commit-bot: I haz the power, 2014-09-10 03:36:16]

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