Issue 2751333006: Create the base Custom Context Menu Dialog.

JJ

jwanda@chromium.org changed reviewers: + tedchoc@chromium.org

3 years, 9 months ago (2017-03-17 20:58:19 UTC) #1

JJ

Please take a look at the CL! I'm still waiting on a few small things ...

3 years, 9 months ago (2017-03-17 20:58:20 UTC) #2

JJ

jwanda@chromium.org changed reviewers: + dtrainor@chromium.org

3 years, 9 months ago (2017-03-17 21:12:43 UTC) #3

JJ

Please take a look when you can! https://codereview.chromium.org/2751333006/diff/1/chrome/browser/android/chrome_feature_list.cc File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2751333006/diff/1/chrome/browser/android/chrome_feature_list.cc#newcode91 chrome/browser/android/chrome_feature_list.cc:91: NOTREACHED() << ...

3 years, 9 months ago (2017-03-17 21:12:44 UTC) #4

JJ

Description was changed from ========== Create the base Custom Context Menu Dialog. This CL creates ...

3 years, 9 months ago (2017-03-17 21:21:46 UTC) #5

Ted C

initial set of comments https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java#newcode135 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:135: public void onCreateContextMenu(ContextMenu menu, View ...

3 years, 9 months ago (2017-03-21 16:29:40 UTC) #6

