Making suggestion popup scrollable in landscape mode

chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java

Index: chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
diff --git a/chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java b/chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
index 883918adac4389ee24c20c8c79c44d17f027598c..53147c60b3a056ef8cbfd611f409b18e59321b1b 100644
--- a/chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
+++ b/chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
@@ -5,6 +5,7 @@
 package org.chromium.chrome.shell.omnibox;
 
 import android.content.Context;
+import android.graphics.Rect;
 import android.os.Handler;
 import android.text.Editable;
 import android.text.TextUtils;
@@ -20,6 +21,7 @@ import android.widget.TextView;
 import org.chromium.chrome.browser.omnibox.AutocompleteController;
 import org.chromium.chrome.browser.omnibox.AutocompleteController.OnSuggestionsReceivedListener;
 import org.chromium.chrome.browser.omnibox.OmniboxSuggestion;
+import org.chromium.chrome.shell.ChromeShellActivity;
 import org.chromium.chrome.shell.ChromeShellToolbar;
 import org.chromium.chrome.shell.R;
 
@@ -40,6 +42,7 @@ public class SuggestionPopup implements OnSuggestionsReceivedListener, TextWatch
     private Runnable mRequestSuggestions;
     private ListPopupWindow mSuggestionsPopup;
     private SuggestionArrayAdapter mSuggestionArrayAdapter;
+    private int mSuggestionsPopupItemsCount;
 
     /**
      * Initializes a suggestion popup that will track urlField value and display suggestions based
@@ -57,6 +60,7 @@ public class SuggestionPopup implements OnSuggestionsReceivedListener, TextWatch
                     int oldLeft, int oldTop, int oldRight, int oldBottom) {
                 if (mSuggestionsPopup == null || !mSuggestionsPopup.isShowing()) return;
                 mSuggestionsPopup.setWidth(mUrlField.getWidth());
+                mSuggestionsPopup.setHeight(getSuggestionPopupHeight());
                 mSuggestionsPopup.show();
             }
         };
@@ -98,13 +102,31 @@ public class SuggestionPopup implements OnSuggestionsReceivedListener, TextWatch
         if (mRequestSuggestions != null) mRequestSuggestions = null;
     }
 
-    // OnSuggestionsReceivedListener implementation
+    private int getSuggestionPopupHeight() {
+        Rect appRect = new Rect();
+        ((ChromeShellActivity)mContext).getWindow().getDecorView().
+                getWindowVisibleDisplayFrame(appRect);
+        int dropDownItemHeight = mContext.getResources().
+                getDimensionPixelSize(R.dimen.dropdown_item_height);
+        int popupHeight = appRect.height();
+        if (mSuggestionsPopup != null) {
+            int height = dropDownItemHeight;

[Bernhard Bauer, 2014/09/02 08:54:15]

This assignment is unnecessary.

[ankit, 2014/09/02 09:23:04]

Done.

+            height = mSuggestionsPopupItemsCount * dropDownItemHeight;
+            if (height < appRect.height())

[Bernhard Bauer, 2014/09/02 08:54:15]

You could use popupHeight instead of appRect.height() here, which would IMO make
it clearer that you're using the smaller one out of the two values.

[ankit, 2014/09/02 09:23:04]

Done.

+                popupHeight = height;
+        }
+        // Maintaining margin height equal to |dropDownItemHeight|.
+        return (popupHeight - dropDownItemHeight > 0) ?
+                (popupHeight - dropDownItemHeight) : popupHeight;

[Bernhard Bauer, 2014/09/02 08:54:15]

There's still a discontinuity here: As |popupHeight| gets smaller, the returned
value goes down until it reaches zero, then it jumps up again to |popupHeight|.
Would it make sense to clamp at 0 instead? Then you could just write this as
Math.max(popupHeight - dropDownItemHeight, 0) and avoid having `popupHeight -
dropDownItemHeight` twice.

(If we keep the logic as it currently is, I'd still suggest extracting that
value to a local variable).

[ankit, 2014/09/02 09:23:04]

As per current logic when number of suggestion item is one, In that case
maintaining margin will make popup height zero. Avoiding that case with this
check. Problem will not be solved by clamping at 0 as in that case height should
not be 0. 
Please suggest if we can achieve same in better way.

[Bernhard Bauer, 2014/09/02 09:31:01]

Hm, now that I think about it, doesn't the margin only apply when we're
constrained by the app rect? That is, when the popup is big, we want it not to
be bigger than the app rect (lines 115-116), and on top of that we want to have
a margin too. If that is correct, then we should apply the margin when comparing
to the app rect, i.e. in line 111 or so. Or am I misunderstanding the behavior
you want to achieve here?

+    }
 
+    // OnSuggestionsReceivedListener implementation
     @Override
     public void onSuggestionsReceived(List<OmniboxSuggestion> suggestions,
             String inlineAutocompleteText) {
         if (!mUrlField.isFocused() || suggestions.isEmpty())
             return;
+        mSuggestionsPopupItemsCount = suggestions.size();
         if (mSuggestionsPopup == null) {
             mSuggestionsPopup = new ListPopupWindow(
                     mContext, null, android.R.attr.autoCompleteTextViewStyle);
@@ -116,9 +138,11 @@ public class SuggestionPopup implements OnSuggestionsReceivedListener, TextWatch
                 }
             });
         }
+        mSuggestionsPopup.setInputMethodMode(ListPopupWindow.INPUT_METHOD_NEEDED);
         mSuggestionsPopup.setWidth(mUrlField.getWidth());
         mSuggestionArrayAdapter =
                 new SuggestionArrayAdapter(mContext, R.layout.dropdown_item, suggestions);
+        mSuggestionsPopup.setHeight(getSuggestionPopupHeight());
         mSuggestionsPopup.setAdapter(mSuggestionArrayAdapter);
         mSuggestionsPopup.setAnchorView(mUrlField);
         mSuggestionsPopup.setOnItemClickListener(new OnItemClickListener() {