Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2486003004:
[TTS][Overlay] Fix content width overflowing narrow panel. (Closed)
|
Created:
4 years, 1 month ago by Donn Denman Modified:
4 years, 1 month ago Reviewers:
mdjones CC:
chromium-reviews, mdjones+watch_chromium.org, donnd+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TTS][Overlay] Fix content width overflowing narrow panel.
When the top controls are shown by the Browser, the Overlay Panel gets
many onSizeChanged events while the controls animate off the top. This
can cause the panel content to overflow a narrow panel on tablet.
The panel now ignores the onSizeChanged calls if they have the same
height as previous calls.
BUG=632483
Committed: https://crrev.com/3f933d51ebdb5027f27e79a9a816857bbdee12ba
Cr-Commit-Position: refs/heads/master@{#431345}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Just changed variable names and comments. #Patch Set 3 : Check if the width changed too! #Messages
Total messages: 15 (7 generated)
donnd@chromium.org changed reviewers: + mdjones@chromium.org
Matt, PTAL.
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java (right): https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:125: /** Cache the previous height of the Overlay Panel to filter onSizeChanged */ nit: + " events." https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:807: // We only care if the orientation is changing, which will certainly change the height. Can you mention that "height" is the window's height and not the visible content height which is why this works? https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:809: mPreviousHeight = height; This isn't really previous height if it always contains the current value. Maybe "mCachedHeight"?
https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java (right): https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:809: mPreviousHeight = height; On 2016/11/10 16:21:39, mdjones wrote: > This isn't really previous height if it always contains the current value. Maybe > "mCachedHeight"? Ignore this comment, the reason I posted this out of context with this change.
Thanks Matt, PTAL. https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java (right): https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:125: /** Cache the previous height of the Overlay Panel to filter onSizeChanged */ On 2016/11/10 16:21:39, mdjones wrote: > nit: + " events." Done, also clarified which class that method is in. https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:807: // We only care if the orientation is changing, which will certainly change the height. On 2016/11/10 16:21:39, mdjones wrote: > Can you mention that "height" is the window's height and not the visible content > height which is why this works? Done sort of. Using the terms defined by the caller -- viewport height of the screen. https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:809: mPreviousHeight = height; On 2016/11/10 16:21:39, mdjones wrote: > This isn't really previous height if it always contains the current value. Maybe > "mCachedHeight"? Done. https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:809: mPreviousHeight = height; On 2016/11/10 16:36:50, mdjones wrote: > On 2016/11/10 16:21:39, mdjones wrote: > > This isn't really previous height if it always contains the current value. > Maybe > > "mCachedHeight"? > > Ignore this comment, the reason I posted this out of context with this change. Acknowledged. https://codereview.chromium.org/2486003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java:809: mPreviousHeight = height; On 2016/11/10 16:21:39, mdjones wrote: > This isn't really previous height if it always contains the current value. Maybe > "mCachedHeight"? Changed to mViewportHeight.
lgtm
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [TTS][Overlay] Fix content width overflowing narrow panel. When the top controls are shown by the Browser, the Overlay Panel gets many onSizeChanged events while the controls animate off the top. This can cause the panel content to overflow a narrow panel on tablet. The panel now ignores the onSizeChanged calls if they have the same height as previous calls. BUG=632483 ========== to ========== [TTS][Overlay] Fix content width overflowing narrow panel. When the top controls are shown by the Browser, the Overlay Panel gets many onSizeChanged events while the controls animate off the top. This can cause the panel content to overflow a narrow panel on tablet. The panel now ignores the onSizeChanged calls if they have the same height as previous calls. BUG=632483 Committed: https://crrev.com/3f933d51ebdb5027f27e79a9a816857bbdee12ba Cr-Commit-Position: refs/heads/master@{#431345} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3f933d51ebdb5027f27e79a9a816857bbdee12ba Cr-Commit-Position: refs/heads/master@{#431345} |