initial set of comments

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:135:
public void onCreateContextMenu(ContextMenu menu, View v, ContextMenuInfo
menuInfo) {
So this path still relies on View.showContextMenu, which we probably shouldn't
call anymore for the new UI.  I think we should be splitting in showContextMenu
above.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:110:
* Transforms the icon id of the item into a tinted drawable of
light_normal_color.
In general, try to avoid writing your comments that are essentially describing
the code.  I think here the comment can simply be, return the drawable
associated with the context menu.

As written, it would require changing if we no longer tinted or tinted to a
different color, but those are details an user of the class "shouldn't" care
about.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:112:
* @return If the icon has an id of 0 it returns null, otherwise it gets the
proper drawable.
the id == 0 is an internal detail, this can also be, return the drawable for the
context menu or null if there is no drawable associated.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:31:
public class CustomContextMenuDialog implements ContextMenuUi,
AdapterView.OnItemClickListener {
If the other name is PlatformContextMenuUi, I think something like
TabularContextMenuUi or GroupedContextMenuUi is somewhat clearer about the
distinction.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:95:
listView.requestLayout();
setLayoutParams does a requestlayout for you

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:105:
* To save time measuring the height, this method gets the first item and
multiplies it by
I wouldn't call out the first item in this comment or in the @return.  I would
put a comment right about the place that you pull out the first view and say
that it will be used as an approximate for all other views.  If we find that we
want to use the shortest string (or longest) then we could do that.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:118:
view.measure(View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED),
I don't "think" this does what you expect.  By specifying UNSPECIFIED for both
width and height, you are allowing the view to be unbounded in size.  I think
you want to give a max width and use AT_MOST for the width dimension.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:120:
height = view.getMeasuredHeight();
I don't think you need a height variable.  I think can just do totalHeight +=
view.getMeasuredHeight() * listAdapter.getCount() here.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:149:
TabLayout tabLayout = (TabLayout) view.findViewById(R.id.tab_layout);
what happens if you set the visibility of the tablayout to GONE?  I'm wondering
if we could have a single layout that works for the multi group case as well as
the single group case.

David Trainor- moved to gerrit

Quick comments on the pngs: - We should have a bunch of these already (download, ...

3 years, 9 months ago (2017-03-21 16:30:03 UTC) #7

David Trainor- moved to gerrit

Also, we should shrink the border shadow nine patch to remove all redundant space.

3 years, 9 months ago (2017-03-21 16:32:56 UTC) #8

JJ

I've addressed many of the issues that are put in place. There are a few ...

3 years, 9 months ago (2017-03-22 23:35:33 UTC) #9

I've addressed many of the issues that are put in place. There are a few that I
have not fixed due to some hesitation that it's the best way to go. Please take
a look. The pngs and the two comments I missed on a first pass will come through
in a bit. Thanks!

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/res/dra...
File
chrome/android/java/res/drawable/custom_context_menu_checkerboard_image_background.xml
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/custom_context_menu_checkerboard_image_background.xml:2:
<bitmap xmlns:android="http://schemas.android.com/apk/res/android"
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> Do we want the copyright here too?

Oh I'm surprised it didn't yell at me for this.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/custom_context_menu_checkerboard_image_background.xml:3:
android:src="@drawable/context_menu_checkerboard"
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> Just curious, do we use a checkerboard for anything else on Android (aside
from
> rendering)?
> 
> We should be able to do this without an actual bitmap.  Can we just define a
> custom drawable that does the checkerboard?

Oooooo! This is interesting. So there is a way to do it but it's complicated. We
create a layerlist for the 4 squares. Convert this to a bitmap, then to a bitmap
drawable then have that bitmap drawable repeat constantly. So it's more of a
question of a tradeoff question more than anything.

However! I've done it and I want to get your opinion on it. Take a look at
CustomContextMenuDialog.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/custom_context_menu_row.xml (right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/custom_context_menu_row.xml:6:
android:orientation="horizontal" android:layout_width="match_parent"
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> new line for width?

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/multi_type_context_menu.xml (right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/multi_type_context_menu.xml:5: <LinearLayout
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> I haven't looked at the CustomContextMenuViewPager yet.  Does this View have
to
> be a LinearLayout (or here at all)?

Actually no. The LinearLayout was leftover I'm pretty sure from old code. It
works just as well without it.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:135:
public void onCreateContextMenu(ContextMenu menu, View v, ContextMenuInfo
menuInfo) {
On 2017/03/21 16:29:39, Ted C wrote:
> So this path still relies on View.showContextMenu, which we probably shouldn't
> call anymore for the new UI.  I think we should be splitting in
showContextMenu
> above.

Alright, refactored. I did try this before in a previous life where we got rid
of the ContextMenu parameter but it turns out that is actually heavily used and
several tests fail if you don't include that! In order to not duplicate too much
code I've added an extra global for callback. Tell me if this is along your line
of thinking.

In order to get rid of that pesky "menu" we'll have to rewrite dozens of tests.
I know this needs to be done but in 500+ line cl it's something I'd rather have
done in a seperate cl.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:79:
* @param stringId The string that describes the action of the item.
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> iconId in javadoc

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:110:
* Transforms the icon id of the item into a tinted drawable of
light_normal_color.
On 2017/03/21 16:29:39, Ted C wrote:
> In general, try to avoid writing your comments that are essentially describing
> the code.  I think here the comment can simply be, return the drawable
> associated with the context menu.
> 
> As written, it would require changing if we no longer tinted or tinted to a
> different color, but those are details an user of the class "shouldn't" care
> about.

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:112:
* @return If the icon has an id of 0 it returns null, otherwise it gets the
proper drawable.
On 2017/03/21 16:29:39, Ted C wrote:
> the id == 0 is an internal detail, this can also be, return the drawable for
the
> context menu or null if there is no drawable associated.

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:118:
return TintedDrawable.constructTintedDrawable(
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> Do we access all icons like this?  Should we just tint the actual bitmap? 
IIRC
> this is pretty slow and takes more memory.

Many icons handed to me are not the correct shade. If needed we can make a quick
switch case for all the ones that need tint and if  I can message Bruno and ask
to send me tinted icons instead?

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> 2017

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:31:
public class CustomContextMenuDialog implements ContextMenuUi,
AdapterView.OnItemClickListener {
On 2017/03/21 16:29:39, Ted C wrote:
> If the other name is PlatformContextMenuUi, I think something like
> TabularContextMenuUi or GroupedContextMenuUi is somewhat clearer about the
> distinction.

I've noted this and will change it shortly. I have two cls based off this and
renaming leads to all my changes as lost. I'll change it in a bit.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:32:
private AlertDialog mDialog;
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> final?

Can't be done since we don't call this in the constructor. Adding them to the
constructor also seems redundant too.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:33:
private Callback<Integer> mCallback;
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> final?

Can't be done since we don't call this in the constructor. Adding them to the
constructor also seems redundant too.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:53:
private AlertDialog createDialog(Activity activity, ContextMenuParams params,
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> Should we have both createDialog and createFinalView?  Don't feel strongly
> either way :).

Yeah the view is used mostly for tests (that aren't shown but they're done and
coming up shortly!). Getting a view from a dialog is a painful and confusing
process.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:61:
* This creates the view hosted within the dialog. Mainly used for testing.
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> Maybe use "//" or turn it into a more real javadoc.
> 
> Not really used for testing, but pulled out to a separate method to allow
tests
> to override it?

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:95:
listView.requestLayout();
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> I think setLayoutParams does this?

Thanks!

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:98:
baseLayout.measure(View.MeasureSpec.makeMeasureSpec(0,
View.MeasureSpec.UNSPECIFIED),
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> Could you explain why we need to measure baseLayout here?  Sorry I might just
be
> missing something.

Oh we don't. Sorry, old experimentation got the better of me.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:105:
* To save time measuring the height, this method gets the first item and
multiplies it by
On 2017/03/21 16:29:39, Ted C wrote:
> I wouldn't call out the first item in this comment or in the @return.  I would
> put a comment right about the place that you pull out the first view and say
> that it will be used as an approximate for all other views.  If we find that
we
> want to use the shortest string (or longest) then we could do that.

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:118:
view.measure(View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED),
On 2017/03/21 16:29:39, Ted C wrote:
> I don't "think" this does what you expect.  By specifying UNSPECIFIED for both
> width and height, you are allowing the view to be unbounded in size.  I think
> you want to give a max width and use AT_MOST for the width dimension.

bugh. Thank you. Measurements are still a little foreign to me.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:120:
height = view.getMeasuredHeight();
On 2017/03/21 16:29:40, Ted C wrote:
> I don't think you need a height variable.  I think can just do totalHeight +=
> view.getMeasuredHeight() * listAdapter.getCount() here.

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:137:
View view = inflater.inflate(R.layout.multi_type_context_menu, null);
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> LayoutInflater.from(activity).inflate(...) like what you did for
> creaetSingleContextView?

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:149:
TabLayout tabLayout = (TabLayout) view.findViewById(R.id.tab_layout);
On 2017/03/21 16:29:39, Ted C wrote:
> what happens if you set the visibility of the tablayout to GONE?  I'm
wondering
> if we could have a single layout that works for the multi group case as well
as
> the single group case.

O: It does! Wow I can't believe I didn't try that. Code is a little changed to
handle this.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuListAdapter.java:24:
private List<ContextMenuItem> mMenuItems;
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> eh final for these as well?

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuPagerAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuPagerAdapter.java:21:
private List<Pair<String, ViewGroup>> mViewList;
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> I'd say final here as well and everything else that you don't expect/want to
> change for the lifecycle of the class (or that you want to guarantee gets set
in
> constructors).  But I'm ok either way.

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuPagerAdapter.java:32:
private static int adjustIndexForDirectionality(int index, int count) {
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> On 2017/03/17 20:58:20, JJ wrote:
> > Possibly thinking in the future of making a Reversible Pager Adapter to get
> the
> > proper position as this is copied off of some other code in the codebase. If
> so,
> > do you think this needs a "todo"?
> 
> Adding a TODO isn't a bad idea.  Does this logic live in other PagerAdapters
in
> Chrome?
> 
> Also, is this fixed in any version of Android at all?  Or is it still an
> existing bug?

Likely an existing bug seeing the last edit was in December.
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

I'll message the person and see if they have made any progress.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuPagerAdapter.java:33:
if (TextUtilsCompat.getLayoutDirectionFromLocale(Locale.getDefault())
On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> Should we cache whether or not the direction is RTL?

Removed static for this class to do this. (I mean I also technically didn't need
to but it didn't really matter).

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuViewPager.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuViewPager.java:27:
/** Used to show the full ViewPager dialog. Without this the dialog would have
no height or
On 2017/03/21 16:30:03, David Trainor-ping if over 24h wrote:
> Fix javadoc?

Done.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:2777: An icon that
represents the item action and title.
On 2017/03/21 16:30:03, David Trainor-ping if over 24h wrote:
> Should we be setting these to text based on the actual icon we're setting? 
This
> doesn't seem very useful for accessibility?

It's not :c I tried it out last week and didn't like it. We could do some
templating here? Like maybe
"$1 Icon" or "An icon that represents $1". Which would be equal to "Open in New
Tab Icon" and write in the description some possibilities.
Thoughts?

David Trainor- moved to gerrit

The nine patches still look too big. You don't really need any redundant that aren't ...

3 years, 9 months ago (2017-03-23 04:58:48 UTC) #10

The nine patches still look too big.  You don't really need any redundant that
aren't part of the border curve.  Feel free to stop by tomorrow if you want and
I can show you :).

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:118:
return TintedDrawable.constructTintedDrawable(
On 2017/03/22 23:35:32, JJ wrote:
> On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> > Do we access all icons like this?  Should we just tint the actual bitmap? 
> IIRC
> > this is pretty slow and takes more memory.
> 
> Many icons handed to me are not the correct shade. If needed we can make a
quick
> switch case for all the ones that need tint and if  I can message Bruno and
ask
> to send me tinted icons instead?

Yeah ask him to send you the tinted ones IMO.  I don't know if the policy on
these kind of assets has changed since I used to add them though.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:32:
private AlertDialog mDialog;
On 2017/03/22 23:35:32, JJ wrote:
> On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> > final?
> 
> Can't be done since we don't call this in the constructor. Adding them to the
> constructor also seems redundant too.

Makes sense I misread "displayMenu" for the constructor since it was the first
method.  My bad!

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:53:
private AlertDialog createDialog(Activity activity, ContextMenuParams params,
On 2017/03/22 23:35:32, JJ wrote:
> On 2017/03/21 16:30:02, David Trainor-ping if over 24h wrote:
> > Should we have both createDialog and createFinalView?  Don't feel strongly
> > either way :).
> 
> Yeah the view is used mostly for tests (that aren't shown but they're done and
> coming up shortly!). Getting a view from a dialog is a painful and confusing
> process.

I guess meant should we just roll createDialog into displayMenu and call
createFinalView from there.  It looks like you modified the methods a bit though
so nbd!

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:118:
view.measure(View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED),
On 2017/03/22 23:35:32, JJ wrote:
> On 2017/03/21 16:29:39, Ted C wrote:
> > I don't "think" this does what you expect.  By specifying UNSPECIFIED for
both
> > width and height, you are allowing the view to be unbounded in size.  I
think
> > you want to give a max width and use AT_MOST for the width dimension.
> 
> bugh. Thank you. Measurements are still a little foreign to me.

Does this still need to change?

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:2777: An icon that
represents the item action and title.
On 2017/03/22 23:35:33, JJ wrote:
> On 2017/03/21 16:30:03, David Trainor-ping if over 24h wrote:
> > Should we be setting these to text based on the actual icon we're setting? 
> This
> > doesn't seem very useful for accessibility?
> 
> It's not :c I tried it out last week and didn't like it. We could do some
> templating here? Like maybe
> "$1 Icon" or "An icon that represents $1". Which would be equal to "Open in
New
> Tab Icon" and write in the description some possibilities.
> Thoughts?

I'd be fine with "$1 icon"

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:98:
private void setBackgroundForImageView(ImageView imageView, Resources resources)
{
Can we do this in XML?

some new drawable:
<bitmap android:src="@drawable/checkerboard_background"
android:tileMode="repeat" />

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuViewPager.java
(right):

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuViewPager.java:44:
if (child.getId() == R.id.tab_layout && child.getVisibility() == VISIBLE) {
!= GONE for correctness?  Invisible children still impact layout IIRC.

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuTest.java
(right):

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuTest.java:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
Yay tests!

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuTest.java:65:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
Maybe:

View view = ThreadUtils.runOnUiThreadBlocking(new Callable<View>() {
  return dialog.createPagerView(getActivity(), new MockMenuParams(url),
itemGroups);
});

JJ

Everything is addressed mins icons and tinting. Waiting on Bruno for that. Also based of ...

3 years, 9 months ago (2017-03-23 18:57:47 UTC) #11

Everything is addressed mins icons and tinting. Waiting on Bruno for that. Also
based of a previous bug I'm going to add another test for an empty dialog so
look out for that. (much easier to do here than on an android based one). 

I also asked a question on the ListAdapter as I'm torn between to options.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:118:
view.measure(View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED),
On 2017/03/23 04:58:47, David Trainor-ping if over 24h wrote:
> On 2017/03/22 23:35:32, JJ wrote:
> > On 2017/03/21 16:29:39, Ted C wrote:
> > > I don't "think" this does what you expect.  By specifying UNSPECIFIED for
> both
> > > width and height, you are allowing the view to be unbounded in size.  I
> think
> > > you want to give a max width and use AT_MOST for the width dimension.
> > 
> > bugh. Thank you. Measurements are still a little foreign to me.
> 
> Does this still need to change?

aha! Sorry I thought I redid this edit before sending. My bad. So actually we
can't as dialogs are really weird and using AT_MOST with the width makes the
height as large as possible. Using AT_MOST on the height makes it literally 0.
Yaaaaaaaay.

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:98:
private void setBackgroundForImageView(ImageView imageView, Resources resources)
{
On 2017/03/23 04:58:47, David Trainor-ping if over 24h wrote:
> Can we do this in XML?
> 
> some new drawable:
> <bitmap android:src="@drawable/checkerboard_background"
> android:tileMode="repeat" />

Nope. It requires a raw png.

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuViewPager.java
(right):

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuViewPager.java:44:
if (child.getId() == R.id.tab_layout && child.getVisibility() == VISIBLE) {
On 2017/03/23 04:58:47, David Trainor-ping if over 24h wrote:
> != GONE for correctness?  Invisible children still impact layout IIRC.

You are correct. Whoops.

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuTest.java
(right):

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuTest.java:65:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
On 2017/03/23 04:58:47, David Trainor-ping if over 24h wrote:
> Maybe:
> 
> View view = ThreadUtils.runOnUiThreadBlocking(new Callable<View>() {
>   return dialog.createPagerView(getActivity(), new MockMenuParams(url),
> itemGroups);
> });

I sometimes forget callable is a thing. Thank you!

https://codereview.chromium.org/2751333006/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuListAdapter.java:79:
viewHolder.mIcon.setContentDescription(iconPair.second);
I had such a tough time with this line. I'm still on the fence. I was wondering
what you all think. Should we set a description even if the icon doesn't exist?
If we don't should we remove that line from the .xml file and put in an ignore
for lint? I'd love to hear thoughts.

Ted C

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java (right): https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java#newcode118 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:118: view.measure(View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED), On 2017/03/23 18:57:46, JJ wrote: > On ...

3 years, 9 months ago (2017-03-24 04:32:54 UTC) #12

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:118:
view.measure(View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED),
On 2017/03/23 18:57:46, JJ wrote:
> On 2017/03/23 04:58:47, David Trainor-ping if over 24h wrote:
> > On 2017/03/22 23:35:32, JJ wrote:
> > > On 2017/03/21 16:29:39, Ted C wrote:
> > > > I don't "think" this does what you expect.  By specifying UNSPECIFIED
for
> > both
> > > > width and height, you are allowing the view to be unbounded in size.  I
> > think
> > > > you want to give a max width and use AT_MOST for the width dimension.
> > > 
> > > bugh. Thank you. Measurements are still a little foreign to me.
> > 
> > Does this still need to change?
> 
> aha! Sorry I thought I redid this edit before sending. My bad. So actually we
> can't as dialogs are really weird and using AT_MOST with the width makes the
> height as large as possible. Using AT_MOST on the height makes it literally 0.
> Yaaaaaaaay.

Sorry, but I don't understand this comment.

You are measuring the row item, which is a linear layout.  By using UNSPECIFIED,
I think you're never going to have a case where you force the text to line wrap
if it would want to naturally.  I think unspecified for height is correct, but
for width we should either be using an exact width or AT_MOST.  But the problem
is we need to get this width from "somewhere".

Here are a couple dialogs that do custom measuring:

For autofill (it wants to measure max width because it wants to be as wide as
the longest item):
https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop...

For the app menu (explicit height calculation, which what I think we want to do
in this case):
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
File chrome/android/java/res/layout/custom_context_menu.xml (right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/custom_context_menu.xml:1: <?xml version="1.0"
encoding="utf-8"?>
maybe context_menu_page.xml is a better name for this file (or
tabular_context_menu_page)

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
File chrome/android/java/res/layout/custom_context_menu_row.xml (right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/custom_context_menu_row.xml:1: <?xml
version="1.0" encoding="utf-8"?>
tabular_context_menu_row

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/custom_context_menu_row.xml:17:
android:contentDescription="@string/context_menu_row_icon_description" />
I wouldn't set a content description here (applies to later in the code), or
more explicitly use @null.  In accessibility, the text is all the user needs to
know to make a decision in the menu.  This would just duplicate the text and
isn't any more actionable.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
File chrome/android/java/res/layout/multi_type_context_menu.xml (right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/multi_type_context_menu.xml:1: <?xml
version="1.0" encoding="utf-8"?>
tabular_context_menu

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:91:
List<Pair<Integer, List<ContextMenuItem>>> items =
I would pull this out and stash it in a local var to share between the two
different UIs.  Then if it is empty, we can early return.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:102:
RecordHistogram.recordBooleanHistogram(
we still want to record this histogram and register the close listener.  I think
the old UI relied on ChromeActivity#onCloseContextMenu to call into
WindowAndroid, and we'll need to make sure your code does this for the new path.
 (it might already but preemptively leaving this).

I would just remove the return from your above if block, put an else

} else {
    view.setOnCreateContextMenuListener(...);
    if (!view.showContextMenu()) return;
}

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:30:
* Used to create a the base adapter for the list view using
custom_context_menu_row.xml
I wouldn't mention the xml file here.  Just something like:

"Adapter for tabular context menu UI"

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:31:
* @param menuItems  The list of items to display in the view.
one too many spaces after menuItems

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:35:
this.mMenuItems = menuItems;
this. isn't necessary as the names are different

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:78:
}
if there is no drawable, should we make the view either as INVISIBLE or GONE? 
INVISIBLE will keep the alignment the same as the other items (then we'd need to
make sure to make it VISIBLE in this else to counteract that).

Even if we don't set the visibility, we need to set the icon to null to clear
out the existing one if this view is reused.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:79:
viewHolder.mIcon.setContentDescription(iconPair.second);
I think we should only set this in the if above.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:63:
AlertDialog.Builder builder = new AlertDialog.Builder(activity);
Do we need an alertdialog in this case?  Would a plain ol' dialog work?  We
aren't using the alert dialog button capabilities, so this seems overkill.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:89:
layoutParams.height = measureApproximateListViewHeight(listView, listAdapter);
what happens if this height is larger than the screen (i.e. in landscape on a
smaller phone)?

Also, don't you want to measure the tallest list across all tabs and set the
height consistently for them?

I was thinking that we wanted to maintain a constant height for the title area
as well as the list heights between pages.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:111:
private void setBackgroundForImageView(ImageView imageView, Resources resources)
{
this seems to be unused.  maybe something we should do in a followup?

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:173:
tabLayout.measure(View.MeasureSpec.makeMeasureSpec(0,
View.MeasureSpec.UNSPECIFIED),
This still seems odd to me.  If we were manually setting the height of the popup
based on the height of the tablayout, longest page within the list, then I could
see this, but we need to figure out why this is needed as it seems like
something is incorrectly configured if this is required.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:35:
if (getChildCount() > 0) {
I don't think you need this check as the for loop just won't do anything if
child count == 0

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:52:
heightMeasureSpec = MeasureSpec.makeMeasureSpec(maxHeight, MeasureSpec.EXACTLY);
what happens if this is larger than the screen dimen again?

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:53:
super.onMeasure(widthMeasureSpec, heightMeasureSpec);
I wonder if we when we are creating the context menu pages in the previous file,
I think we should just measure the header text + the list view height.  Then max
those across the pages, add it to the tab height as you measure before and then
explicitly set the height of the popup (bounded on screen dimens).  Then I
"don't" think you should need this.

David Trainor- moved to gerrit

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java (right): https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java#newcode98 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:98: private void setBackgroundForImageView(ImageView imageView, Resources resources) { On 2017/03/23 ...

3 years, 9 months ago (2017-03-24 17:24:20 UTC) #13

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:98:
private void setBackgroundForImageView(ImageView imageView, Resources resources)
{
On 2017/03/23 18:57:46, JJ wrote:
> On 2017/03/23 04:58:47, David Trainor-ping if over 24h wrote:
> > Can we do this in XML?
> > 
> > some new drawable:
> > <bitmap android:src="@drawable/checkerboard_background"
> > android:tileMode="repeat" />
> 
> Nope. It requires a raw png.

Sadness :(.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:36:
this.mActivity = activity;
Can this just be a Context?

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:78:
}
On 2017/03/24 04:32:53, Ted C wrote:
> if there is no drawable, should we make the view either as INVISIBLE or GONE? 
> INVISIBLE will keep the alignment the same as the other items (then we'd need
to
> make sure to make it VISIBLE in this else to counteract that).
> 
> Even if we don't set the visibility, we need to set the icon to null to clear
> out the existing one if this view is reused.

Ah sorry I misread your comment!  Says exactly what I would have :).

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuPagerAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuPagerAdapter.java:34:
// Used thanks due to a bug in displaying RTL.
// Addresses bug RTL display bug?

JJ

The CQ bit was checked by jwanda@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-24 17:28:48 UTC) #14

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/100001

3 years, 9 months ago (2017-03-24 17:29:42 UTC) #15

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-24 17:33:39 UTC) #16

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/177084) ios-device-xcode-clang on ...

3 years, 9 months ago (2017-03-24 17:33:40 UTC) #17

JJ

Hey y'all I hope I've answered all thoughts on how to handle this change. I'm ...

3 years, 9 months ago (2017-03-25 01:40:26 UTC) #18

Hey y'all I hope I've answered all thoughts on how to handle this change. I'm a
little scatterbrained atm so there's a good chance that I missed something. I
apologize for any mistakes that were made.

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java
(right):

https://codereview.chromium.org/2751333006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/CustomContextMenuDialog.java:118:
view.measure(View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED),
On 2017/03/24 04:32:53, Ted C wrote:
> On 2017/03/23 18:57:46, JJ wrote:
> > On 2017/03/23 04:58:47, David Trainor-ping if over 24h wrote:
> > > On 2017/03/22 23:35:32, JJ wrote:
> > > > On 2017/03/21 16:29:39, Ted C wrote:
> > > > > I don't "think" this does what you expect.  By specifying UNSPECIFIED
> for
> > > both
> > > > > width and height, you are allowing the view to be unbounded in size. 
I
> > > think
> > > > > you want to give a max width and use AT_MOST for the width dimension.
> > > > 
> > > > bugh. Thank you. Measurements are still a little foreign to me.
> > > 
> > > Does this still need to change?
> > 
> > aha! Sorry I thought I redid this edit before sending. My bad. So actually
we
> > can't as dialogs are really weird and using AT_MOST with the width makes the
> > height as large as possible. Using AT_MOST on the height makes it literally
0.
> > Yaaaaaaaay.
> 
> Sorry, but I don't understand this comment.
> 
> You are measuring the row item, which is a linear layout.  By using
UNSPECIFIED,
> I think you're never going to have a case where you force the text to line
wrap
> if it would want to naturally.  I think unspecified for height is correct, but
> for width we should either be using an exact width or AT_MOST.  But the
problem
> is we need to get this width from "somewhere".
> 
> Here are a couple dialogs that do custom measuring:
> 
> For autofill (it wants to measure max width because it wants to be as wide as
> the longest item):
>
https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/Drop...
> 
> For the app menu (explicit height calculation, which what I think we want to
do
> in this case):
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

I think I've repeated this in a few other lines but it this does line wrap as
I've tested. I'm not sure how we'd get the width for somewhere since it can only
be measured on Display (it seems Android gives us the width and we just have to
obey).

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
File chrome/android/java/res/layout/custom_context_menu.xml (right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/custom_context_menu.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2017/03/24 04:32:53, Ted C wrote:
> maybe context_menu_page.xml is a better name for this file (or
> tabular_context_menu_page)

Done.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
File chrome/android/java/res/layout/custom_context_menu_row.xml (right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/custom_context_menu_row.xml:1: <?xml
version="1.0" encoding="utf-8"?>
On 2017/03/24 04:32:53, Ted C wrote:
> tabular_context_menu_row

Done.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/re...
chrome/android/java/res/layout/custom_context_menu_row.xml:17:
android:contentDescription="@string/context_menu_row_icon_description" />
On 2017/03/24 04:32:53, Ted C wrote:
> I wouldn't set a content description here (applies to later in the code), or
> more explicitly use @null.  In accessibility, the text is all the user needs
to
> know to make a decision in the menu.  This would just duplicate the text and
> isn't any more actionable.

Done.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:91:
List<Pair<Integer, List<ContextMenuItem>>> items =
On 2017/03/24 04:32:53, Ted C wrote:
> I would pull this out and stash it in a local var to share between the two
> different UIs.  Then if it is empty, we can early return.

So I really really wanted to do this. When separating the callback I thought we
could do this but it turns out doing it like this will cause tests to fail since
the ContextMenu is null. I'm try8ing

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:30:
* Used to create a the base adapter for the list view using
custom_context_menu_row.xml
On 2017/03/24 04:32:53, Ted C wrote:
> I wouldn't mention the xml file here.  Just something like:
> 
> "Adapter for tabular context menu UI"

Done.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:31:
* @param menuItems  The list of items to display in the view.
On 2017/03/24 04:32:53, Ted C wrote:
> one too many spaces after menuItems

Done.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:35:
this.mMenuItems = menuItems;
On 2017/03/24 04:32:53, Ted C wrote:
> this. isn't necessary as the names are different

Done.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:79:
viewHolder.mIcon.setContentDescription(iconPair.second);
On 2017/03/24 04:32:53, Ted C wrote:
> I think we should only set this in the if above.

Thanks, was having trouble with this line.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:63:
AlertDialog.Builder builder = new AlertDialog.Builder(activity);
On 2017/03/24 04:32:53, Ted C wrote:
> Do we need an alertdialog in this case?  Would a plain ol' dialog work?  We
> aren't using the alert dialog button capabilities, so this seems overkill.

Well what do you know, we can? There was definitely a reason we couldn't do this
before I submitted it but it must have rid itself.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:89:
layoutParams.height = measureApproximateListViewHeight(listView, listAdapter);
On 2017/03/24 04:32:53, Ted C wrote:
> what happens if this height is larger than the screen (i.e. in landscape on a
> smaller phone)?
> 
> Also, don't you want to measure the tallest list across all tabs and set the
> height consistently for them?
> 
> I was thinking that we wanted to maintain a constant height for the title area
> as well as the list heights between pages.

There's a max height that dialogs are willing to go. In no case (that I've
tested by setting the height to 6000 and other various heights) will it go over.
Feel free to stop at my desk for a slick demo of Android being awesome.

For the point on the tallest list. So I did do something about this and it
solves a measure but then makes the code less readable. So while I've done it
I'd feel a little remiss to not say that readability is impacted a little.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:111:
private void setBackgroundForImageView(ImageView imageView, Resources resources)
{
On 2017/03/24 04:32:53, Ted C wrote:
> this seems to be unused.  maybe something we should do in a followup?

Moved to the next CL.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:173:
tabLayout.measure(View.MeasureSpec.makeMeasureSpec(0,
View.MeasureSpec.UNSPECIFIED),
On 2017/03/24 04:32:53, Ted C wrote:
> This still seems odd to me.  If we were manually setting the height of the
popup
> based on the height of the tablayout, longest page within the list, then I
could
> see this, but we need to figure out why this is needed as it seems like
> something is incorrectly configured if this is required.

It's not needed. It's a relic. My bad.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:35:
if (getChildCount() > 0) {
On 2017/03/24 04:32:53, Ted C wrote:
> I don't think you need this check as the for loop just won't do anything if
> child count == 0

Whoops you're right

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:52:
heightMeasureSpec = MeasureSpec.makeMeasureSpec(maxHeight, MeasureSpec.EXACTLY);
On 2017/03/24 04:32:54, Ted C wrote:
> what happens if this is larger than the screen dimen again?

Again nothing. The Dialog does not go bigger than the screen.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:53:
super.onMeasure(widthMeasureSpec, heightMeasureSpec);
On 2017/03/24 04:32:54, Ted C wrote:
> I wonder if we when we are creating the context menu pages in the previous
file,
> I think we should just measure the header text + the list view height.  Then
max
> those across the pages, add it to the tab height as you measure before and
then
> explicitly set the height of the popup (bounded on screen dimens).  Then I
> "don't" think you should need this.

That's a good thought. I tried to something with this but then I realized how
this won't work when we get an image that has to remeasure the height. As a
result I don't think we should go through this path? Tell me if you feel free
differently.

JJ

I've tried to eradicate most references of the image and move it over to a ...

3 years, 9 months ago (2017-03-26 22:09:49 UTC) #19

JJ

The CQ bit was checked by jwanda@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-27 18:18:38 UTC) #20

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/160001

3 years, 9 months ago (2017-03-27 18:19:23 UTC) #21

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-27 18:24:01 UTC) #22

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/395503) ios-simulator-xcode-clang on ...

3 years, 9 months ago (2017-03-27 18:24:02 UTC) #23

Ted C

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java (right): https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java#newcode53 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:53: super.onMeasure(widthMeasureSpec, heightMeasureSpec); On 2017/03/25 01:40:25, JJ wrote: > On ...

3 years, 9 months ago (2017-03-27 20:53:34 UTC) #24

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:53:
super.onMeasure(widthMeasureSpec, heightMeasureSpec);
On 2017/03/25 01:40:25, JJ wrote:
> On 2017/03/24 04:32:54, Ted C wrote:
> > I wonder if we when we are creating the context menu pages in the previous
> file,
> > I think we should just measure the header text + the list view height.  Then
> max
> > those across the pages, add it to the tab height as you measure before and
> then
> > explicitly set the height of the popup (bounded on screen dimens).  Then I
> > "don't" think you should need this.
> 
> That's a good thought. I tried to something with this but then I realized how
> this won't work when we get an image that has to remeasure the height. As a
> result I don't think we should go through this path? Tell me if you feel free
> differently.

In the design spec, it mentions the height never changes for the header for an
image, so I wonder if this is still what we should end up trying to do.

If we ever end up having more than two pages, we might also want to look at
ViewPager#setOffscreenPageLimit to more than 1 to ensure the view traversal is
valid.

If we didn't override onMeasure here or for the listview in the previous file,
what happens if you just set the layout params to the ViewPager to be height
AT_MOST and screen size - image popup padding?  Does it try to fill the whole
space?

Now that I'm thinking about this, I wonder if this is redundant of what
ViewPager#onMeasure does in the first place.

If we just tell it to fill up the most amount of room it can with AT_MOST, does
it iterate across the children and measure them correctly?  It might end up
being more expensive for the listview layouts, but it could be more straight
forward.

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java
(right):

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:127:
drawable = ApiCompatibilityUtils.getDrawable(context.getResources(), iconId);
any reason we don't tint these drawables?  Is it an optimization?  I'd argue to
just use a single code path for clarity until we need to if it is just a speed
thing

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java
(right):

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:89:
ViewGroup createSingleContextView(Activity activity, ContextMenuParams params,
I think createContextMenuGroupListUi/createContextMenuPageUi is clearer to me
that it isn't a UI used for a context menu with a single group.

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:155:
int maxCount = 0;
The maxCount is a bit misleading as it can change with each subsequent group.

I think instead of keeping mMenuItemHeight and passing in maxCount, I think we
should calculate the item height here once and pass it into the
createSingleContextView method.

So, we'll need to iterate over the itemgroups twice, but that isn't a long list.

Also, just realized but we should use the for (int i = 0; i <... approach since
this is a List and not a Set).

JJ

3/4 comments don't need a change associated with it. The last one I'll do in ...

3 years, 9 months ago (2017-03-28 00:09:27 UTC) #25

3/4 comments don't need a change associated with it. The last one I'll do in the
morning. Please take a look when you can.

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java
(right):

https://codereview.chromium.org/2751333006/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuViewPager.java:53:
super.onMeasure(widthMeasureSpec, heightMeasureSpec);
On 2017/03/27 20:53:33, Ted C wrote:
> On 2017/03/25 01:40:25, JJ wrote:
> > On 2017/03/24 04:32:54, Ted C wrote:
> > > I wonder if we when we are creating the context menu pages in the previous
> > file,
> > > I think we should just measure the header text + the list view height. 
Then
> > max
> > > those across the pages, add it to the tab height as you measure before and
> > then
> > > explicitly set the height of the popup (bounded on screen dimens).  Then I
> > > "don't" think you should need this.
> > 
> > That's a good thought. I tried to something with this but then I realized
how
> > this won't work when we get an image that has to remeasure the height. As a
> > result I don't think we should go through this path? Tell me if you feel
free
> > differently.
> 
> In the design spec, it mentions the height never changes for the header for an
> image, so I wonder if this is still what we should end up trying to do.
> 
> If we ever end up having more than two pages, we might also want to look at
> ViewPager#setOffscreenPageLimit to more than 1 to ensure the view traversal is
> valid.
Awesome, I'll keep that in mind. I'm not sure if this is supposed to be an
actionable change?

> 
> If we didn't override onMeasure here or for the listview in the previous file,
> what happens if you just set the layout params to the ViewPager to be height
> AT_MOST and screen size - image popup padding?  Does it try to fill the whole
> space?
Yeah it still tries to fill up all the space regadless of the what we put in our
"AT_MOST" value.

> 
> Now that I'm thinking about this, I wonder if this is redundant of what
> ViewPager#onMeasure does in the first place.
Deleting on Measure entirely means that it will try to fill up the entire
screen.

 
> If we just tell it to fill up the most amount of room it can with AT_MOST,
does
> it iterate across the children and measure them correctly?  It might end up
> being more expensive for the listview layouts, but it could be more straight
> forward.


So basically long short onMeasure is required. I'd highly recommend trying out
some other possibliities just in case I missed one.

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java
(right):

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:127:
drawable = ApiCompatibilityUtils.getDrawable(context.getResources(), iconId);
On 2017/03/27 20:53:33, Ted C wrote:
> any reason we don't tint these drawables?  Is it an optimization?  I'd argue
to
> just use a single code path for clarity until we need to if it is just a speed
> thing

It's a speed thing as discussed in previous paths with David. Apparently it's
pretty harsh to use tinted versions if we don't need to.

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java
(right):

https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:89:
ViewGroup createSingleContextView(Activity activity, ContextMenuParams params,
On 2017/03/27 20:53:33, Ted C wrote:
> I think createContextMenuGroupListUi/createContextMenuPageUi is clearer to me
> that it isn't a UI used for a context menu with a single group.

Done.

David Trainor- moved to gerrit

lgtm % tedchoc https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2751333006/diff/170001/chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java#newcode116 chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:116: // The Platform Context Menu requires ...

3 years, 9 months ago (2017-03-28 03:25:59 UTC) #26

JJ

When you get the chance. David also defered to you about the tinted icons. I'm ...

3 years, 8 months ago (2017-03-28 16:57:01 UTC) #27

JJ

The CQ bit was checked by jwanda@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-28 16:57:16 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/190001

3 years, 8 months ago (2017-03-28 16:57:38 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-03-28 17:01:23 UTC) #30

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/178962) ios-device-xcode-clang on ...

3 years, 8 months ago (2017-03-28 17:01:24 UTC) #31

Ted C

Also, we shouldn't be adding the new incognito icon. We should use what we already ...

3 years, 8 months ago (2017-03-28 17:31:44 UTC) #34

JJ

Fixed! Removed the icon. Turns out we have a white one that I missed when ...

3 years, 8 months ago (2017-03-28 21:16:12 UTC) #36

Fixed! Removed the icon. Turns out we have a white one that I missed when
scrolling through. I put a mention on the test. Maybe in the future we can get
robolectric to work with different Android Manifests.

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/java/re...
File chrome/android/java/res/layout/tabular_context_menu_row.xml (right):

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/java/re...
chrome/android/java/res/layout/tabular_context_menu_row.xml:17:
android:contentDescription="@string/context_menu_row_icon_description" />
On 2017/03/28 17:31:43, Ted C wrote:
> I think this should be @null here

Done.

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/java/re...
chrome/android/java/res/layout/tabular_context_menu_row.xml:27: <ImageView
On 2017/03/28 17:31:44, Ted C wrote:
> should we remove this for now?

Done.

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java
(right):

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItem.java:120:
context.getString(R.string.context_menu_row_icon_description,
getString(context));
On 2017/03/28 17:31:44, Ted C wrote:
> as we discussed last week, we shouldn't be setting content descriptions on
these
> icons, so I think we can delete this and remove the row_icon_description
string
> entirely

Done.

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2785: <message
name="IDS_CONTEXT_MENU_ROW_SHARE_ICON_DESCRIPTION" desc="The description of the
icon next to the share menu item within a context menu.">
On 2017/03/28 17:31:44, Ted C wrote:
> we can remove this from this patch as well.  we should also just consider
using
> accessibility_menu_share_via as the content description for this.

Done.

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java
(right):

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:30:
public class TabularContextMenuUiTest extends
ChromeActivityTestCaseBase<ChromeActivity> {
On 2017/03/28 17:31:44, Ted C wrote:
> Can this be a robolectric test?  We don't seem to be needing any native code
or
> really any of the chrome activity stuff, so it would be awesome if we could
use
> non-instrumentation tests for these.

We need to grab the assets for each test. As soon as we do anything involving
strings, icons, and other assets we can't use robolectric. It's not that we need
ChromeActivity more like we need the assets that come with it. I've tried to do
robolectric in the past and couldn't get it to work nor could I find any
examples that call any sort of Chrome-like manifest :c.

JJ

The CQ bit was checked by jwanda@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-28 23:03:15 UTC) #37

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/210001

3 years, 8 months ago (2017-03-28 23:04:07 UTC) #38

Ted C

The robolectric comment is an FYI and not something we should investigate as part of ...

3 years, 8 months ago (2017-03-28 23:14:00 UTC) #39

The robolectric comment is an FYI and not something we should investigate as
part of this change.

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java
(right):

https://codereview.chromium.org/2751333006/diff/190001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:30:
public class TabularContextMenuUiTest extends
ChromeActivityTestCaseBase<ChromeActivity> {
On 2017/03/28 21:16:12, JJ wrote:
> On 2017/03/28 17:31:44, Ted C wrote:
> > Can this be a robolectric test?  We don't seem to be needing any native code
> or
> > really any of the chrome activity stuff, so it would be awesome if we could
> use
> > non-instrumentation tests for these.
> 
> We need to grab the assets for each test. As soon as we do anything involving
> strings, icons, and other assets we can't use robolectric. It's not that we
need
> ChromeActivity more like we need the assets that come with it. I've tried to
do
> robolectric in the past and couldn't get it to work nor could I find any
> examples that call any sort of Chrome-like manifest :c.

We might be using an older version of robolectric or we haven't configured it
correctly /fully, but it "seems" like this should be supported:

http://robolectric.org/configuring/ (look for Configure Resource and Asset
Paths).

http://robolectric.org/using-qualifiers/

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:75:
viewHolder.mIcon.setVisibility(iconVisibility);
I would just inline the ternary into here.  You can also drop the ()s around the
icon != null

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java
(right):

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:40:
public void displayMenu(Activity activity, ContextMenuParams params,
Can we actually use Context here instead of Activity (and throughout the file)?

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java
(right):

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:74:
return dialog.createPagerView(getActivity(), new MockMenuParams(url),
itemGroups);
if we can not use activity in the UI code, then I think we should look at making
this class a NativeLibraryTestBase where we don't actually need to start the
activities at all to make them a bit faster.

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:123:
assertEquals(expectedUrl, actualUrl);
I would just inline actualUrl here (and instead of String.valueOf, I would just
do textView.getText().toString();)

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:141:
assertTrue(view.findViewById(R.id.context_header_text).getVisibility() ==
View.GONE);
use assertEquals here and other places where you have equality

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-03-28 23:42:08 UTC) #40

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/237768)

3 years, 8 months ago (2017-03-28 23:42:09 UTC) #41

JJ

I've moved to using context instead of Activity and shifted that to the "Share" cl ...

3 years, 8 months ago (2017-03-29 00:24:27 UTC) #42

I've moved to using context instead of Activity and shifted that to the "Share"
cl (https://codereview.chromium.org/2775373002/).

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:75:
viewHolder.mIcon.setVisibility(iconVisibility);
On 2017/03/28 23:14:00, Ted C wrote:
> I would just inline the ternary into here.  You can also drop the ()s around
the
> icon != null

Done.

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java
(right):

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:40:
public void displayMenu(Activity activity, ContextMenuParams params,
On 2017/03/28 23:14:00, Ted C wrote:
> Can we actually use Context here instead of Activity (and throughout the
file)?

Yeah. It was used in share icon but I'll keep that change stuck in the share
helper portion then.

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java
(right):

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:74:
return dialog.createPagerView(getActivity(), new MockMenuParams(url),
itemGroups);
On 2017/03/28 23:14:00, Ted C wrote:
> if we can not use activity in the UI code, then I think we should look at
making
> this class a NativeLibraryTestBase where we don't actually need to start the
> activities at all to make them a bit faster.

That's super interesting. Is it possible to still use context? (We still need to
grab resources and inflate the view). Also do we have the ability to get a test
case?

Also this is awesome but is slightly moot in a seperate CL
https://codereview.chromium.org/2775373002/

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:123:
assertEquals(expectedUrl, actualUrl);
On 2017/03/28 23:14:00, Ted C wrote:
> I would just inline actualUrl here (and instead of String.valueOf, I would
just
> do textView.getText().toString();)

Done.

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:141:
assertTrue(view.findViewById(R.id.context_header_text).getVisibility() ==
View.GONE);
On 2017/03/28 23:14:00, Ted C wrote:
> use assertEquals here and other places where you have equality

Done.

JJ

The CQ bit was checked by jwanda@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-29 00:37:01 UTC) #43

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/230001

3 years, 8 months ago (2017-03-29 00:37:38 UTC) #44

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-03-29 02:19:34 UTC) #45

commit-bot: I haz the power

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_clang_dbg_recipe/builds/237520)

3 years, 8 months ago (2017-03-29 02:19:35 UTC) #46

Ted C

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java (right): https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java#newcode74 chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:74: return dialog.createPagerView(getActivity(), new MockMenuParams(url), itemGroups); On 2017/03/29 00:24:27, JJ ...

3 years, 8 months ago (2017-03-29 16:18:14 UTC) #47

Ted C

On 2017/03/29 16:18:14, Ted C wrote: > https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java > File > chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java > (right): > ...

3 years, 8 months ago (2017-03-29 16:19:28 UTC) #48

On 2017/03/29 16:18:14, Ted C wrote:
>
https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java
> (right):
> 
>
https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
>
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:74:
> return dialog.createPagerView(getActivity(), new MockMenuParams(url),
> itemGroups);
> On 2017/03/29 00:24:27, JJ wrote:
> > On 2017/03/28 23:14:00, Ted C wrote:
> > > if we can not use activity in the UI code, then I think we should look at
> > making
> > > this class a NativeLibraryTestBase where we don't actually need to start
the
> > > activities at all to make them a bit faster.
> > 
> > That's super interesting. Is it possible to still use context? (We still
need
> to
> > grab resources and inflate the view). Also do we have the ability to get a
> test
> > case?
> > 
> > Also this is awesome but is slightly moot in a seperate CL
> > https://codereview.chromium.org/2775373002/
> 
> You should still be able to use NativeLibraryTestBase if you need an Activity.

> You can create a new Activity with just "new Activity()", which you should do
> instead of having to start up Chrome.  If you don't need ChromeActivity then
you
> should still make the switch.
> 
>
https://codereview.chromium.org/2751333006/diff/230001/chrome/android/java/st...
> File chrome/android/java/strings/android_chrome_strings.grd (right):
> 
>
https://codereview.chromium.org/2751333006/diff/230001/chrome/android/java/st...
> chrome/android/java/strings/android_chrome_strings.grd:2782: <message
> name="IDS_CONTEXT_MENU_ROW_ICON_DESCRIPTION" desc="The description of the icon
> next to an item in a context menu. For example 'Open In New Tab Icon' or
> 'Download Image icon.' This is used for a the description of the icon for
> accessability purposes.">
> is this still needed?

FYI, you'll need to fix findbugs as well:

FindBugs reported the following issues:

SIC_INNER_SHOULD_BE_STATIC: Should be a static inner class
  In class
org.chromium.chrome.browser.contextmenu.TabularContextMenuUiTest$MockMenuParams
  At TabularContextMenuUiTest.java:[lines 35-52]

JJ

Thanks, I'm aware of the bug and a fix for it will be up soon ...

3 years, 8 months ago (2017-03-29 16:26:36 UTC) #49

JJ

Quickly read the comment about testing. It might be good to start thinking about how ...

3 years, 8 months ago (2017-03-29 18:00:22 UTC) #50

Quickly read the comment about testing. It might be good to start thinking about
how to add a manifest around build time? Anyway, tell me if you need any more
help.

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java
(right):

https://codereview.chromium.org/2751333006/diff/210001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUiTest.java:74:
return dialog.createPagerView(getActivity(), new MockMenuParams(url),
itemGroups);
On 2017/03/29 16:18:14, Ted C wrote:
> On 2017/03/29 00:24:27, JJ wrote:
> > On 2017/03/28 23:14:00, Ted C wrote:
> > > if we can not use activity in the UI code, then I think we should look at
> > making
> > > this class a NativeLibraryTestBase where we don't actually need to start
the
> > > activities at all to make them a bit faster.
> > 
> > That's super interesting. Is it possible to still use context? (We still
need
> to
> > grab resources and inflate the view). Also do we have the ability to get a
> test
> > case?
> > 
> > Also this is awesome but is slightly moot in a seperate CL
> > https://codereview.chromium.org/2775373002/
> 
> You should still be able to use NativeLibraryTestBase if you need an Activity.

> You can create a new Activity with just "new Activity()", which you should do
> instead of having to start up Chrome.  If you don't need ChromeActivity then
you
> should still make the switch.

So I tried a few things to confirm any of the methods work
1. Try moving back to roblectric - Doesn't allow us to get assets specifically.
No other tests in Clank show a way for us to get assets. All the junit tests
mainly use Config.NONE. Pastebin for this error. (https://pastebin.com/kfCT36fu)
2. Use NativeLibraryTestBase - "new Activity()" Requires us to call the onCreate
method. Trying to forcefully call it doesn't work on Lolipop devices and below.
As far as I could search I couldn't see any way to run the activity using some
sort of runner.
3. Use BaseActivityTestCaseBase<Activity> - Doesn't work. Fails to construct the
method.

I 100% agree that I wish we could use anything else and I've tried several
options (with more combinations than the ones listed above) but Chrome is ultra
ultra complex. If we get to a point where it's easier for us to put a manifest
in for Robolectric to use, we can jump to that in a heartbeat.

https://codereview.chromium.org/2751333006/diff/230001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2751333006/diff/230001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2782: <message
name="IDS_CONTEXT_MENU_ROW_ICON_DESCRIPTION" desc="The description of the icon
next to an item in a context menu. For example 'Open In New Tab Icon' or
'Download Image icon.' This is used for a the description of the icon for
accessability purposes.">
On 2017/03/29 16:18:14, Ted C wrote:
> is this still needed?

Removed it from the wrong branch, whoops.

JJ

The CQ bit was checked by jwanda@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-29 18:23:19 UTC) #51

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/250001

3 years, 8 months ago (2017-03-29 18:24:16 UTC) #52

JJ

The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, holte@chromium.org Link to ...

3 years, 8 months ago (2017-03-30 00:12:56 UTC) #55

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/270001

3 years, 8 months ago (2017-03-30 00:13:51 UTC) #56

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-03-30 02:15:58 UTC) #57

commit-bot: I haz the power

Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder ...

3 years, 8 months ago (2017-03-30 02:16:00 UTC) #58

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2751333006/270001

3 years, 8 months ago (2017-03-30 02:20:34 UTC) #60

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 270001, "attempt_start_ts": 1490840397342220, "parent_rev": "8c39bf0cbed618be0e6b040be0bf7bed36a9384d", "commit_rev": "1582bcfa6979192f20e21d3ae4afb6951f0793c6"}

3 years, 8 months ago (2017-03-30 04:33:56 UTC) #61

commit-bot: I haz the power

Description was changed from ========== Create the base Custom Context Menu Dialog. This CL creates ...

3 years, 8 months ago (2017-03-30 04:34:46 UTC) #62

commit-bot: I haz the power

3 years, 8 months ago (2017-03-30 04:34:48 UTC) #63

Message was sent while issue was closed.

Committed patchset #15 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/1582bcfa6979192f20e21d3ae4af...

Issue 2751333006: Create the base Custom Context Menu Dialog. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Code based issues fixed. Upcoming patch on pngs and 2 copyright nits #

Patch Set 3 : Modified pngs to only contain what we need. #

Patch Set 4 : Fixed based of dtrainor's comments #

Patch Set 5 : Switched from Custom -> Tabular #

Patch Set 6 : Updated tests and header #

Patch Set 7 : Fixed based off dtrainor's + tedchoc's comments #

Patch Set 8 : Updated with the new Icons #

Patch Set 9 : Minor text fixes #

Patch Set 10 : git rebase #

Patch Set 11 : Fixed based off tedchoc's comments #

Patch Set 12 : Fixed based off tedchoc's comments #

Patch Set 13 : Fixed based off tedchoc's comments #

Patch Set 14 : Updated based off tedchoc's comments #

Patch Set 15 : git rebase #

Messages