Making suggestion popup scrollable in landscape mode

Making suggestion popup scrollable in landscape mode

Setting height for Suggestion popup correctly for portrait
as well as landscape mode so that it is scrollable and does
not overlap with IME.

BUG=409619
Committed: https://crrev.com/3d52a11f9d247d937c74983c32b95fac39456945
Cr-Commit-Position: refs/heads/master@{#292917}

[ankit, 2014-09-01 12:58:57]

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

[ankit, 2014-09-01 12:58:58]

PTAL

[Bernhard Bauer, 2014-09-01 13:45:11]

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
(right):

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:110:
int height = dropDownItemHeight;
This value isn't used (and the variable only inside of the if-statement), so
remove this line and declare the variable on line 112.

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:112:
height = mSuggestionsPopup.getListView().getCount() * dropDownItemHeight;
Instead of asking the view for its number of items, you could ask the adapter,
which actually holds the items.

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:113:
if (appRect.height() > height)
I don't quite understand this. If the height of the whole list is less than the
app window, we return the list height, but otherwise we return the app window
height, but minus the height of one item. Don't we also need to subtract that
one item here?

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:116:
//maintaining margin height of dropdDownItemHeight
Add a space after //, capitalize the first character of the sentence, put
|dropDownItemHeight| between pipe symbols, spell it correctly, and end the
sentence with a period please.

[ankit, 2014-09-02 04:20:12]

@Bernhard
PTAL new patch.

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
(right):

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:110:
int height = dropDownItemHeight;
On 2014/09/01 13:45:10, Bernhard Bauer wrote:
> This value isn't used (and the variable only inside of the if-statement), so
> remove this line and declare the variable on line 112.

Done.

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:112:
height = mSuggestionsPopup.getListView().getCount() * dropDownItemHeight;
On 2014/09/01 13:45:10, Bernhard Bauer wrote:
> Instead of asking the view for its number of items, you could ask the adapter,
> which actually holds the items.
Stored value in a local variable and using it instead of asking the view.

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:113:
if (appRect.height() > height)
On 2014/09/01 13:45:10, Bernhard Bauer wrote:
> I don't quite understand this. If the height of the whole list is less than
the
> app window, we return the list height, but otherwise we return the app window
> height, but minus the height of one item. Don't we also need to subtract that
> one item here?

Done.

https://codereview.chromium.org/517403003/diff/1/chrome/android/shell/java/sr...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:116:
//maintaining margin height of dropdDownItemHeight
On 2014/09/01 13:45:10, Bernhard Bauer wrote:
> Add a space after //, capitalize the first character of the sentence, put
> |dropDownItemHeight| between pipe symbols, spell it correctly, and end the
> sentence with a period please.

Done.

[Bernhard Bauer, 2014-09-02 08:54:16]

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
(right):

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:113:
int height = dropDownItemHeight;
This assignment is unnecessary.

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:115:
if (height < appRect.height())
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.

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:120:
(popupHeight - dropDownItemHeight) : popupHeight;
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]

@Bernhard
PTAL

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
(right):

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:113:
int height = dropDownItemHeight;
On 2014/09/02 08:54:15, Bernhard Bauer wrote:
> This assignment is unnecessary.

Done.

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:115:
if (height < appRect.height())
On 2014/09/02 08:54:15, Bernhard Bauer wrote:
> 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.

Done.

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:120:
(popupHeight - dropDownItemHeight) : popupHeight;
On 2014/09/02 08:54:15, Bernhard Bauer wrote:
> 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).

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:02]

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
(right):

https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:120:
(popupHeight - dropDownItemHeight) : popupHeight;
On 2014/09/02 09:23:04, ankit wrote:
> On 2014/09/02 08:54:15, Bernhard Bauer wrote:
> > 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).
> 
> 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.

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?

[ankit, 2014-09-02 09:51:48]

On 2014/09/02 09:31:02, Bernhard Bauer wrote:
>
https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
> File
>
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java
> (right):
> 
>
https://codereview.chromium.org/517403003/diff/20001/chrome/android/shell/jav...
>
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionPopup.java:120:
> (popupHeight - dropDownItemHeight) : popupHeight;
> On 2014/09/02 09:23:04, ankit wrote:
> > On 2014/09/02 08:54:15, Bernhard Bauer wrote:
> > > 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).
> > 
> > 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.
> 
> 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?

Thanks for clarification. 
Margin only applies when we are constrained by app rect. Made changes
accordingly.
PTAL new patch

[Bernhard Bauer, 2014-09-02 10:10:52]

LGTM!

[ankit, 2014-09-02 10:12:39]

On 2014/09/02 10:10:52, Bernhard Bauer wrote:
> LGTM!

Thanks.

[ankit, 2014-09-02 10:12:43]

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

[commit-bot: I haz the power, 2014-09-02 10:13:48]

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

[commit-bot: I haz the power, 2014-09-02 13:56:07]

Committed patchset #5 (id:80001) as 05f8975e73c14ebe264873bb5464579b9962d620

[commit-bot: I haz the power, 2014-09-10 03:18:13]

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