Replace Manage Languages with dialog and dropdown item

Replace Manage Languages with dialog and dropdown item

Remove the sub-page. Add a simple dialog to select new languages. Add an
option to the language list to remove a language.

A search box will be added to the dialog next.

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

Committed: https://crrev.com/9a670d48822912050a3c06bf1f3afb1a01c1e0a3
Cr-Commit-Position: refs/heads/master@{#414246}

[michaelpg, 2016-08-22 19:02:21]

Description was changed from

==========
Replace Manage Languages with dialog and dropdown item

Remove the sub-page. Add a simple dialog to select new languages. Add an
option to the language list to remove a language.

A search box will be added to the dialog next.

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

to

==========
Replace Manage Languages with dialog and dropdown item

Remove the sub-page. Add a simple dialog to select new languages. Add an
option to the language list to remove a language.

A search box will be added to the dialog next.

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

[michaelpg, 2016-08-22 20:33:17]

PTAL. Mostly moving content from a subpage to a dialog. I'll look into
scrolling-list improvements in a follow-up once the old page is removed.

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

https://codereview.chromium.org/2265253002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/add_languages_dialog.html:9:
<dom-module id="settings-add-languages-dialog">
Largely a copy of manage_languages_page.html, if you're wondering where these
styles come from.

[stevenjb, 2016-08-23 17:08:04]

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/add_languages_dialog.js
(right):

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/add_languages_dialog.js:40: *
Returns the supported languages that are not yet enabled.
The dependencies for this are subtle, we should document that
languageHelper.isLanguageEnabled depends on languages.enabled.* (assuming that
is the case).

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

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:105:
}.bind(this));
Don't we need to call show() or showModal() on the dialog??

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:232: opacity: .65;
We should define .65 in settings_vars_css.html and use it here and above.

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/languages_page_browsertest.js (right):

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:79: var
addLanguagesButton;
Not needed.

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:94: cancelButton =
dialog.$$('.cancel-button');
assert these?

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:107:
MockInteractions.tap(cancelButton);
Does this test anything?

[michaelpg, 2016-08-23 23:37:07]

PTAL. I've expanded the tests, too.

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/add_languages_dialog.js
(right):

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/add_languages_dialog.js:40: *
Returns the supported languages that are not yet enabled.
On 2016/08/23 17:08:04, stevenjb wrote:
> The dependencies for this are subtle, we should document that
> languageHelper.isLanguageEnabled depends on languages.enabled.* (assuming that
> is the case).

Done.

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

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:105:
}.bind(this));
On 2016/08/23 17:08:04, stevenjb wrote:
> Don't we need to call show() or showModal() on the dialog??

attached() does that (so we don't have to do it async)

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2265253002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:232: opacity: .65;
On 2016/08/23 17:08:04, stevenjb wrote:
> We should define .65 in settings_vars_css.html and use it here and above.

Done.

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/languages_page_browsertest.js (right):

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:79: var
addLanguagesButton;
On 2016/08/23 17:08:04, stevenjb wrote:
> Not needed.

Done.

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:94: cancelButton =
dialog.$$('.cancel-button');
On 2016/08/23 17:08:04, stevenjb wrote:
> assert these?

Done.

https://codereview.chromium.org/2265253002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:107:
MockInteractions.tap(cancelButton);
On 2016/08/23 17:08:04, stevenjb wrote:
> Does this test anything?

Yes, it tests that the cancel button closes the dialog (via the teardown). I've
added comments.

https://codereview.chromium.org/2265253002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/languages_page_browsertest.js (right):

https://codereview.chromium.org/2265253002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:249:
languagesPage.$$('settings-language-detail-page');
hmm oops, i'll revert this

https://codereview.chromium.org/2265253002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:260:
inputMethodsCollapse.querySelector('.list-button:last-of-type');
and this

[stevenjb, 2016-08-24 15:45:18]

lgtm

https://codereview.chromium.org/2265253002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/languages_page_browsertest.js (right):

https://codereview.chromium.org/2265253002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:144: // Canceling
the dialog should close and remove it.
I gues what is not obvious to me is where this test would fail if the dialog was
not closed, i.e. if the close fails?

[michaelpg, 2016-08-24 20:46:41]

https://codereview.chromium.org/2265253002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/languages_page_browsertest.js (right):

https://codereview.chromium.org/2265253002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:144: // Canceling
the dialog should close and remove it.
On 2016/08/24 15:45:18, stevenjb wrote:
> I gues what is not obvious to me is where this test would fail if the dialog
was
> not closed, i.e. if the close fails?

in teardown: "After every test, check that the dialog is removed from the DOM."

[michaelpg, 2016-08-24 20:46:48]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2016-08-24 20:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2265253002/#ps100001
(title: "indent")

[commit-bot: I haz the power, 2016-08-24 20:48:25]

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-08-24 20:52:42]

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

[commit-bot: I haz the power, 2016-08-24 20:52:43]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-08-24 23:56:44]

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-08-25 02:06:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-25 02:09:51]

Description was changed from

==========
Replace Manage Languages with dialog and dropdown item

Remove the sub-page. Add a simple dialog to select new languages. Add an
option to the language list to remove a language.

A search box will be added to the dialog next.

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

to

==========
Replace Manage Languages with dialog and dropdown item

Remove the sub-page. Add a simple dialog to select new languages. Add an
option to the language list to remove a language.

A search box will be added to the dialog next.

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

Committed: https://crrev.com/9a670d48822912050a3c06bf1f3afb1a01c1e0a3
Cr-Commit-Position: refs/heads/master@{#414246}
==========

[commit-bot: I haz the power, 2016-08-25 02:09:52]

Patchset 6 (id:??) landed as
https://crrev.com/9a670d48822912050a3c06bf1f3afb1a01c1e0a3
Cr-Commit-Position: refs/heads/master@{#414246}