Modify PasswordEntryEditor UI to display account credentials in a user-friendly manner

Modify PasswordEntryEditor UI to display account credentials in a user-friendly manner

This CL modifies the way in which account credentials (URL/username) are displayed on PasswordEntryEditor.java and adds buttons which will allow users to view/copy their credentials.

BUG=628669
Committed: https://crrev.com/f7795abfa54c8e1639a817b09f94bd8694d98077
Cr-Commit-Position: refs/heads/master@{#407158}

[dozsa, 2016-07-15 16:34:55]

Description was changed from

==========
Modify PasswordEntryEditor UI to display account credentials in a user-friendly
manner

This CL modifies the way in which account credentials (URL/username) are
displayed on PasswordEntryEditor.java and adds buttons which will allow users to
view/copy their credentials.

BUG= 628669
==========

to

==========
Modify PasswordEntryEditor UI to display account credentials in a user-friendly
manner

This CL modifies the way in which account credentials (URL/username) are
displayed on PasswordEntryEditor.java and adds buttons which will allow users to
view/copy their credentials.

BUG= 628669
==========

[dozsa, 2016-07-15 16:34:55]

dozsa@google.com changed reviewers:
+ vabr@chromium.org

[dozsa, 2016-07-18 15:45:59]

I've added similar buttons to the one displayed in the screenshot in the bug, as
I'm waiting to receive the correct ones from Hwi. I also have yet to hide the
changes behind a flag.

[vabr (Chromium), 2016-07-18 16:00:26]

Hi Paula,

Thanks for the CL. I have not seen the code checking the flag for the new
feature -- how does it know which UI to display when?

Cheers,
Vaclav

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:92:
/*private void hookupCancelDeleteButtons(View v) {
Please make sure to deleted unused code before landing.

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:336: <message
name="IDS_PASSWORD_ENTRY_EDITOR_LINK" desc='Title preceding link on password
entry editor.'>
This does not appear to be used?

[dozsa, 2016-07-18 18:44:52]

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:92:
/*private void hookupCancelDeleteButtons(View v) {
On 2016/07/18 16:00:26, vabr (Chromium) wrote:
> Please make sure to deleted unused code before landing.

Sorry, that's part of the previous view, I commented it out before hiding the
view I'm implementing behind a flag.

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:336: <message
name="IDS_PASSWORD_ENTRY_EDITOR_LINK" desc='Title preceding link on password
entry editor.'>
On 2016/07/18 16:00:26, vabr (Chromium) wrote:
> This does not appear to be used?

It's used in the new UI, before displaying the link of an account.

[dozsa, 2016-07-19 08:25:10]

dozsa@google.com changed reviewers:
+ melandory@chromium.org

[dozsa, 2016-07-19 08:29:16]

Hi Tanja,

I'm having an issue with hiding the newly created xml using the ViewPasswords
feature. This happens using the if else statement in onCreateView, where if the
ViewPasswords is enabled, I set the view to the layout I created, otherwise
keeping it the same as it was. 

I receive a DeadObjectException whenever I try to open the PasswordEntryEditor
page with the way the code is currently implemented. However, if I were to set
the view to either the old xml or the new xml without the if else statement, it
works just fine for both cases. As soon as I add the if else logic, no matter
what I put in the condition, (e.g. 1==1), the app crashes. 

Do you have any idea what I might be doing wrong?

Thank you,
Paula

[vabr (Chromium), 2016-07-19 08:44:38]

Thanks, Paula.

I remembered one thing we did not talk about yet: accessibility labels. The
image-buttons need a way to tell their purpose for users with screen readers. I
assume labelling them something like "copy URL", "copy username", "unmask
password", "copy password" would work. Could you please check out how this is
done in similar UI in Chrome on Android and introduce it here?

Also, I understand that you contacted Hwi to get the approved images. If not or
if you want me to ask Hwi, please let me know.

Otherwise the code looks good and once the accessibility issue is addressed, I'd
suggest to loop in the OWNERS.

Thanks!
Vaclav

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:336: <message
name="IDS_PASSWORD_ENTRY_EDITOR_LINK" desc='Title preceding link on password
entry editor.'>
On 2016/07/18 18:44:52, dozsa wrote:
> On 2016/07/18 16:00:26, vabr (Chromium) wrote:
> > This does not appear to be used?
> 
> It's used in the new UI, before displaying the link of an account.

Ah, that's the @string/password_entry_editor_link reference in the XML? That's
OK then, I was just naively looking for the IDS_... name being mentioned
elsewhere, but seems like that's not how the references work here.

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/password_entry_editor_1.xml (right):

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:2: <!-- Copyright
2014 The Chromium Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:43: <View
Just for my own understanding: what is the purpose of the 0x0 views? Are they
some kind of separators?

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:58:
(TextView) v.findViewById(R.id.password_entry_editor_name);
nit: The line break seems unnecessary. Could you please undo it so that you CL
does not touch this line (where nothing else is changed)?

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:67:
(TextView) v.findViewById(R.id.password_entry_editor_url);
nit: Also here, could you please undo the line break?

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:336: <message
name="IDS_PASSWORD_ENTRY_EDITOR_LINK" desc='Title preceding link on password
entry editor.'>
Is it indeed a link (i.e., does tapping on it get the user to the site)?
Also, we should mention that the URL/site involved is the origin of the saved
password. One suggestion could be:
desc='Title preceding the identification of the site where the stored password
comes from.'

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:337: Url
The mock seems to say "Site" while the string is "Url". Should it be changed?

[melandory, 2016-07-19 09:31:35]

On 2016/07/19 08:29:16, dozsa wrote:
> Hi Tanja,
> 
> I'm having an issue with hiding the newly created xml using the ViewPasswords
> feature. This happens using the if else statement in onCreateView, where if
the
> ViewPasswords is enabled, I set the view to the layout I created, otherwise
> keeping it the same as it was. 
> 
> I receive a DeadObjectException whenever I try to open the PasswordEntryEditor
> page with the way the code is currently implemented. However, if I were to set
> the view to either the old xml or the new xml without the if else statement,
it
> works just fine for both cases. As soon as I add the if else logic, no matter
> what I put in the condition, (e.g. 1==1), the app crashes.
As heads up for future generations, who will enjoy reading threads in review.
The issue was in the setup of the feature, not in the inflating.
> 
> Do you have any idea what I might be doing wrong?
> 
> Thank you,
> Paula

[melandory, 2016-07-19 09:32:49]

Also, should I start looking at the code or it's too early?

[chromium-reviews, 2016-07-19 09:36:09]

I'll let you know as soon as I address Vaclav's comments and add the OWNER
reviewers. Thank you!

On Tue, Jul 19, 2016 at 11:32 AM, <melandory@chromium.org> wrote:

> Also, should I start looking at the code or it's too early?
>
> https://codereview.chromium.org/2156613002/
>



-- 
Paula Valentina Dozsa
SWE Intern Google Munich
NYU Abu Dhabi Class of '18

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dozsa, 2016-07-19 10:00:03]

dozsa@google.com changed reviewers:
+ bauerb@chromium.org

[dozsa, 2016-07-19 10:01:05]

Hello Bernhard and Tanja,

Could you please take a look over this CL?

Thank you!
Paula

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/password_entry_editor_1.xml (right):

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:2: <!-- Copyright
2014 The Chromium Authors. All rights reserved.
On 2016/07/19 08:44:38, vabr (Chromium) wrote:
> nit: 2016

Done.

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:43: <View
On 2016/07/19 08:44:38, vabr (Chromium) wrote:
> Just for my own understanding: what is the purpose of the 0x0 views? Are they
> some kind of separators?

Yes, I'm using them so that the copy/view buttons are displayed at the end of
the line instead of right next to the account credentials.

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:58:
(TextView) v.findViewById(R.id.password_entry_editor_name);
On 2016/07/19 08:44:38, vabr (Chromium) wrote:
> nit: The line break seems unnecessary. Could you please undo it so that you CL
> does not touch this line (where nothing else is changed)?

Done.

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:67:
(TextView) v.findViewById(R.id.password_entry_editor_url);
On 2016/07/19 08:44:38, vabr (Chromium) wrote:
> nit: Also here, could you please undo the line break?

Done.

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:336: <message
name="IDS_PASSWORD_ENTRY_EDITOR_LINK" desc='Title preceding link on password
entry editor.'>
On 2016/07/19 08:44:38, vabr (Chromium) wrote:
> Is it indeed a link (i.e., does tapping on it get the user to the site)?
> Also, we should mention that the URL/site involved is the origin of the saved
> password. One suggestion could be:
> desc='Title preceding the identification of the site where the stored password
> comes from.'

Sorry, not a link, just the title preceding the link. Does this current name
work?

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:337: Url
On 2016/07/19 08:44:38, vabr (Chromium) wrote:
> The mock seems to say "Site" while the string is "Url". Should it be changed?

Sorry, my bad, changed it to "Site."

[vabr (Chromium), 2016-07-19 10:54:12]

Thanks, Paula, for addressing the comments.
I have a few more below, but the CL in general LGTM (still assuming that OWNERS
will review).

Cheers,
Vaclav

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/40001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:336: <message
name="IDS_PASSWORD_ENTRY_EDITOR_LINK" desc='Title preceding link on password
entry editor.'>
On 2016/07/19 10:01:05, dozsa wrote:
> On 2016/07/19 08:44:38, vabr (Chromium) wrote:
> > Is it indeed a link (i.e., does tapping on it get the user to the site)?
> > Also, we should mention that the URL/site involved is the origin of the
saved
> > password. One suggestion could be:
> > desc='Title preceding the identification of the site where the stored
password
> > comes from.'
> 
> Sorry, not a link, just the title preceding the link. Does this current name
> work?

It does, thanks!

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:32:
public String VIEW_PASSWORDS = "view-passwords";
Should this be final?

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:355: View password
optional nit: I'm not sure how much "view" makes sense for screen reader users.
My suggestion would be "unmask", which seems to make sense for everyone. If you
get a chance, it would be good to have advice from some accessibility or UX
expert on the phrasing. I won't block this CL on that, though.

[Bernhard Bauer, 2016-07-19 11:46:49]

We only have the images in hdpi? What is going to happen at a higher resolution?

Also, sorry, but the princess is in another cast-- I mean, I'm only a reviewer
for New Tab Page files in chrome/android/java/res/.

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/password_entry_editor_1.xml (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:33:
android:weightSum="1" >
This is unnecessary -- the default is going to be the sum of the child weights.

(Also, no space before the closing angle bracket).

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:32:
public String VIEW_PASSWORDS = "view-passwords";
On 2016/07/19 10:54:12, vabr (Chromium) wrote:
> Should this be final?

And static.

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:45:
v = inflater.inflate(R.layout.password_entry_editor_1, container, false);
Props for using zero-based indexing, but can we use a more descriptive suffix
for this than "1"? What distinguishes this layout from the other one?

https://codereview.chromium.org/2156613002/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:34:
&password_manager::features::kViewPasswords,
Please keep these ordered alphabetically.

[dozsa, 2016-07-19 11:51:54]

On 2016/07/19 11:46:49, Bernhard Bauer wrote:
> We only have the images in hdpi? What is going to happen at a higher
resolution?

Sorry, I forgot to mention that I am waiting for the final images from the UX
designer who created the mocks for this.

[dozsa, 2016-07-20 12:07:09]

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/password_entry_editor_1.xml (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:33:
android:weightSum="1" >
On 2016/07/19 11:46:50, Bernhard Bauer wrote:
> This is unnecessary -- the default is going to be the sum of the child
weights.
> 
> (Also, no space before the closing angle bracket).

Done.

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java
(right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:32:
public String VIEW_PASSWORDS = "view-passwords";
On 2016/07/19 10:54:12, vabr (Chromium) wrote:
> Should this be final?

Done.

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/password/PasswordEntryEditor.java:45:
v = inflater.inflate(R.layout.password_entry_editor_1, container, false);
On 2016/07/19 11:46:50, Bernhard Bauer wrote:
> Props for using zero-based indexing, but can we use a more descriptive suffix
> for this than "1"? What distinguishes this layout from the other one?

Would calling it "interactive" be appropriate? Given that it would permit users
to copy/view credentials.

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:355: View password
On 2016/07/19 10:54:12, vabr (Chromium) wrote:
> optional nit: I'm not sure how much "view" makes sense for screen reader
users.
> My suggestion would be "unmask", which seems to make sense for everyone. If
you
> get a chance, it would be good to have advice from some accessibility or UX
> expert on the phrasing. I won't block this CL on that, though.

I've changed it to "unmask" for now, waiting for confirmation from UX designer.

https://codereview.chromium.org/2156613002/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_feature_list.cc:34:
&password_manager::features::kViewPasswords,
On 2016/07/19 11:46:50, Bernhard Bauer wrote:
> Please keep these ordered alphabetically.

Done.

[dozsa, 2016-07-20 13:27:19]

The CQ bit was checked by dozsa@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 13:27:32]

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

[Bernhard Bauer, 2016-07-20 14:25:56]

Can you remove the _googblue_24dp part from the icon names? Also, why is the RTL
xxxhpdi icon in sw600dp? Also, could we get the RTL icons by just mirroring the
regular icon?

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/password_entry_editor_1.xml (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:33:
android:weightSum="1" >
On 2016/07/20 12:07:08, dozsa wrote:
> On 2016/07/19 11:46:50, Bernhard Bauer wrote:
> > This is unnecessary -- the default is going to be the sum of the child
> weights.
> > 
> > (Also, no space before the closing angle bracket).
> 
> Done.

Not really? I still see this.

[commit-bot: I haz the power, 2016-07-20 14:30:30]

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

[commit-bot: I haz the power, 2016-07-20 14:30:31]

Dry run: This issue passed the CQ dry run.

[dozsa, 2016-07-20 15:32:59]

I've removed the _googblue_24dp part from the icon names, and thank you for
noticing the misplaced xxxhpdi icon. I'm not sure what you mean when you say I
should mirror the regular icon? I was told by the UX designer that the icons can
be found at https://icons.googleplex.com/#icon=ic_content_copy&search=copy and
https://icons.googleplex.com/#icon=ic_visibility_off&search=eye so I just
downloaded the .png files from there and included them in the specified
directories.

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/password_entry_editor_1.xml (right):

https://codereview.chromium.org/2156613002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/password_entry_editor_1.xml:33:
android:weightSum="1" >
On 2016/07/20 14:25:56, Bernhard Bauer wrote:
> On 2016/07/20 12:07:08, dozsa wrote:
> > On 2016/07/19 11:46:50, Bernhard Bauer wrote:
> > > This is unnecessary -- the default is going to be the sum of the child
> > weights.
> > > 
> > > (Also, no space before the closing angle bracket).
> > 
> > Done.
> 
> Not really? I still see this.

Oops. Done now.

[Bernhard Bauer, 2016-07-21 09:06:40]

On 2016/07/20 15:32:59, dozsa wrote:
> I've removed the _googblue_24dp part from the icon names, and thank you for
> noticing the misplaced xxxhpdi icon. I'm not sure what you mean when you say I
> should mirror the regular icon? I was told by the UX designer that the icons
can
> be found at https://icons.googleplex.com/#icon=ic_content_copy&search=copy and
> https://icons.googleplex.com/#icon=ic_visibility_off&search=eye so I just
> downloaded the .png files from there and included them in the specified
> directories.

You have a mirrored version of e.g. drawable-hdpi/ic_content_copy.png in
drawable-ldrtl-hdpi-v17/ic_content_copy.png. For one, did you actually check
with a designer whether we want a mirrored copy icon for RTL layouts? And even
if that is the desired behavior, we don't necessarily need to bundle a mirrored
copy in the resulting APK -- we could just take the regular image and apply a
transformation to it when displaying it.

https://codereview.chromium.org/2156613002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/layout/password_entry_editor_interactive.xml
(right):

https://codereview.chromium.org/2156613002/diff/120001/chrome/android/java/re...
chrome/android/java/res/layout/password_entry_editor_interactive.xml:115:
android:weightSum="1" >
Remove the weightSum and the space here as well.

[dozsa, 2016-07-21 09:42:21]

Okay, I get what you mean now, thanks for noticing the mirrored images. I just
included them without realizing they were mirrored, and we only need the version
that appears in the mocks.

[dozsa, 2016-07-21 09:43:43]

https://codereview.chromium.org/2156613002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/layout/password_entry_editor_interactive.xml
(right):

https://codereview.chromium.org/2156613002/diff/120001/chrome/android/java/re...
chrome/android/java/res/layout/password_entry_editor_interactive.xml:115:
android:weightSum="1" >
On 2016/07/21 09:06:40, Bernhard Bauer wrote:
> Remove the weightSum and the space here as well.

Done.

[Bernhard Bauer, 2016-07-21 09:46:48]

LGTM, but you should probably get an additional reviewer for the files in
chrome/android/java/res/, as I'm only an OWNER for New Tab Page stuff there.

[dozsa, 2016-07-21 09:50:10]

dozsa@google.com changed reviewers:
+ tedchoc@chromium.org

[dozsa, 2016-07-21 09:50:56]

Hello Ted,

Could you please take a look over this CL?

Best,
Paula

[dozsa, 2016-07-21 11:05:35]

On 2016/07/21 09:50:56, dozsa wrote:
> Hello Ted,
> 
> Could you please take a look over this CL?
> 
> Best,
> Paula

Hello again Ted, 

I forgot to mention that I only need the changes to chrome/android/java/res/
reviewed.

Best,
Paula

[Ted C, 2016-07-21 17:33:45]

On 2016/07/21 11:05:35, dozsa wrote:
> On 2016/07/21 09:50:56, dozsa wrote:
> > Hello Ted,
> > 
> > Could you please take a look over this CL?
> > 
> > Best,
> > Paula
> 
> Hello again Ted, 
> 
> I forgot to mention that I only need the changes to chrome/android/java/res/
> reviewed.
> 
> Best,
> Paula

Did you run these pngs through the optimization steps mentioned here:
https://cs.chromium.org/chromium/src/chrome/app/theme/README?q=chrome/app/the...

i.e. this command
tools/resources/optimize-png-files.sh -o2 new_pngs_dir

FWIW, I recommend moving your assets to a new folder before running this as it
is quite slow and it will attempt to run on all assets if you just point it
at res/

[dozsa, 2016-07-22 13:03:48]

Hi Ted, I've just optimized the pngs as you suggested.

[Ted C, 2016-07-22 13:48:17]

lgtm

[dozsa, 2016-07-22 13:54:18]

The CQ bit was checked by dozsa@google.com

[dozsa, 2016-07-22 13:54:19]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2156613002/#ps160001
(title: "Optimize pngs")

[commit-bot: I haz the power, 2016-07-22 13:54:31]

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

[commit-bot: I haz the power, 2016-07-22 13:56:10]

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

[commit-bot: I haz the power, 2016-07-22 13:56:12]

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...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[dozsa, 2016-07-22 14:08:46]

The CQ bit was checked by dozsa@google.com

[dozsa, 2016-07-22 14:08:46]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
bauerb@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2156613002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-22 14:09:01]

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

[commit-bot: I haz the power, 2016-07-22 14:51:51]

Description was changed from

==========
Modify PasswordEntryEditor UI to display account credentials in a user-friendly
manner

This CL modifies the way in which account credentials (URL/username) are
displayed on PasswordEntryEditor.java and adds buttons which will allow users to
view/copy their credentials.

BUG= 628669
==========

to

==========
Modify PasswordEntryEditor UI to display account credentials in a user-friendly
manner

This CL modifies the way in which account credentials (URL/username) are
displayed on PasswordEntryEditor.java and adds buttons which will allow users to
view/copy their credentials.

BUG= 628669
==========

[commit-bot: I haz the power, 2016-07-22 14:51:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-22 14:53:12]

Description was changed from

==========
Modify PasswordEntryEditor UI to display account credentials in a user-friendly
manner

This CL modifies the way in which account credentials (URL/username) are
displayed on PasswordEntryEditor.java and adds buttons which will allow users to
view/copy their credentials.

BUG= 628669
==========

to

==========
Modify PasswordEntryEditor UI to display account credentials in a user-friendly
manner

This CL modifies the way in which account credentials (URL/username) are
displayed on PasswordEntryEditor.java and adds buttons which will allow users to
view/copy their credentials.

BUG= 628669

Committed: https://crrev.com/f7795abfa54c8e1639a817b09f94bd8694d98077
Cr-Commit-Position: refs/heads/master@{#407158}
==========

[commit-bot: I haz the power, 2016-07-22 14:53:14]

Patchset 10 (id:??) landed as
https://crrev.com/f7795abfa54c8e1639a817b09f94bd8694d98077
Cr-Commit-Position: refs/heads/master@{#407158}