Use one instance of language settings detail menu for all languages

Use one instance of language settings detail menu for all languages

Wrap the language settings overflow menu in a <cr-shared-menu>. The contents
of the language detail page will be moved into this menu, so it would be
better not to stamp it once for each item in the list of enabled languages.

BUG=630982
TEST=SettingsLanguagesPageBrowserTest
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[michaelpg, 2016-08-24 23:55:44]

Description was changed from

==========
Use one instance of language settings detail menu for all languages

Wrap the language settings overflow menu in a <cr-shared-menu>. The contents
of the language detail page will be moved into this menu, so it would be
better not to stamp it once for each item in the list of enabled languages.

BUG=630982
TEST=SettingsLanguagesPageBrowserTest
R=stevenjb@chromium.org
==========

to

==========
Use one instance of language settings detail menu for all languages

Wrap the language settings overflow menu in a <cr-shared-menu>. The contents
of the language detail page will be moved into this menu, so it would be
better not to stamp it once for each item in the list of enabled languages.

BUG=630982
TEST=SettingsLanguagesPageBrowserTest
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-08-25 02:14:41]

PTAL.

[stevenjb, 2016-08-25 16:36:58]

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.html:169:
lazy-render. -->
Any reason not to just use dom-if now?

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:108:
isFirstLanguage_: function(language) {
@param for language

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:117:
isLastLanguage_: function(language) {
ditto

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:118: return
language == this.languages.enabled.slice(-1)[0];
eww, but I guess that's JS for you. (I did a sear and, it seems like this is
what people do, efficiency be damned).

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:133:
onMoveUpTap_: function(e) {
elim (e) if unused

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:341:
toggleMenu_: function(e) {
@param for e

[michaelpg, 2016-08-26 22:16:21]

PTAL

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.html:169:
lazy-render. -->
On 2016/08/25 16:36:57, stevenjb wrote:
> Any reason not to just use dom-if now?

Meh, a tiny bit extra work. Done.

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:108:
isFirstLanguage_: function(language) {
On 2016/08/25 16:36:57, stevenjb wrote:
> @param for language

Done.

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:117:
isLastLanguage_: function(language) {
On 2016/08/25 16:36:57, stevenjb wrote:
> ditto

Done.

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:118: return
language == this.languages.enabled.slice(-1)[0];
On 2016/08/25 16:36:57, stevenjb wrote:
> eww, but I guess that's JS for you. (I did a sear and, it seems like this is
> what people do, efficiency be damned).
> 

pretty sure creating a 1-length array containing a pointer isn't the bottleneck
of opening the menu :-)

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:133:
onMoveUpTap_: function(e) {
On 2016/08/25 16:36:57, stevenjb wrote:
> elim (e) if unused

Done.

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:341:
toggleMenu_: function(e) {
On 2016/08/25 16:36:57, stevenjb wrote:
> @param for e

Done.

[stevenjb, 2016-08-26 22:56:48]

lgtm

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/2277633003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.html:169:
lazy-render. -->
On 2016/08/26 22:16:20, michaelpg wrote:
> On 2016/08/25 16:36:57, stevenjb wrote:
> > Any reason not to just use dom-if now?
> 
> Meh, a tiny bit extra work. Done.

:)

https://codereview.chromium.org/2277633003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2277633003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:353:
this.$.menuTemplate.render();
Nice. I assume this is more robust than data-binding to, e.g. if="[[showMenu]]"?

[michaelpg, 2016-08-27 02:04:59]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-08-27 02:05:36]

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

[michaelpg, 2016-08-27 02:07:31]

https://codereview.chromium.org/2277633003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/2277633003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:353:
this.$.menuTemplate.render();
On 2016/08/26 22:56:48, stevenjb wrote:
> Nice. I assume this is more robust than data-binding to, e.g.
if="[[showMenu]]"?

Yeah, I didn't want to interfere with the menu's own show/hide logic.

[commit-bot: I haz the power, 2016-08-27 10:39:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-27 10:42:51]

Description was changed from

==========
Use one instance of language settings detail menu for all languages

Wrap the language settings overflow menu in a <cr-shared-menu>. The contents
of the language detail page will be moved into this menu, so it would be
better not to stamp it once for each item in the list of enabled languages.

BUG=630982
TEST=SettingsLanguagesPageBrowserTest
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use one instance of language settings detail menu for all languages

Wrap the language settings overflow menu in a <cr-shared-menu>. The contents
of the language detail page will be moved into this menu, so it would be
better not to stamp it once for each item in the list of enabled languages.

BUG=630982
TEST=SettingsLanguagesPageBrowserTest
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-08-27 10:42:52]

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