Add history popup when long pressing on the back key.

Add history popup when long pressing on the back key.

BUG=150316
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175195

[David Trainor- moved to gerrit, 2013-01-03 18:57:31]

lgtm.  Mostly nits.

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java
(right):

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:38:
private static final String HISTORY_URL = "chrome://history/";
Is this in some native resource?

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:40:
private static final int LIST_ITEM_HEIGHT_DP = 48;
These should probably be in res/dimens.xml

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:45:
private static final int MAXIMUM_HISTORY_ITEMS = 8;
res/values.xml?

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:65:
void openHistory();
Does the implementer have to know what HISTORY_URL is?  Should it be a public
static final member of this class?

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:82:
mDelegate = delegate;
assert delegate isn't null since we don't null check below?

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:97:
FixedViewInfo footerInfo = new ListView(context).new FixedViewInfo();
Gah this is how you build the FixedViewInfo?  Weird.

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:99:
mShowHistoryView.setText(R.string.show_history_label);
Does this string already exist natively?

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:149: if
(!TextUtils.equals(url, entry.getUrl())) continue;
Might be a cleaner flow to get rid of ! and just make it a one line if
statement?  I think it'll fit.

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/navigation_popup.cc (right):

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/navigation_popup.cc:72: ScopedJavaLocalRef<jstring>
jurl =
For things like this use jurl(base::android::ConvertUTF8ToJavaString(...)); if
you can as it saves running the default constructor followed by the =operator
instead of just the copy constructor (or so I was told a long time ago).

Not sure if this class supports it but worth a shot.

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/navigation_popup.h (right):

https://chromiumcodereview.appspot.com/11743011/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/navigation_popup.h:23: virtual ~NavigationPopup();
If this can't be destroyed by anything but Destroy(), should make this
protected.

