Add context menus in md-setting for the address and credit card lists.

Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348, 607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b609f48aa006b1fce36c7c9b1edd31cf766aca6c
Cr-Commit-Position: refs/heads/master@{#395199}

[hcarmona, 2016-05-18 21:20:51]

Description was changed from

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
==========

to

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-05-18 21:59:38]

Description was changed from

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-05-18 21:59:39]

hcarmona@chromium.org changed reviewers:
+ tommycli@chromium.org

[hcarmona, 2016-05-18 22:00:20]

PTAL, Thanks!

[tommycli, 2016-05-18 23:29:20]

https://codereview.chromium.org/1992043002/diff/1/chrome/app/settings_strings...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1992043002/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:184: <message name="IDS_SETTINGS_ADDRESS_MENU"
desc="Label for a button shows options available for this address item.
Available options should be 'Edit' and 'Remove'">
Can these strings be consolidated with the IDS_SETTINGS_PASSWORD_MENU strings?

These all manage passwords/autofill right?

Just a suggestion, let me know if there's some reason they must be different.

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
(right):

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:64:
/** @type {CrSharedMenuElement} */(
Are all these typecasts definitely necessary for closure?

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:92:
var data =
Is data currently used? Or is this for the future?

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:104:
var data =
same here

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:172:
this.$.creditCardSharedMenu).closeMenu();
I think these shouldn't require typecasts if CrSharedMenuElement is listed under
the dependencies of compiled_resources2.gyp. I can look on your machine if I'm
wrong...

[hcarmona, 2016-05-20 20:18:58]

https://codereview.chromium.org/1992043002/diff/1/chrome/app/settings_strings...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1992043002/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:184: <message name="IDS_SETTINGS_ADDRESS_MENU"
desc="Label for a button shows options available for this address item.
Available options should be 'Edit' and 'Remove'">
On 2016/05/18 23:29:20, tommycli wrote:
> Can these strings be consolidated with the IDS_SETTINGS_PASSWORD_MENU strings?
> 
> These all manage passwords/autofill right?
> 
> Just a suggestion, let me know if there's some reason they must be different.

The words are the same in English, they may be different in other
languages depending on the context.

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
(right):

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:64:
/** @type {CrSharedMenuElement} */(
On 2016/05/18 23:29:20, tommycli wrote:
> Are all these typecasts definitely necessary for closure?

Looked at them again, got rid of all the unnecessary ones.

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:92:
var data =
On 2016/05/18 23:29:20, tommycli wrote:
> Is data currently used? Or is this for the future?

Not used currently. Removed and will add when used.
Same for all uses of |data|.

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:104:
var data =
On 2016/05/18 23:29:20, tommycli wrote:
> same here

same.

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:172:
this.$.creditCardSharedMenu).closeMenu();
On 2016/05/18 23:29:20, tommycli wrote:
> I think these shouldn't require typecasts if CrSharedMenuElement is listed
under
> the dependencies of compiled_resources2.gyp. I can look on your machine if I'm
> wrong... 

Done.

[tommycli, 2016-05-20 20:24:18]

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
(right):

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:64:
/** @type {CrSharedMenuElement} */(
On 2016/05/20 20:18:58, Hector Carmona wrote:
> On 2016/05/18 23:29:20, tommycli wrote:
> > Are all these typecasts definitely necessary for closure?
> 
> Looked at them again, got rid of all the unnecessary ones.

Perhaps try something that looks like this for assigning a type to the Event
within the @param tag:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

[hcarmona, 2016-05-20 20:59:51]

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
(right):

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:64:
/** @type {CrSharedMenuElement} */(
On 2016/05/20 20:24:18, tommycli wrote:
> On 2016/05/20 20:18:58, Hector Carmona wrote:
> > On 2016/05/18 23:29:20, tommycli wrote:
> > > Are all these typecasts definitely necessary for closure?
> > 
> > Looked at them again, got rid of all the unnecessary ones.
> 
> Perhaps try something that looks like this for assigning a type to the Event
> within the @param tag:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Can't do that in this case because |e| also needs to be an Event in
order to get |localTarget| from Polymer.dom and call |stopPropagation|.

[tommycli, 2016-05-20 21:14:42]

LGTM assuming you either (per offline discussion):
a) Consolidate the Menu/Edit/Remove strings
OR
b) Leave them separate but write the justification as to why they are separate
as a comment within the desc= field.

