MD Settings: add empty list message for spell-check edit page.

MD Settings: add empty list message for spell-check edit page.

Also move the word list to a template dom-if, and fix auto-scroll when new words are added.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2690263002
Cr-Commit-Position: refs/heads/master@{#452633}
Committed: https://chromium.googlesource.com/chromium/src/+/08b33480b93950a0153c179f5336aeaf382ff8d5

[scottchen, 2017-02-13 22:46:24]

Description was changed from

==========
MD Settings: add empty list message for spell-check edit page.

Also move the word list to a template dom-if, and fix auto-scroll when new words
are added.

BUG=684152
==========

to

==========
MD Settings: add empty list message for spell-check edit page.

Also move the word list to a template dom-if, and fix auto-scroll when new words
are added.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[scottchen, 2017-02-15 19:34:45]

scottchen@chromium.org changed reviewers:
+ dpapad@chromium.org

[scottchen, 2017-02-15 19:34:46]

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js
(left):

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:24:
chrome.languageSettingsPrivate.getSpellcheckWords(function(words) {
This change is needed so that the languageSettingsPrivate can be spoofed during
test.

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:114:
index = this.words_.length - 1;
This was not scrolling to the bottom correctly

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:119:
this.async(function(){
With the list being in a dom-if now, this would need to be async to give it time
to show up in the dom.

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:463: */
This is fixed now so we can remove it.

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/languages_page_browsertest.js:453: if
(!cr.isMac) {
This is being moved to the spellcheck-specific test.

[scottchen, 2017-02-15 19:35:03]

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

[commit-bot: I haz the power, 2017-02-15 19:35:50]

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

[dpapad, 2017-02-15 20:36:37]

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/edit_dictionary_page.html
(right):

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.html:56:
<iron-list id="list" items="{{words_}}" on-created="onListLoad_">
is the on-created="onListLoad_" part still relevant? I don't see such a function
in the JS file.

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:110:
var index = this.words_.indexOf(word);
Unrelated to your fix: I hope real users don't have too many words. Otherwise
linear search is not the best choice here.

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:127: *
@return {!boolea}
Typo (should cause compilation error).

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:130:
return !!(this.words_ && this.words_.length);
words_ is initialized to the empty array. Why do we need to check if it exists?

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:463: */
On 2017/02/15 at 19:34:46, scottchen wrote:
> This is fixed now so we can remove it.

Can you elaborate how you verified that this works without the iron-resize?

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:6: 
Nit (optional): Remove unnecessary blank line?

https://codereview.chromium.org/2690263002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:85: === 'none'); //
Make sure add-word button actually clickable.
https://google.github.io/styleguide/jsguide.html#formatting-where-to-break

Line break should be after the operator.

https://codereview.chromium.org/2690263002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:90:
editDictPage.words_ = [];
Can we improve this test? Currently it is accessing private state, as well as
shortcutting code that should be tested.

Inside the ready() lifecycle method a call to getSpellcheckWords() is made. We
should our fake languagePrivate implementation provide the empty list, instead
of assigning to words_ directly. So this way the ready() callback is tested
itself. TestBrowserProxy can be used to notify the test when a method was
walled, something as follows (assuming that fakePrivateApi extends
TestBrowserProxy).

test('mytest', function() {
 // setup fake data
 fakePrivateApi.setWords([]);
 // attach element to DOM ...
 
 // Wait for the |getSpellheckWords| method to be called.
 return fakePrivateApi.whenCalled('getSpellcheckWords').then(function() {
   // Do assertions here.
 });
});

[commit-bot: I haz the power, 2017-02-15 21:16:59]

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

[commit-bot: I haz the power, 2017-02-15 21:16:59]

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

[scottchen, 2017-02-22 21:42:25]

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/edit_dictionary_page.html
(right):

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.html:56:
<iron-list id="list" items="{{words_}}" on-created="onListLoad_">
On 2017/02/15 20:36:37, dpapad wrote:
> is the on-created="onListLoad_" part still relevant? I don't see such a
function
> in the JS file.

oops left over from test code, removing.

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:110:
var index = this.words_.indexOf(word);
On 2017/02/15 20:36:37, dpapad wrote:
> Unrelated to your fix: I hope real users don't have too many words. Otherwise
> linear search is not the best choice here.

Acknowledged.

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:127: *
@return {!boolea}
On 2017/02/15 20:36:37, dpapad wrote:
> Typo (should cause compilation error).

Done.

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:130:
return !!(this.words_ && this.words_.length);
On 2017/02/15 20:36:37, dpapad wrote:
> words_ is initialized to the empty array. Why do we need to check if it
exists?

I was thinking just in case languageSettingsPrivate.getSpellcheckWords(cb) calls
cb with null or undefined for whatever reason (or in the future). Or do you
think its not necessary to pre-error-prevent?

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_page.js:463: */
On 2017/02/15 20:36:37, dpapad wrote:
> On 2017/02/15 at 19:34:46, scottchen wrote:
> > This is fixed now so we can remove it.
> 
> Can you elaborate how you verified that this works without the iron-resize?

michaelpg@ says the iron list used to not show up when  you navigate to the
subpage, but this doesn't happen anymore, and the iron-list displays as expected
with this function removed.

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:6: 
On 2017/02/15 20:36:37, dpapad wrote:
> Nit (optional): Remove unnecessary blank line?

Done.

https://codereview.chromium.org/2690263002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:85: === 'none'); //
Make sure add-word button actually clickable.
On 2017/02/15 20:36:37, dpapad wrote:
> https://google.github.io/styleguide/jsguide.html#formatting-where-to-break
> 
> Line break should be after the operator.

Done.

https://codereview.chromium.org/2690263002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:90:
editDictPage.words_ = [];
On 2017/02/15 20:36:37, dpapad wrote:
> Can we improve this test? Currently it is accessing private state, as well as
> shortcutting code that should be tested.
> 
> Inside the ready() lifecycle method a call to getSpellcheckWords() is made. We
> should our fake languagePrivate implementation provide the empty list, instead
> of assigning to words_ directly. So this way the ready() callback is tested
> itself. TestBrowserProxy can be used to notify the test when a method was
> walled, something as follows (assuming that fakePrivateApi extends
> TestBrowserProxy).
> 
> test('mytest', function() {
>  // setup fake data
>  fakePrivateApi.setWords([]);
>  // attach element to DOM ...
>  
>  // Wait for the |getSpellheckWords| method to be called.
>  return fakePrivateApi.whenCalled('getSpellcheckWords').then(function() {
>    // Do assertions here.
>  });
> });

Done.

https://codereview.chromium.org/2690263002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(left):

https://codereview.chromium.org/2690263002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:929:
LocalizedString localized_strings[] = {
presubmit asked me to run `git cl format .` and this happened.

[scottchen, 2017-02-22 21:42:34]

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

[commit-bot: I haz the power, 2017-02-22 21:43:30]

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

[dpapad, 2017-02-22 21:58:52]

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:130:
return !!(this.words_ && this.words_.length);
On 2017/02/22 at 21:42:25, scottchen wrote:
> On 2017/02/15 20:36:37, dpapad wrote:
> > words_ is initialized to the empty array. Why do we need to check if it
exists?
> 
> I was thinking just in case languageSettingsPrivate.getSpellcheckWords(cb)
calls cb with null or undefined for whatever reason (or in the future). Or do
you think its not necessary to pre-error-prevent?

I don't think this is necessary to be defensive in that way. The JSDoc at [1],
should be reflecting the implementation, and therefore no null/undefined should
be returned. We are defensive by trusting the JSDoc and the JSCompiler to catch
errors.

[1]
https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/lan...

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/edit_dictionary_page_test.js (right):

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:5:
cr.define('settings-edit-dictionary-page', function() {
Can we remove the unnecessary cr.define wrapper?

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:46: var
editDictPage;
Nit: Let's add @type annotations for these variables.

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:92:
assertTrue(!!editDictPage.root.querySelector('#noWordsLabel'));
Nit: |root| is not a speced property AFAIR. Does it work if you just replace
with |shadowRoot|? You could also just use the helper $$() instead.

[Dan Beam, 2017-02-22 22:42:05]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2017-02-22 22:42:06]

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/edit_dictionary_page_test.js (right):

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:5:
cr.define('settings-edit-dictionary-page', function() {
On 2017/02/22 21:58:52, dpapad wrote:
> Can we remove the unnecessary cr.define wrapper?

+1

[Dan Beam, 2017-02-22 22:42:12]

Description was changed from

==========
MD Settings: add empty list message for spell-check edit page.

Also move the word list to a template dom-if, and fix auto-scroll when new words
are added.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: add empty list message for spell-check edit page.

Also move the word list to a template dom-if, and fix auto-scroll when new words
are added.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-02-22 22:42:12]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[scottchen, 2017-02-22 23:30:18]

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/edit_dictionary_page_test.js (right):

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:5:
cr.define('settings-edit-dictionary-page', function() {
On 2017/02/22 22:42:05, Dan Beam wrote:
> On 2017/02/22 21:58:52, dpapad wrote:
> > Can we remove the unnecessary cr.define wrapper?
> 
> +1

Done.

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:46: var
editDictPage;
On 2017/02/22 21:58:52, dpapad wrote:
> Nit: Let's add @type annotations for these variables.

Done.

https://codereview.chromium.org/2690263002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:92:
assertTrue(!!editDictPage.root.querySelector('#noWordsLabel'));
On 2017/02/22 21:58:52, dpapad wrote:
> Nit: |root| is not a speced property AFAIR. Does it work if you just replace
> with |shadowRoot|? You could also just use the helper $$() instead.

Done.

[dpapad, 2017-02-23 00:32:03]

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:130:
return !!(this.words_ && this.words_.length);
On 2017/02/22 at 21:58:52, dpapad wrote:
> On 2017/02/22 at 21:42:25, scottchen wrote:
> > On 2017/02/15 20:36:37, dpapad wrote:
> > > words_ is initialized to the empty array. Why do we need to check if it
exists?
> > 
> > I was thinking just in case languageSettingsPrivate.getSpellcheckWords(cb)
calls cb with null or undefined for whatever reason (or in the future). Or do
you think its not necessary to pre-error-prevent?
> 
> I don't think this is necessary to be defensive in that way. The JSDoc at [1],
should be reflecting the implementation, and therefore no null/undefined should
be returned. We are defensive by trusting the JSDoc and the JSCompiler to catch
errors.
> 
> [1]
https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/lan...

Ping ^. Can we remove the check for this.words_? Then you can probably get rid
of hasWords_() helper completely and directly use words_.length in the binding.

https://codereview.chromium.org/2690263002/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/settings/edit_dictionary_page_test.js (right):

https://codereview.chromium.org/2690263002/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:1: // Copyright
2016 The Chromium Authors. All rights reserved.
2017

[scottchen, 2017-02-23 01:00:20]

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

https://codereview.chromium.org/2690263002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:130:
return !!(this.words_ && this.words_.length);
On 2017/02/23 00:32:03, dpapad wrote:
> On 2017/02/22 at 21:58:52, dpapad wrote:
> > On 2017/02/22 at 21:42:25, scottchen wrote:
> > > On 2017/02/15 20:36:37, dpapad wrote:
> > > > words_ is initialized to the empty array. Why do we need to check if it
> exists?
> > > 
> > > I was thinking just in case languageSettingsPrivate.getSpellcheckWords(cb)
> calls cb with null or undefined for whatever reason (or in the future). Or do
> you think its not necessary to pre-error-prevent?
> > 
> > I don't think this is necessary to be defensive in that way. The JSDoc at
[1],
> should be reflecting the implementation, and therefore no null/undefined
should
> be returned. We are defensive by trusting the JSDoc and the JSCompiler to
catch
> errors.
> > 
> > [1]
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/lan...
> 
> Ping ^. Can we remove the check for this.words_? Then you can probably get rid
> of hasWords_() helper completely and directly use words_.length in the
binding.

It appears that I cannot to [[!!boolean]] in template so I think I still need a
middleman function. Removed the null check.

https://codereview.chromium.org/2690263002/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/settings/edit_dictionary_page_test.js (right):

https://codereview.chromium.org/2690263002/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/settings/edit_dictionary_page_test.js:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/02/23 00:32:03, dpapad wrote:
> 2017

Done.

[dpapad, 2017-02-23 01:19:55]

LGTM

https://codereview.chromium.org/2690263002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js
(right):

https://codereview.chromium.org/2690263002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:130:
return !!this.words_.length;
Optional (and arguable) nit: Avoid implicit boolean conversions (it is safe
here, but in general implicit boolean conversions are an invitation for trouble,
from my past experience).

return this.words_length > 0;

[scottchen, 2017-02-23 20:05:39]

https://codereview.chromium.org/2690263002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js
(right):

https://codereview.chromium.org/2690263002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:130:
return !!this.words_.length;
On 2017/02/23 01:19:54, dpapad wrote:
> Optional (and arguable) nit: Avoid implicit boolean conversions (it is safe
> here, but in general implicit boolean conversions are an invitation for
trouble,
> from my past experience).
> 
> return this.words_length > 0;

Done.

[scottchen, 2017-02-23 20:05:54]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-02-23 20:05:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2690263002/#ps180001
(title: "use comparison instead of boolean conversion")

[commit-bot: I haz the power, 2017-02-23 20:06:40]

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, 2017-02-23 21:19:41]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487880354429830,
"parent_rev": "c8d527fa602efdd841ab3d4bfbcec0e7d02a35fd", "commit_rev":
"08b33480b93950a0153c179f5336aeaf382ff8d5"}

[commit-bot: I haz the power, 2017-02-23 21:20:22]

Description was changed from

==========
MD Settings: add empty list message for spell-check edit page.

Also move the word list to a template dom-if, and fix auto-scroll when new words
are added.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: add empty list message for spell-check edit page.

Also move the word list to a template dom-if, and fix auto-scroll when new words
are added.

BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2690263002
Cr-Commit-Position: refs/heads/master@{#452633}
Committed:
https://chromium.googlesource.com/chromium/src/+/08b33480b93950a0153c179f5336...
==========

[commit-bot: I haz the power, 2017-02-23 21:20:23]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/08b33480b93950a0153c179f5336...