https://chromiumcodereview.appspot.com/11743011/diff/1/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://chromiumcodereview.appspot.com/11743011/diff/1/content/browser/androi...
content/browser/android/content_view_core_impl.cc:1170:
ScopedJavaLocalRef<jstring> j_url = ConvertUTF8ToJavaString(
Try to use the copy constructor if you can for these.

https://chromiumcodereview.appspot.com/11743011/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/11743011/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:892:
public void goToNavigationIndex(int index) {
javadoc!?  What happens if index is out of bounds?

[Ted C, 2013-01-03 19:56:41]

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java
(right):

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:38:
private static final String HISTORY_URL = "chrome://history/";
On 2013/01/03 18:57:31, dtrainor wrote:
> Is this in some native resource?

Changed it so it's pulling it from native.

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:40:
private static final int LIST_ITEM_HEIGHT_DP = 48;
On 2013/01/03 18:57:31, dtrainor wrote:
> These should probably be in res/dimens.xml

I'd prefer not until it needs to be configuration for different configurations. 
Reading from XML is slow and it's bitten me in the past for other UI bits.

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:65:
void openHistory();
On 2013/01/03 18:57:31, dtrainor wrote:
> Does the implementer have to know what HISTORY_URL is?  Should it be a public
> static final member of this class?

I exposed a method getHistoryUrl that pulls it from native.

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:82:
mDelegate = delegate;
On 2013/01/03 18:57:31, dtrainor wrote:
> assert delegate isn't null since we don't null check below?

Done.

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:97:
FixedViewInfo footerInfo = new ListView(context).new FixedViewInfo();
On 2013/01/03 18:57:31, dtrainor wrote:
> Gah this is how you build the FixedViewInfo?  Weird.

Lame I know...but it's the only way :-/.  We do the same for the menu popup.

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:99:
mShowHistoryView.setText(R.string.show_history_label);
On 2013/01/03 18:57:31, dtrainor wrote:
> Does this string already exist natively?

I think in android we do different casing, so it's not exactly the same.

https://codereview.chromium.org/11743011/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:149: if
(!TextUtils.equals(url, entry.getUrl())) continue;
On 2013/01/03 18:57:31, dtrainor wrote:
> Might be a cleaner flow to get rid of ! and just make it a one line if
> statement?  I think it'll fit.

Much nicer...done.

https://codereview.chromium.org/11743011/diff/1/chrome/browser/ui/android/nav...
File chrome/browser/ui/android/navigation_popup.cc (right):

https://codereview.chromium.org/11743011/diff/1/chrome/browser/ui/android/nav...
chrome/browser/ui/android/navigation_popup.cc:72: ScopedJavaLocalRef<jstring>
jurl =
On 2013/01/03 18:57:31, dtrainor wrote:
> For things like this use jurl(base::android::ConvertUTF8ToJavaString(...)); if
> you can as it saves running the default constructor followed by the =operator
> instead of just the copy constructor (or so I was told a long time ago).
> 
> Not sure if this class supports it but worth a shot.

Done.  Updated everywhere that I saw.

https://codereview.chromium.org/11743011/diff/1/chrome/browser/ui/android/nav...
File chrome/browser/ui/android/navigation_popup.h (right):

https://codereview.chromium.org/11743011/diff/1/chrome/browser/ui/android/nav...
chrome/browser/ui/android/navigation_popup.h:23: virtual ~NavigationPopup();
On 2013/01/03 18:57:31, dtrainor wrote:
> If this can't be destroyed by anything but Destroy(), should make this
> protected.

Done.

https://codereview.chromium.org/11743011/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/11743011/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1170:
ScopedJavaLocalRef<jstring> j_url = ConvertUTF8ToJavaString(
On 2013/01/03 18:57:31, dtrainor wrote:
> Try to use the copy constructor if you can for these.

Done.

https://codereview.chromium.org/11743011/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/11743011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:892:
public void goToNavigationIndex(int index) {
On 2013/01/03 18:57:31, dtrainor wrote:
> javadoc!?  What happens if index is out of bounds?

It's behavior for out of bounds is determined by the implementation of the
navigation controller, so it seems odd to me to specify that here (or any of the
other methods for that matter as we don't actually check the input ourselves).

[Ted C, 2013-01-03 22:31:55]

+thakis for chrome*.gypi changes

[Nico, 2013-01-03 22:48:51]

lgtm stamp because I'm clueless about Android. To prove that, a clueless remark
below:

(Also, I observe that this CL adds new features but no new tests – boo!)

https://codereview.chromium.org/11743011/diff/8001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java
(right):

https://codereview.chromium.org/11743011/diff/8001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:155: if
(TextUtils.equals(url, entry.getUrl())) entry.updateFavicon((Bitmap) favicon);
nit: break before if body

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/navigation_popup.cc (right):

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/navigation_popup.cc:81:
gfx::ConvertToJavaBitmap(image.ToSkBitmap()).obj());
This will only give you the lowest-resolution variant of a favicon. Is that what
you want? On Cros and Mac, we try to show higher-res variants on higher-res
displays (generally, by keeping them all in memory and picking the right one at
draw time. With tablets / phones, I suppose the scale factor doesn't change
often and you can just pick the right one here?)

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/navigation_popup.h (right):

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/navigation_popup.h:20: class NavigationPopup {
class comment

[Ted C, 2013-01-04 01:11:45]

Test added for the navigation querying.  I'll work on trying to do something for
the NavigationPopup

https://codereview.chromium.org/11743011/diff/8001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java
(right):

https://codereview.chromium.org/11743011/diff/8001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:155: if
(TextUtils.equals(url, entry.getUrl())) entry.updateFavicon((Bitmap) favicon);
On 2013/01/03 22:48:51, Nico wrote:
> nit: break before if body

http://source.android.com/source/code-style.html#use-standard-brace-style

If the condition and body fit on one line, then it's ok to put it all on one. 
We use this fairly often in the android land.

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/navigation_popup.cc (right):

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/navigation_popup.cc:81:
gfx::ConvertToJavaBitmap(image.ToSkBitmap()).obj());
On 2013/01/03 22:48:51, Nico wrote:
> This will only give you the lowest-resolution variant of a favicon. Is that
what
> you want? On Cros and Mac, we try to show higher-res variants on higher-res
> displays (generally, by keeping them all in memory and picking the right one
at
> draw time. With tablets / phones, I suppose the scale factor doesn't change
> often and you can just pick the right one here?)

Hmm...I think this needs to be done when I request the favicon in
FetchFaviconForUrl.  I currently hardcode 16 as my desired dp, but really I
should passed a scaled value.  I added a TODO that will will handle passing down
scale (or calculating here if android provides the density correctly somewhere).

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/navigation_popup.h (right):

https://codereview.chromium.org/11743011/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/navigation_popup.h:20: class NavigationPopup {
On 2013/01/03 22:48:51, Nico wrote:
> class comment

Done.

[Ted C, 2013-01-04 03:14:50]

Alrighty...added tests for the popup as well.

[David Trainor- moved to gerrit, 2013-01-04 18:11:11]

lgtm

https://chromiumcodereview.appspot.com/11743011/diff/1/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/11743011/diff/1/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:892:
public void goToNavigationIndex(int index) {
Fair enough!
On 2013/01/03 19:56:41, Ted C wrote:
> On 2013/01/03 18:57:31, dtrainor wrote:
> > javadoc!?  What happens if index is out of bounds?
> 
> It's behavior for out of bounds is determined by the implementation of the
> navigation controller, so it seems odd to me to specify that here (or any of
the
> other methods for that matter as we don't actually check the input ourselves).

https://chromiumcodereview.appspot.com/11743011/diff/8017/chrome/android/java...
File
chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java
(right):

https://chromiumcodereview.appspot.com/11743011/diff/8017/chrome/android/java...
chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java:122:
}));
Should there be a string component for this assert since it won't be plainly
obvious what failed in the logs?

[commit-bot: I haz the power, 2013-01-04 18:20:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11743011/7006

[commit-bot: I haz the power, 2013-01-04 19:10:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11743011/16001

[commit-bot: I haz the power, 2013-01-04 21:23:16]

Change committed as 175195