Thanks,

Tommy

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
(right):

https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:64:
/** @type {CrSharedMenuElement} */(
On 2016/05/20 20:59:51, Hector Carmona wrote:
> On 2016/05/20 20:24:18, tommycli wrote:
> > On 2016/05/20 20:18:58, Hector Carmona wrote:
> > > On 2016/05/18 23:29:20, tommycli wrote:
> > > > Are all these typecasts definitely necessary for closure?
> > > 
> > > Looked at them again, got rid of all the unnecessary ones.
> > 
> > Perhaps try something that looks like this for assigning a type to the Event
> > within the @param tag:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> Can't do that in this case because |e| also needs to be an Event in
> order to get |localTarget| from Polymer.dom and call |stopPropagation|.

Ah. I see that's a good explanation.

[hcarmona, 2016-05-20 22:44:17]

On 2016/05/20 21:14:42, tommycli wrote:
> LGTM assuming you either (per offline discussion):
> a) Consolidate the Menu/Edit/Remove strings
> OR
> b) Leave them separate but write the justification as to why they are separate
> as a comment within the desc= field.
> 
> Thanks,
> 
> Tommy
> 
>
https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
> File
> chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
> (right):
> 
>
https://codereview.chromium.org/1992043002/diff/1/chrome/browser/resources/se...
>
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:64:
> /** @type {CrSharedMenuElement} */(
> On 2016/05/20 20:59:51, Hector Carmona wrote:
> > On 2016/05/20 20:24:18, tommycli wrote:
> > > On 2016/05/20 20:18:58, Hector Carmona wrote:
> > > > On 2016/05/18 23:29:20, tommycli wrote:
> > > > > Are all these typecasts definitely necessary for closure?
> > > > 
> > > > Looked at them again, got rid of all the unnecessary ones.
> > > 
> > > Perhaps try something that looks like this for assigning a type to the
Event
> > > within the @param tag:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > 
> > Can't do that in this case because |e| also needs to be an Event in
> > order to get |localTarget| from Polymer.dom and call |stopPropagation|.
> 
> Ah. I see that's a good explanation.

Can you take a quick look at this? I updated the i18n to consolidate
"Menu" because it's a noun. I also added |meaning| to "Edit" and
"Remove" since those are verbs. I learned that verbs must match up to
their subject in some languages we support (Swahili is an example).

Thanks!

[tommycli, 2016-05-20 22:59:31]

https://codereview.chromium.org/1992043002/diff/40001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1992043002/diff/40001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:175: <message name="IDS_SETTINGS_OVERFLOW_MENU"
desc="Alt text for the overflow button on a list item. Triggers a context menu
when clicked.">
I see the desc on this changed as well. To confirm: the actual button is just an
icon but this is the alt text? The "overflow button" is the three dot thing?

[tommycli, 2016-05-20 23:00:30]

LGTM

nevermind I answered my own question by reading the code.

Thanks for digging so deeply into the i18n issue. Perhaps there are other areas
in Settings where we should de-dupe strings for 100% language accuracy.

Tommy

[hcarmona, 2016-05-20 23:04:40]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2016-05-20 23:05:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992043002/40001

[commit-bot: I haz the power, 2016-05-20 23:32:48]

Description was changed from

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-05-20 23:32:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-20 23:35:04]

Description was changed from

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add context menus in md-setting for the address and credit card lists.

Menus will show/hide appropriately.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-05-20 23:35:05]

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