Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777773002:
Show the image header for the Context Menu (Closed)
DescriptionShow the image header for the Context Menu
This CL should show an image above the context menu. It also adds a
background as we wait for the image to load. Screenshots are below:
https://drive.google.com/open?id=0B6D5A57VLDpeajkxZTBwVWZMcm8
https://drive.google.com/open?id=0B6D5A57VLDpeZWtIWlR1UlRyQ3c
https://drive.google.com/open?id=0B6D5A57VLDpeY0dpU3lTT3NDb1k
BUG=655359
Review-Url: https://codereview.chromium.org/2777773002
Cr-Commit-Position: refs/heads/master@{#461254}
Committed: https://chromium.googlesource.com/chromium/src/+/88172028d81d3cedbf5f315bbbd0f280dd252ce4
Patch Set 1 #
Total comments: 25
Patch Set 2 : Fixed based off twellington's comments #
Total comments: 14
Patch Set 3 : Fixes based off twellington's comments #Patch Set 4 : Mostly rebasing off previous patches #Patch Set 5 : git rebase #
Total comments: 14
Patch Set 6 : updated based off dtrainor's comments #
Total comments: 2
Patch Set 7 : Needed to add tests #Patch Set 8 : Fixed based off twellington's comments #Patch Set 9 : Fixed a test #Patch Set 10 : Fixed based off dtrainor's comments #
Total comments: 16
Patch Set 11 : Fixed based of tedchoc's comments #
Total comments: 13
Patch Set 12 : Fixed nits #Patch Set 13 : I think I deleted on teh wrong branch? #Patch Set 14 : Why be public when you can be private #Dependent Patchsets: Messages
Total messages: 51 (30 generated)
jwanda@chromium.org changed reviewers: + twellington@chromium.org
Currently the other CL is out for review. I want your opinion on this before this goes out to the other reviewers. Thanks! https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/tabular_context_menu_page.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/tabular_context_menu_page.xml:22: tools:ignore="UseCompoundDrawables"> For those who are curious on why we're doing this: So basically a compund drawable could work if we could set the background and max height but since we can't we have to put this here. Using a linear layout around it allows us to apply the margins regardless of what is inside the views.
https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... File chrome/android/java/res/drawable/checkerboard_background.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/checkerboard_background.xml:4: found in the LICENSE file. --> nit: add a blank line after the Copyright. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/checkerboard_background.xml:8: <solid android:color="#FFFFFF" /> nit: @android:color/white https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/checkerboard_background.xml:14: <solid android:color="#EEE"/> nit: Let's define this as google_grey_200 in colors.xml. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... File chrome/android/java/res/drawable/tabular_context_menu_image_border.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/tabular_context_menu_image_border.xml:8: android:color="@color/google_grey_400"/> nit: does this fit on the line above? https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/tabular_context_menu_page.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/tabular_context_menu_page.xml:22: tools:ignore="UseCompoundDrawables"> On 2017/03/27 00:38:54, JJ wrote: > For those who are curious on why we're doing this: So basically a compund > drawable could work if we could set the background and max height but since we > can't we have to put this here. > > Using a linear layout around it allows us to apply the margins regardless of > what is inside the views. It would be good to capture this in a comment in the XML so that we remember why it wasn't implemented as a compound drawable. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:173: public void getThumbnail() { nit: this needs a JavaDoc https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:49: * @param contextMenuHelper This is used to retrieve the thumbnail natively from the Context nit: The {@link ContextMenuHelper} used to retrieve the thumbnail from native. Or something like that. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:107: Pair<Integer, List<ContextMenuItem>> itemGroup, int maxCount) { Can the title string id and list of items be separate parameters? Also, can the new param be a boolean used to control whether the context menu is for an image so that we can check a boolean below rather than checking if the title == R.string.contextmenu_image_title? https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:156: baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); I think this could be simplified: displayHeaderIfVisibleItems(...) // #displayHeaderIfVisibleItems() sets these two views to GONE if the header // text is empty, but they should still be visible because we have an image // to display. baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); baseLayout.findViewById(R.id.context_divider).setVisibility(View.VISIBLE); https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:183: imageView.setBackground(bm); This method seems more complex than needed. I think you can create a background XML with android:tileMode=repeat and set that as imageView background rather than programatically constructing the background. http://stackoverflow.com/questions/9279012/android-image-background-repeat
Thanks for the quick update! I appreciate it. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... File chrome/android/java/res/drawable/checkerboard_background.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/checkerboard_background.xml:4: found in the LICENSE file. --> On 2017/03/27 18:28:26, Theresa wrote: > nit: add a blank line after the Copyright. Done. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/checkerboard_background.xml:8: <solid android:color="#FFFFFF" /> On 2017/03/27 18:28:26, Theresa wrote: > nit: @android:color/white Done. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/checkerboard_background.xml:14: <solid android:color="#EEE"/> On 2017/03/27 18:28:26, Theresa wrote: > nit: Let's define this as google_grey_200 in colors.xml. Done. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... File chrome/android/java/res/drawable/tabular_context_menu_image_border.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/tabular_context_menu_image_border.xml:8: android:color="@color/google_grey_400"/> On 2017/03/27 18:28:26, Theresa wrote: > nit: does this fit on the line above? It does? I thought it was one line per attribute? https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/tabular_context_menu_page.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/tabular_context_menu_page.xml:22: tools:ignore="UseCompoundDrawables"> On 2017/03/27 18:28:26, Theresa wrote: > On 2017/03/27 00:38:54, JJ wrote: > > For those who are curious on why we're doing this: So basically a compund > > drawable could work if we could set the background and max height but since we > > can't we have to put this here. > > > > Using a linear layout around it allows us to apply the margins regardless of > > what is inside the views. > > It would be good to capture this in a comment in the XML so that we remember why > it wasn't implemented as a compound drawable. Done. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:173: public void getThumbnail() { On 2017/03/27 18:28:26, Theresa wrote: > nit: this needs a JavaDoc Done. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:49: * @param contextMenuHelper This is used to retrieve the thumbnail natively from the Context On 2017/03/27 18:28:26, Theresa wrote: > nit: The {@link ContextMenuHelper} used to retrieve the thumbnail from native. > > Or something like that. Done. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:107: Pair<Integer, List<ContextMenuItem>> itemGroup, int maxCount) { On 2017/03/27 18:28:26, Theresa wrote: > Can the title string id and list of items be separate parameters? Also, can the > new param be a boolean used to control whether the context menu is for an image > so that we can check a boolean below rather than checking if the title == > R.string.contextmenu_image_title? Yeah we can do that. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:156: baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); On 2017/03/27 18:28:26, Theresa wrote: > I think this could be simplified: > > displayHeaderIfVisibleItems(...) > > // #displayHeaderIfVisibleItems() sets these two views to GONE if the header > // text is empty, but they should still be visible because we have an image > // to display. > baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); > baseLayout.findViewById(R.id.context_divider).setVisibility(View.VISIBLE); Done. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:183: imageView.setBackground(bm); On 2017/03/27 18:28:26, Theresa wrote: > This method seems more complex than needed. I think you can create a background > XML with android:tileMode=repeat and set that as imageView background rather > than programatically constructing the background. > > http://stackoverflow.com/questions/9279012/android-image-background-repeat haha we tried. However it was deemed impossible if the drawable is an xml so we have to do it pragmatically.
jwanda@chromium.org changed reviewers: + dtrainor@chromium.org, tedchoc@chromium.org
dtrainor@chromium.org: Please review changes in all of the share code. tedchoc@chromium.org: Please review changes in in the .cc code (you're the only owner familiar with the context menu and also has .cc and .h level status on chrome/browser/ui/android)
https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... File chrome/android/java/res/drawable/tabular_context_menu_image_border.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/tabular_context_menu_image_border.xml:8: android:color="@color/google_grey_400"/> On 2017/03/27 20:47:12, JJ wrote: > On 2017/03/27 18:28:26, Theresa wrote: > > nit: does this fit on the line above? > > It does? I thought it was one line per attribute? Typically it is, but our shapes defined in XML are a notable exception in the code base. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:156: baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); On 2017/03/27 20:47:12, JJ wrote: > On 2017/03/27 18:28:26, Theresa wrote: > > I think this could be simplified: > > > > displayHeaderIfVisibleItems(...) > > > > // #displayHeaderIfVisibleItems() sets these two views to GONE if the header > > // text is empty, but they should still be visible because we have an image > > // to display. > > > baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); > > baseLayout.findViewById(R.id.context_divider).setVisibility(View.VISIBLE); > > Done. I meant the whole top of the method private displayImageHeader(...) { displayHeaderIfVisibleItems(params, baseLayout); // #displayHeaderIfVisibleItems() sets these two views to GONE if the header // text is empty, but they should still be visible because we have an image // to display. baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); baseLayout.findViewById(R.id.context_divider).setVisibility(View.VISIBLE); final ImageView imageView = (ImageView) baseLayout.findViewById(R.id.context_header_image); setBackgroundForImageView(imageView, resources); .... } https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_page.xml (right): https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_page.xml:23: <!-- Cannot use a compound drawable since it does not allow a good change in maxHeight and Does it allow a hacky change? https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_page.xml:33: android:minHeight="56dp" Should this be defined in dimens.xml as context_menu_header_image_min_size? https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:43: interface OnThumbnailReceivedListener { nit: This should probably have a JavaDoc too https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:174: * This gets the thumbnail of the current image that triggered the context menu. nit: s/This gets/Gets https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:198: public void setOnThumbnailReceivedListener(OnThumbnailReceivedListener listener) { nit: JavaDoc here too https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:103: * @return Returns a filled LinearLayout with all the context menu items nit: add @param isImage and @param maxCount to JavaDocs https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:179: BitmapDrawable bm = new BitmapDrawable(resources, bitmap); I think this can be simplified to something like: BitmapDrawable bm = new BitmapDrawable(activity.getResources(), BitmapFactory.decodeResource(activity.getResources(), R.drawable.checkerboard_background)); bm.setTileModeXY(...)
Next round of comments complete! Hit me up if it doesn't make sense. https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... File chrome/android/java/res/drawable/tabular_context_menu_image_border.xml (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/res/dra... chrome/android/java/res/drawable/tabular_context_menu_image_border.xml:8: android:color="@color/google_grey_400"/> On 2017/03/28 17:23:08, Theresa wrote: > On 2017/03/27 20:47:12, JJ wrote: > > On 2017/03/27 18:28:26, Theresa wrote: > > > nit: does this fit on the line above? > > > > It does? I thought it was one line per attribute? > > Typically it is, but our shapes defined in XML are a notable exception in the > code base. Interesting. Done! https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:156: baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); On 2017/03/28 17:23:08, Theresa wrote: > On 2017/03/27 20:47:12, JJ wrote: > > On 2017/03/27 18:28:26, Theresa wrote: > > > I think this could be simplified: > > > > > > displayHeaderIfVisibleItems(...) > > > > > > // #displayHeaderIfVisibleItems() sets these two views to GONE if the header > > > // text is empty, but they should still be visible because we have an image > > > // to display. > > > > > > baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); > > > baseLayout.findViewById(R.id.context_divider).setVisibility(View.VISIBLE); > > > > Done. > > I meant the whole top of the method > > private displayImageHeader(...) { > displayHeaderIfVisibleItems(params, baseLayout); > > // #displayHeaderIfVisibleItems() sets these two views to GONE if the header > // text is empty, but they should still be visible because we have an image > // to display. > > baseLayout.findViewById(R.id.context_header_layout).setVisibility(View.VISIBLE); > baseLayout.findViewById(R.id.context_divider).setVisibility(View.VISIBLE); > > final ImageView imageView = (ImageView) > baseLayout.findViewById(R.id.context_header_image); > setBackgroundForImageView(imageView, resources); > .... > } Oh whoops, apologies. https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/tabular_context_menu_page.xml (right): https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_page.xml:23: <!-- Cannot use a compound drawable since it does not allow a good change in maxHeight and On 2017/03/28 17:23:08, Theresa wrote: > Does it allow a hacky change? The hackiest of changes. (You'd have to measure the height subtract the change in text to figure out the height. Then for a drawable you'd have to programatically add the drawable in the background. It would end up being a mess. I changed it so that it is more definitive (while slightly lying) https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/tabular_context_menu_page.xml:33: android:minHeight="56dp" On 2017/03/28 17:23:08, Theresa wrote: > Should this be defined in dimens.xml as context_menu_header_image_min_size? Why not. https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:43: interface OnThumbnailReceivedListener { On 2017/03/28 17:23:08, Theresa wrote: > nit: This should probably have a JavaDoc too Done. https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:174: * This gets the thumbnail of the current image that triggered the context menu. On 2017/03/28 17:23:08, Theresa wrote: > nit: s/This gets/Gets Done. https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:198: public void setOnThumbnailReceivedListener(OnThumbnailReceivedListener listener) { On 2017/03/28 17:23:08, Theresa wrote: > nit: JavaDoc here too Done. https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:103: * @return Returns a filled LinearLayout with all the context menu items On 2017/03/28 17:23:08, Theresa wrote: > nit: add @param isImage and @param maxCount to JavaDocs Done. https://codereview.chromium.org/2777773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:179: BitmapDrawable bm = new BitmapDrawable(resources, bitmap); On 2017/03/28 17:23:08, Theresa wrote: > I think this can be simplified to something like: > > BitmapDrawable bm = new BitmapDrawable(activity.getResources(), > BitmapFactory.decodeResource(activity.getResources(), > R.drawable.checkerboard_background)); > bm.setTileModeXY(...) Doesn't work :< Doesn't keep the repetition up, sadly.
The CQ bit was checked by jwanda@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jwanda@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...
Friendly ping as it's been 24 hours c:
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:188: public void getThumbnail() { I actually think this (and maybe shareImage() with some rework) could probably be done with Callbacks. In this particular case, it looks like you're doing: setOnThumbnailReceivedListener(listener) getThumbnail() Could you just make it getThumbnail(Callback<byte[]> callback)? Maybe something like the following: getThumbnail(new Callback<byte[]>() { void onResult(byte[] thumbnail) {...}});? Native can trigger the callback directly too. The method signature would be better if you could return a Bitmap, so maybe that's preferable if you can make that work. Finally... if you don't like this, just pass OnThumbnailReceivedListener into getThumbnail()... https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:45: private ContextMenuHelper mContextMenuHelper; final? https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:174: private void setBackgroundForImageView(ImageView imageView, Resources resources) { This still makes me sad! :P https://codereview.chromium.org/2777773002/diff/80001/chrome/browser/ui/andro... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.cc:205: 0, gfx::Size(kShareImageMaxWidth, kShareImageMaxHeight), Our thumbnail size is the same size as the share? Just confirming this is on purpose.
Thanks so much for the reviews! I put in the admittedly easier change for now. If an LGTM comes in before it I'll leave as is but I've got an example in front of me too so I'll see what wins. https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:188: public void getThumbnail() { On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > I actually think this (and maybe shareImage() with some rework) could probably > be done with Callbacks. > > In this particular case, it looks like you're doing: > setOnThumbnailReceivedListener(listener) > getThumbnail() > > Could you just make it getThumbnail(Callback<byte[]> callback)? Maybe something > like the following: > > getThumbnail(new Callback<byte[]>() { void onResult(byte[] thumbnail) {...}});? > > Native can trigger the callback directly too. The method signature would be > better if you could return a Bitmap, so maybe that's preferable if you can make > that work. > > Finally... if you don't like this, just pass OnThumbnailReceivedListener into > getThumbnail()... You make an interesting point on that native can call the callback directly. For now I'll change it to just passing the onThumbnailReceivedListener into getThumbnail. I'll keep looking into the other method and some examples I've found later on to see if I can go that route cause it sounds interesting. https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:45: private ContextMenuHelper mContextMenuHelper; On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > final? final! https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:174: private void setBackgroundForImageView(ImageView imageView, Resources resources) { On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > This still makes me sad! :P It makes me even sadder! https://codereview.chromium.org/2777773002/diff/80001/chrome/browser/ui/andro... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.cc:205: 0, gfx::Size(kShareImageMaxWidth, kShareImageMaxHeight), On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > Our thumbnail size is the same size as the share? Just confirming this is on > purpose. When running this morning I thought about the display size and realized instead we could do 216dp * 4(xxxhdpi) = 864 pixels. Thought I could sneakily add that in before you said something but alas. You caught me before I got it in!
Follow up comment on the possible change with callbacks. In short it's almost possible but not exactly possible. https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:188: public void getThumbnail() { On 2017/03/30 16:00:25, JJ wrote: > On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > > I actually think this (and maybe shareImage() with some rework) could probably > > be done with Callbacks. > > > > In this particular case, it looks like you're doing: > > setOnThumbnailReceivedListener(listener) > > getThumbnail() > > > > Could you just make it getThumbnail(Callback<byte[]> callback)? Maybe > something > > like the following: > > > > getThumbnail(new Callback<byte[]>() { void onResult(byte[] thumbnail) > {...}});? > > > > Native can trigger the callback directly too. The method signature would be > > better if you could return a Bitmap, so maybe that's preferable if you can > make > > that work. > > > > Finally... if you don't like this, just pass OnThumbnailReceivedListener into > > getThumbnail()... > > You make an interesting point on that native can call the callback directly. For > now I'll change it to just passing the onThumbnailReceivedListener into > getThumbnail. I'll keep looking into the other method and some examples I've > found later on to see if I can go that route cause it sounds interesting. Checked it out! It's not impossible, but there are some clear roadblocks. Right now that means adding a param "RequestThumbnailForContextMenu" which means we'd need to change share. This is however a good spot for improving as the code is extremely similar and we can aaaaaaaaaalmost change it but it needs some refactoring to do it.
The CQ bit was checked by jwanda@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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
lgtm % nit https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:174: private void setBackgroundForImageView(ImageView imageView, Resources resources) { On 2017/03/30 16:00:26, JJ wrote: > On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > > This still makes me sad! :P > > It makes me even sadder! I concur, but I still haven't come up with a better solution. https://codereview.chromium.org/2777773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:160: headerTextView.setVisibility(View.VISIBLE); This and the line above should be handled by displayHeaderIfVisibleItems()
Thanks for the review! https://codereview.chromium.org/2777773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:160: headerTextView.setVisibility(View.VISIBLE); On 2017/03/30 18:34:34, Theresa wrote: > This and the line above should be handled by displayHeaderIfVisibleItems() You're right! I changed it for clearer intent.
The CQ bit was checked by jwanda@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by jwanda@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/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:188: public void getThumbnail() { On 2017/03/30 17:09:00, JJ wrote: > On 2017/03/30 16:00:25, JJ wrote: > > On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > > > I actually think this (and maybe shareImage() with some rework) could > probably > > > be done with Callbacks. > > > > > > In this particular case, it looks like you're doing: > > > setOnThumbnailReceivedListener(listener) > > > getThumbnail() > > > > > > Could you just make it getThumbnail(Callback<byte[]> callback)? Maybe > > something > > > like the following: > > > > > > getThumbnail(new Callback<byte[]>() { void onResult(byte[] thumbnail) > > {...}});? > > > > > > Native can trigger the callback directly too. The method signature would be > > > better if you could return a Bitmap, so maybe that's preferable if you can > > make > > > that work. > > > > > > Finally... if you don't like this, just pass OnThumbnailReceivedListener > into > > > getThumbnail()... > > > > You make an interesting point on that native can call the callback directly. > For > > now I'll change it to just passing the onThumbnailReceivedListener into > > getThumbnail. I'll keep looking into the other method and some examples I've > > found later on to see if I can go that route cause it sounds interesting. > > Checked it out! It's not impossible, but there are some clear roadblocks. Right > now that means adding a param "RequestThumbnailForContextMenu" which means we'd > need to change share. This is however a good spot for improving as the code is > extremely similar and we can aaaaaaaaaalmost change it but it needs some > refactoring to do it. You could probably just do it all with finals. For this function I think the below would be good: public void getThumbnail(final OnThumbnailReceivedListener listener) { Callback<byte[]> callback = new Callback<byte[]>() { @Override public void onResult(byte[] result) { listener.onThumbnailReceived(BitmapFactory.decodeByteArray(result, 0, result.length); } } } https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:174: private void setBackgroundForImageView(ImageView imageView, Resources resources) { On 2017/03/30 18:34:34, Theresa wrote: > On 2017/03/30 16:00:26, JJ wrote: > > On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > > > This still makes me sad! :P > > > > It makes me even sadder! > > I concur, but I still haven't come up with a better solution. Custom drawable!? (future work...?) Might be slower though. I donno. https://codereview.chromium.org/2777773002/diff/80001/chrome/browser/ui/andro... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.cc:205: 0, gfx::Size(kShareImageMaxWidth, kShareImageMaxHeight), On 2017/03/30 16:00:26, JJ wrote: > On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > > Our thumbnail size is the same size as the share? Just confirming this is on > > purpose. > > When running this morning I thought about the display size and realized instead > we could do 216dp * 4(xxxhdpi) = 864 pixels. Thought I could sneakily add that > in before you said something but alas. You caught me before I got it in! Should we scale this based on the density then? That should be accessible through a static in base or ui.
Alright! I've updated the fixes! Tell me how it looks to you! https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:188: public void getThumbnail() { On 2017/03/30 23:45:14, David Trainor-ping if over 24h wrote: > On 2017/03/30 17:09:00, JJ wrote: > > On 2017/03/30 16:00:25, JJ wrote: > > > On 2017/03/30 05:03:14, David Trainor-ping if over 24h wrote: > > > > I actually think this (and maybe shareImage() with some rework) could > > probably > > > > be done with Callbacks. > > > > > > > > In this particular case, it looks like you're doing: > > > > setOnThumbnailReceivedListener(listener) > > > > getThumbnail() > > > > > > > > Could you just make it getThumbnail(Callback<byte[]> callback)? Maybe > > > something > > > > like the following: > > > > > > > > getThumbnail(new Callback<byte[]>() { void onResult(byte[] thumbnail) > > > {...}});? > > > > > > > > Native can trigger the callback directly too. The method signature would > be > > > > better if you could return a Bitmap, so maybe that's preferable if you can > > > make > > > > that work. > > > > > > > > Finally... if you don't like this, just pass OnThumbnailReceivedListener > > into > > > > getThumbnail()... > > > > > > You make an interesting point on that native can call the callback directly. > > For > > > now I'll change it to just passing the onThumbnailReceivedListener into > > > getThumbnail. I'll keep looking into the other method and some examples I've > > > found later on to see if I can go that route cause it sounds interesting. > > > > Checked it out! It's not impossible, but there are some clear roadblocks. > Right > > now that means adding a param "RequestThumbnailForContextMenu" which means > we'd > > need to change share. This is however a good spot for improving as the code is > > extremely similar and we can aaaaaaaaaalmost change it but it needs some > > refactoring to do it. > > You could probably just do it all with finals. For this function I think the > below would be good: > > public void getThumbnail(final OnThumbnailReceivedListener listener) { > Callback<byte[]> callback = new Callback<byte[]>() { > @Override > public void onResult(byte[] result) { > listener.onThumbnailReceived(BitmapFactory.decodeByteArray(result, 0, > result.length); > } > } > } > It's not the Java side that's the problem. That part is actually easy. It's the C++ side. Right now it calls a RequestTumbnailForContextNode which only accepts specifics arguments. The problem is that I can't pass a callback into there as sadly this affects shared methods. So I'd have to change it for Share Image as well and for any other potential images as well. So while I think it is an awesome idea that should be explored in the future. I don't think it's one I can do at this moment, especially while keeping the cl short.
https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu_page.xml (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu_page.xml:30: android:contentDescription="@string/context_menu_header_image_default_desc_text" I think we shouldn't use a content description here either. I would use @null. We could consider using a content description if there is an alt text on the image to try and construct something that would be helpful to those visually impaired, but here we're just saying it's an image so I think we should just drop it. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:48: interface OnThumbnailReceivedListener { FWIW, you can just use Callback.java from base/ to make these single value callback a bit easier to define. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:132: mOnMenuClosed); from my comment in the other file, I think we should do: final TabularContextMenuUi menuUi = new TabularContextMenuUi(this); ... if (mCurrentContextMenuParams.isImage()) { getThumbnail(new Callback<Bitmap>() { @Override public void call(Bitmap bitmap) { menuUi.onImageThumbnailRetreived(bitmap); } }); } Then you don't need to pass in the helper. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:200: Bitmap bitmap = BitmapFactory.decodeByteArray(jpegImageData, 0, jpegImageData.length); Can you add another: // TODO(tedchoc): Decode in separate process before launch. Here we are receiving image data from the renderer, which is inherently untrusted, so we can't decode in the browser process safely. I think we can either use a separate java process or the utility process. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:52: TabularContextMenuUi(ContextMenuHelper contextMenuHelper) { I don't think this class should take the ContextMenuHelper. I think it should just have the method onImageThumbnailReceived and the context menu helper should kick of the image fetching and pass it in. I want to keep the UI class devoid of any direct native dependencies. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:243: boolean isImage = itemGroup.first == R.string.contextmenu_image_title; Hmm...looking at this I think we might want to pass the ContextMenuGroup identifier instead of the string resource id as that is probably what we should be keying this off of. Another TODO perhaps? https://codereview.chromium.org/2777773002/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:37: const int kHeaderImageMaxWidth = 864; // max size (216dp) * max density we can remove these now https://codereview.chromium.org/2777773002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:193: void ContextMenuHelper::RetrieveHeaderThumbnail( Can you add // TODO(tedchoc): Unify RetrieveHeaderThumbnail and ShareImage.
here are the updated comments! https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/tabular_context_menu_page.xml (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/tabular_context_menu_page.xml:30: android:contentDescription="@string/context_menu_header_image_default_desc_text" On 2017/03/31 16:19:32, Ted C wrote: > I think we shouldn't use a content description here either. I would use @null. > We could consider using a content description if there is an alt text on the > image to try and construct something that would be helpful to those visually > impaired, but here we're just saying it's an image so I think we should just > drop it. Done. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:48: interface OnThumbnailReceivedListener { On 2017/03/31 16:19:32, Ted C wrote: > FWIW, you can just use Callback.java from base/ to make these single value > callback a bit easier to define. Done. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:132: mOnMenuClosed); On 2017/03/31 16:19:33, Ted C wrote: > from my comment in the other file, I think we should do: > > final TabularContextMenuUi menuUi = new TabularContextMenuUi(this); > ... > if (mCurrentContextMenuParams.isImage()) { > getThumbnail(new Callback<Bitmap>() { > @Override > public void call(Bitmap bitmap) { > menuUi.onImageThumbnailRetreived(bitmap); > } > }); > } > > Then you don't need to pass in the helper. Done. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:200: Bitmap bitmap = BitmapFactory.decodeByteArray(jpegImageData, 0, jpegImageData.length); On 2017/03/31 16:19:33, Ted C wrote: > Can you add another: > > // TODO(tedchoc): Decode in separate process before launch. > > Here we are receiving image data from the renderer, which is inherently > untrusted, so we can't decode in the browser process safely. I think we can > either use a separate java process or the utility process. Done. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:52: TabularContextMenuUi(ContextMenuHelper contextMenuHelper) { On 2017/03/31 16:19:33, Ted C wrote: > I don't think this class should take the ContextMenuHelper. I think it should > just have the method onImageThumbnailReceived and the context menu helper should > kick of the image fetching and pass it in. I want to keep the UI class devoid > of any direct native dependencies. Done. https://codereview.chromium.org/2777773002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:243: boolean isImage = itemGroup.first == R.string.contextmenu_image_title; On 2017/03/31 16:19:33, Ted C wrote: > Hmm...looking at this I think we might want to pass the ContextMenuGroup > identifier instead of the string resource id as that is probably what we should > be keying this off of. > > Another TODO perhaps? Added but I'd give a word of caution it's more of the fact that we're trying to setup the image tab more than determining if it's an image. https://codereview.chromium.org/2777773002/diff/180001/chrome/browser/ui/andr... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2777773002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:37: const int kHeaderImageMaxWidth = 864; // max size (216dp) * max density On 2017/03/31 16:19:33, Ted C wrote: > we can remove these now Done. https://codereview.chromium.org/2777773002/diff/180001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:193: void ContextMenuHelper::RetrieveHeaderThumbnail( On 2017/03/31 16:19:33, Ted C wrote: > Can you add > > // TODO(tedchoc): Unify RetrieveHeaderThumbnail and ShareImage. Done.
https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:46: ContextMenuHelper() {} is this still needed? https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:197: // TODO(tedchoc): Decode in separate process before launch. nit, should go above previous line https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:77: public void onClick(View view) {} is this needed right now? https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:247: public void onImageThumbnailRetrieved(Bitmap bitmap) { add javadoc https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2794: <message name="IDS_CONTEXT_MENU_HEADER_IMAGE_DEFAULT_DESC_TEXT" desc="The image shown before a thumbnail loads within the context menu."> delete https://codereview.chromium.org/2777773002/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:37: const int kHeaderImageMaxWidth = 864; // max size (216dp) * max density remove these two lines https://codereview.chromium.org/2777773002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:193: void ContextMenuHelper::RetrieveHeaderThumbnail( missing todo here to combine
Yay c: https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:46: ContextMenuHelper() {} On 2017/03/31 18:48:32, Ted C wrote: > is this still needed? Sorry, deleted in another branch. https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:197: // TODO(tedchoc): Decode in separate process before launch. On 2017/03/31 18:48:31, Ted C wrote: > nit, should go above previous line Done. https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:77: public void onClick(View view) {} On 2017/03/31 18:48:32, Ted C wrote: > is this needed right now? Nope, sorry. https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:247: public void onImageThumbnailRetrieved(Bitmap bitmap) { On 2017/03/31 18:48:32, Ted C wrote: > add javadoc Done. https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2794: <message name="IDS_CONTEXT_MENU_HEADER_IMAGE_DEFAULT_DESC_TEXT" desc="The image shown before a thumbnail loads within the context menu."> On 2017/03/31 18:48:32, Ted C wrote: > delete Done. https://codereview.chromium.org/2777773002/diff/200001/chrome/browser/ui/andr... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2777773002/diff/200001/chrome/browser/ui/andr... chrome/browser/ui/android/context_menu_helper.cc:193: void ContextMenuHelper::RetrieveHeaderThumbnail( On 2017/03/31 18:48:32, Ted C wrote: > missing todo here to combine 100% certain I did this. Whoops.
The CQ bit was checked by jwanda@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jwanda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2777773002/#ps260001 (title: "Why be public when you can be private")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490998670195000,
"parent_rev": "7e189606bbbf9cf5f81200bca143e1ff3729faf2", "commit_rev":
"88172028d81d3cedbf5f315bbbd0f280dd252ce4"}
Message was sent while issue was closed.
Description was changed from ========== Show the image header for the Context Menu This CL should show an image above the context menu. It also adds a background as we wait for the image to load. Screenshots are below: https://drive.google.com/open?id=0B6D5A57VLDpeajkxZTBwVWZMcm8 https://drive.google.com/open?id=0B6D5A57VLDpeZWtIWlR1UlRyQ3c https://drive.google.com/open?id=0B6D5A57VLDpeY0dpU3lTT3NDb1k BUG=655359 ========== to ========== Show the image header for the Context Menu This CL should show an image above the context menu. It also adds a background as we wait for the image to load. Screenshots are below: https://drive.google.com/open?id=0B6D5A57VLDpeajkxZTBwVWZMcm8 https://drive.google.com/open?id=0B6D5A57VLDpeZWtIWlR1UlRyQ3c https://drive.google.com/open?id=0B6D5A57VLDpeY0dpU3lTT3NDb1k BUG=655359 Review-Url: https://codereview.chromium.org/2777773002 Cr-Commit-Position: refs/heads/master@{#461254} Committed: https://chromium.googlesource.com/chromium/src/+/88172028d81d3cedbf5f315bbbd0... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/88172028d81d3cedbf5f315bbbd0... |
