Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
421173002:
Paste PopupMenu gets cropped by StatusBar (Closed)
|
Created:
6 years, 4 months ago by kingshuk.j Modified:
6 years, 4 months ago Reviewers:
jdduke (slow) CC:
chromium-reviews, darin-cc_chromium.org, jam, AKVT, AviD, ankit, Cyan, aurimas (slooooooooow), cjhopman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Android] Account for status bar offset when position paste popup
The paste PopupMenu was getting clipped by the status bar when the insertion
handle was brought close to the status bar. Update the position of the PopupMenu
using the height of the status bar.
BUG=397915
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287467
Patch Set 1 #
Total comments: 2
Patch Set 2 : Paste PopupMenu is getting cropped #
Total comments: 2
Patch Set 3 : Calculating the statusbarHeight only once #
Total comments: 6
Patch Set 4 : Renamed variables and removed unnecessary code. #
Total comments: 2
Patch Set 5 : Modified line indentations #
Messages
Total messages: 19 (0 generated)
@jdduke PTAL
@aurimas, @cjhopman PTAL
https://codereview.chromium.org/421173002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:135: int resourceId = mContext.getResources().getIdentifier("status_bar_height", "dimen", "android"); What guarantees do we have that the status bar will be visible?
https://codereview.chromium.org/421173002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:135: int resourceId = mContext.getResources().getIdentifier("status_bar_height", "dimen", "android"); ok, I will put a check for the status bar visibility and upload.
@jdduke PTAL
https://codereview.chromium.org/421173002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:136: int resourceId = mContext.getResources() Could we cache the status bar height (mStatusBarHeight) upon creation, then only use it if the SYSTEM_UI_FLAG_VISIBLE condition holds?
https://codereview.chromium.org/421173002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:136: int resourceId = mContext.getResources() On 2014/07/29 15:37:02, jdduke wrote: > Could we cache the status bar height (mStatusBarHeight) upon creation, then only > use it if the SYSTEM_UI_FLAG_VISIBLE condition holds? Yes, statusBar height calculation can be done at the time of PastePopup menu creation.
@jdduke PTAL
In the future, please add a single reviewer, or only the minimal subset necessary for OWNER's approval unless their are particular reviewers you'd like to focus on a certain subset of the patch. Also, rather than stating the bug in the patch title, state what the patch *does*, and in the description state the problem and provide more details about how the solution fixes the problem. https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:85: mStatusBarHeight = 0; Nit: No need to initialize to 0, it will always be 0 upon creation (but please keep a newline between the mWidthOffsetX assignment and the status bar height calculation). https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:86: int resourceId = mContext.getResources() Can we call this statusBarHeightResourceId? https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:142: int statusBarHeight = 0; Let's rename this to something like |minOffsetY|.
https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:85: mStatusBarHeight = 0; On 2014/07/30 15:24:16, jdduke wrote: > Nit: No need to initialize to 0, it will always be 0 upon creation (but please > keep a newline between the mWidthOffsetX assignment and the status bar height > calculation). Done. https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:86: int resourceId = mContext.getResources() On 2014/07/30 15:24:16, jdduke wrote: > Can we call this statusBarHeightResourceId? Done. https://codereview.chromium.org/421173002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:142: int statusBarHeight = 0; On 2014/07/30 15:24:16, jdduke wrote: > Let's rename this to something like |minOffsetY|. Done.
https://codereview.chromium.org/421173002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:86: int statusBarHeightResourceId = mContext.getResources() Prefer line breaking on arguments, or if the RHS can fit on an entire line, after the equality, so these lines become: final int statusBarHeightResourceId = mContext.getResources().getIdentifier("status_bar_height", "dimen", "android"); and mStatusBarHeight = mContext.getResources().getDimensionPixelSize(statusBarHeightResourceId);
On 2014/07/31 17:07:39, jdduke wrote: > mStatusBarHeight = > mContext.getResources().getDimensionPixelSize(statusBarHeightResourceId); Hmm, the formatting there came out really bad, the second line should be indented by 8 spaces.
PTAL https://codereview.chromium.org/421173002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java (right): https://codereview.chromium.org/421173002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/PastePopupMenu.java:86: int statusBarHeightResourceId = mContext.getResources() On 2014/07/31 17:07:39, jdduke wrote: > Prefer line breaking on arguments, or if the RHS can fit on an entire line, > after the equality, so these lines become: > > final int statusBarHeightResourceId = > mContext.getResources().getIdentifier("status_bar_height", "dimen", > "android"); > > and > > mStatusBarHeight = > mContext.getResources().getDimensionPixelSize(statusBarHeightResourceId); Done.
OK, this looks good but please update the description contents per the email I just sent you, thanks.
On 2014/08/01 15:19:19, jdduke wrote: > OK, this looks good but please update the description contents per the email I > just sent you, thanks. Updated the description contents as requested. PTAL
lgtm (I tweaked the description, note that the "Description" in Rietveld is what lands with the patch commit, so the first line of the "Description" field is the title, and the "Title" field is only cosmetic and used by Rietveld).
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kingshuk.j@samsung.com/421173002/80001
Message was sent while issue was closed.
Change committed as 287467 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
