[MD Bookmarks] Add toasts.

[MD Bookmarks] Add toasts.

This CL adds a bookmarks-toast-manager element which shows toasts for
the bookmark manager. Toasts currently show for item deletion,
copying urls, and sorting a folder. An undo button may also be shown
in the toast.

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

Review-Url: https://codereview.chromium.org/2898303004
Cr-Commit-Position: refs/heads/master@{#477564}
Committed: https://chromium.googlesource.com/chromium/src/+/efe47735cb33458311b6d71a3ff47ff65f4200e8

[calamity, 2017-05-25 07:31:38]

Description was changed from

==========
[MD Bookmarks] Add toasts.

This CL adds the paper-toast component and uses it in the bookmark
manager through a ToastManager element. Toasts currently show for item
deletion, copying urls, and sorting a folder. An undo button may also be
shown in the toast.

BUG=725786
==========

to

==========
[MD Bookmarks] Add toasts.

This CL adds the paper-toast component and uses it in the bookmark
manager through a ToastManager element. Toasts currently show for item
deletion, copying urls, and sorting a folder. An undo button may also be
shown in the toast.

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

[calamity, 2017-05-25 07:47:57]

Patchset #1 (id:1) has been deleted

[calamity, 2017-05-25 07:48:16]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[tsergeant, 2017-05-26 05:19:25]

Cool! This all works pretty nicely.

https://codereview.chromium.org/2898303004/diff/20001/chrome/app/bookmarks_st...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2898303004/diff/20001/chrome/app/bookmarks_st...
chrome/app/bookmarks_strings.grdp:426: <ph
name="NUMBER_OF_ITEMS_DELETED">$1</ph> bookmarks deleted
Do we need to worry about localization more here? It works for English plural
rules, but that might not work for every language.

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/toast_manager.html (right):

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:15: max-width: 568px;
A couple of additions to match the mocks:

font-size: inherit;
margin: 24px;

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:22: height: 0;
Is this necessary?

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:35: font-size: 13px;
With font-size: inherit above, you won't need to apply this here as well.

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:40: padding: 0;
I think the mocks call for padding: 8px here. 

It makes sense in the case where either the font size is increased, or the
translation for 'undo' is long. When that happens, there's no space around the
text and the ripple pushes up against it badly. Adding in the padding doesn't
change anything for the English case, either.

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js
(right):

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js:11:
Polymer.IronOverlayBehavior
Adding a dependency on IronOverlayBehavior is a bit of a bummer (it's 35KB and
pretty finicky). At some point in the future we might want to consider rolling
our own toast implementation.

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
File third_party/polymer/v1_0/components_summary.txt (left):

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
third_party/polymer/v1_0/components_summary.txt:260: Repository:
https://github.com/PolymerelEments/paper-listbox.git
🤔🤔🤔

[Dan Beam, 2017-05-30 23:25:31]

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

[Dan Beam, 2017-05-30 23:25:31]

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js
(right):

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js:11:
Polymer.IronOverlayBehavior
On 2017/05/26 05:19:24, tsergeant wrote:
> Adding a dependency on IronOverlayBehavior is a bit of a bummer (it's 35KB and
> pretty finicky). At some point in the future we might want to consider rolling
> our own toast implementation.

+1, why do we need this?

[dpapad, 2017-05-30 23:33:38]

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

[dpapad, 2017-05-30 23:33:39]

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js
(right):

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js:11:
Polymer.IronOverlayBehavior
On 2017/05/30 at 23:25:31, Dan Beam wrote:
> On 2017/05/26 05:19:24, tsergeant wrote:
> > Adding a dependency on IronOverlayBehavior is a bit of a bummer (it's 35KB
and
> > pretty finicky). At some point in the future we might want to consider
rolling
> > our own toast implementation.
> 
> +1, why do we need this?

Would it be too hard to roll our own toast now? Instead of punting this for the
future? FWIW, iron-overlay-behavior is what paper-dialog uses internally, which
we've replaced with native <dialog>. In theory one could implement a toast with
a native non-modal <dialog>.

[calamity, 2017-05-31 07:19:01]

Patchset #6 (id:120001) has been deleted

[calamity, 2017-05-31 08:10:42]

🍞🍞🍞

https://codereview.chromium.org/2898303004/diff/20001/chrome/app/bookmarks_st...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2898303004/diff/20001/chrome/app/bookmarks_st...
chrome/app/bookmarks_strings.grdp:426: <ph
name="NUMBER_OF_ITEMS_DELETED">$1</ph> bookmarks deleted
On 2017/05/26 05:19:24, tsergeant wrote:
> Do we need to worry about localization more here? It works for English plural
> rules, but that might not work for every language.

Wrote a plural string handler. Will remove the single string in a follow up.

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/toast_manager.html (right):

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:15: max-width: 568px;
On 2017/05/26 05:19:24, tsergeant wrote:
> A couple of additions to match the mocks:
> 
> font-size: inherit;
> margin: 24px;

Pushed into cr-toast.

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:22: height: 0;
On 2017/05/26 05:19:24, tsergeant wrote:
> Is this necessary?

Whole thing wasn't necessary actually. Removed.

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:35: font-size: 13px;
On 2017/05/26 05:19:24, tsergeant wrote:
> With font-size: inherit above, you won't need to apply this here as well.

Acknowledged.

https://codereview.chromium.org/2898303004/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toast_manager.html:40: padding: 0;
On 2017/05/26 05:19:24, tsergeant wrote:
> I think the mocks call for padding: 8px here. 
> 
> It makes sense in the case where either the font size is increased, or the
> translation for 'undo' is long. When that happens, there's no space around the
> text and the ripple pushes up against it badly. Adding in the padding doesn't
> change anything for the English case, either.

Done.

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js
(right):

https://codereview.chromium.org/2898303004/diff/20001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-toast/paper-toast-extracted.js:11:
Polymer.IronOverlayBehavior
On 2017/05/30 23:33:39, dpapad wrote:
> On 2017/05/30 at 23:25:31, Dan Beam wrote:
> > On 2017/05/26 05:19:24, tsergeant wrote:
> > > Adding a dependency on IronOverlayBehavior is a bit of a bummer (it's 35KB
> and
> > > pretty finicky). At some point in the future we might want to consider
> rolling
> > > our own toast implementation.
> > 
> > +1, why do we need this?
> 
> Would it be too hard to roll our own toast now? Instead of punting this for
the
> future? FWIW, iron-overlay-behavior is what paper-dialog uses internally,
which
> we've replaced with native <dialog>. In theory one could implement a toast
with
> a native non-modal <dialog>.

Rolled.

[dpapad, 2017-05-31 17:52:38]

https://codereview.chromium.org/2898303004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast_manager.js (right):

https://codereview.chromium.org/2898303004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.js:18: attached: function()
{
@override

https://codereview.chromium.org/2898303004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.js:49: /** @private
{bookmarks.ToastManager} */
?bookmarks.ToastManager

https://codereview.chromium.org/2898303004/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements_resources.grdp (right):

https://codereview.chromium.org/2898303004/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements_resources.grdp:156:
file="../../webui/resources/cr_elements/cr_toast/cr_toast.html"
Are those files supposed to be added in the same CL? I could not find another CL
already adding them.

Generally, when adding new elements, we've been adding them where their are
being used (chrome/browser/resources/md_bookmarks) first, and only promote them
to
ui/webui/resources/cr_elements/ once the need to use those elements from a 2nd
WebUI page occurs.

The main benefit of this approach is that 
 - we don't end up with cr_elements that are only used by a single page
indefinitely 
 - cr_elements/ proven that they are "re-usable" since they have 2+ clients.

For example, if someone where to re-use cr_toast.html, might find that
toast_manager.html is also useful/needed, and therefore it would (maybe) make
sense to also put toast_manager.html in cr_elements. This decision is easier to
make once 2nd usage of "toast" is identified.

[calamity, 2017-06-01 06:40:08]

https://codereview.chromium.org/2898303004/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast_manager.js (right):

https://codereview.chromium.org/2898303004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.js:18: attached: function()
{
On 2017/05/31 17:52:38, dpapad wrote:
> @override

Done.

https://codereview.chromium.org/2898303004/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.js:49: /** @private
{bookmarks.ToastManager} */
On 2017/05/31 17:52:38, dpapad wrote:
> ?bookmarks.ToastManager

Done.

https://codereview.chromium.org/2898303004/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements_resources.grdp (right):

https://codereview.chromium.org/2898303004/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements_resources.grdp:156:
file="../../webui/resources/cr_elements/cr_toast/cr_toast.html"
On 2017/05/31 17:52:38, dpapad wrote:
> Are those files supposed to be added in the same CL? I could not find another
CL
> already adding them.
> 
> Generally, when adding new elements, we've been adding them where their are
> being used (chrome/browser/resources/md_bookmarks) first, and only promote
them
> to
> ui/webui/resources/cr_elements/ once the need to use those elements from a 2nd
> WebUI page occurs.
> 
> The main benefit of this approach is that 
>  - we don't end up with cr_elements that are only used by a single page
> indefinitely 
>  - cr_elements/ proven that they are "re-usable" since they have 2+ clients.
> 
> For example, if someone where to re-use cr_toast.html, might find that
> toast_manager.html is also useful/needed, and therefore it would (maybe) make
> sense to also put toast_manager.html in cr_elements. This decision is easier
to
> make once 2nd usage of "toast" is identified.

Oops, forgot to git add. I've moved the toast to md_bookmarks/ (and actually
added it this time).

[tsergeant, 2017-06-01 08:24:59]

You should update the CL description, which still mentions paper-toast.

https://codereview.chromium.org/2898303004/diff/180001/chrome/app/bookmarks_s...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/app/bookmarks_s...
chrome/app/bookmarks_strings.grdp:422: <message
name="IDS_MD_BOOKMARK_MANAGER_TOAST_FOLDER_SORTED" desc="Label displayed in
toast when a folder's children are sorted.">
maybe say "toast popup message" instead of just "toast" these descriptions to
help out the translators. Toast is a pretty weird word if you don't know its
meaning in this context.

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast.html (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.html:3: <dom-module
id="bookmarks-toast">
To be honest, I don't really see the need to make a distinction between
bookmarks-toast and bookmarks-toast-manager anymore, except for the fact that
*maybe* one day we will want to share one but not the other.

<bookmarks-toast> is simple enough that I think we should just implement it in
<bookmarks-toast-manager> directly.

Sorry for causing even more churn on this CL.

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.html:8: background-color:
var(--bookmarks-toast-background-color, #323232);
Relatedly, I don't think we need to have these unused CSS vars lying around

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/plural_string_handler.cc (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/plural_string_handler.cc:40: DCHECK(name_to_id_.end() !=
message_id_it);
Should this be a CHECK instead of a DCHECK? What gets returned to JS if this
happens? Surely passing a nonexistent message id is just as bad as passing no
message id at all.

[dpapad, 2017-06-01 17:35:44]

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast.js (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.js:16: duration: Number,
Nit: Can we give this an initial value of 0? This way we don't assign any
semantic meaning to "undefined".

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.js:24: setTimeout(function() {
Shouldn't we be canceling this timeout to avoid race conditions? Example

toast.open = true -> triggers setTimeout()
toast.open = false -> openChanged_() returns early, but previous timeout keeps
going.
toast.open = true -> triggers 2nd setTimeout(). The toast will be closed when
1st timeout expires though, not when the 2nd timeout expires.

https://codereview.chromium.org/2898303004/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/toast_manager_test.js (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/toast_manager_test.js:14: test('simple
show/hide', function() {
The test claims to test show/hide, but only toastManager.show() is called. Can
we add additional assertions for hide?

[calamity, 2017-06-02 01:51:21]

Description was changed from

==========
[MD Bookmarks] Add toasts.

This CL adds the paper-toast component and uses it in the bookmark
manager through a ToastManager element. Toasts currently show for item
deletion, copying urls, and sorting a folder. An undo button may also be
shown in the toast.

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

to

==========
[MD Bookmarks] Add toasts.

This CL adds the bookmarks-toast element and uses it in the bookmark
manager through a ToastManager element. Toasts currently show for item
deletion, copying urls, and sorting a folder. An undo button may also be
shown in the toast.

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

[calamity, 2017-06-02 05:32:57]

Patchset #9 (id:200001) has been deleted

[calamity, 2017-06-02 05:34:06]

Description was changed from

==========
[MD Bookmarks] Add toasts.

This CL adds the bookmarks-toast element and uses it in the bookmark
manager through a ToastManager element. Toasts currently show for item
deletion, copying urls, and sorting a folder. An undo button may also be
shown in the toast.

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

to

==========
[MD Bookmarks] Add toasts.

This CL adds a bookmarks-toast-manager element which shows toasts for
the bookmark manager. Toasts currently show for item deletion,
copying urls, and sorting a folder. An undo button may also be shown
in the toast.

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

[calamity, 2017-06-02 05:34:22]

https://codereview.chromium.org/2898303004/diff/180001/chrome/app/bookmarks_s...
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/app/bookmarks_s...
chrome/app/bookmarks_strings.grdp:422: <message
name="IDS_MD_BOOKMARK_MANAGER_TOAST_FOLDER_SORTED" desc="Label displayed in
toast when a folder's children are sorted.">
On 2017/06/01 08:25:00, tsergeant wrote:
> maybe say "toast popup message" instead of just "toast" these descriptions to
> help out the translators. Toast is a pretty weird word if you don't know its
> meaning in this context.

Done.

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast.html (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.html:3: <dom-module
id="bookmarks-toast">
On 2017/06/01 08:25:00, tsergeant wrote:
> To be honest, I don't really see the need to make a distinction between
> bookmarks-toast and bookmarks-toast-manager anymore, except for the fact that
> *maybe* one day we will want to share one but not the other.
> 
> <bookmarks-toast> is simple enough that I think we should just implement it in
> <bookmarks-toast-manager> directly.
> 
> Sorry for causing even more churn on this CL.

Done.

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.html:8: background-color:
var(--bookmarks-toast-background-color, #323232);
On 2017/06/01 08:25:00, tsergeant wrote:
> Relatedly, I don't think we need to have these unused CSS vars lying around

Done.

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast.js (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.js:16: duration: Number,
On 2017/06/01 17:35:44, dpapad wrote:
> Nit: Can we give this an initial value of 0? This way we don't assign any
> semantic meaning to "undefined".

Done.

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast.js:24: setTimeout(function() {
On 2017/06/01 17:35:44, dpapad wrote:
> Shouldn't we be canceling this timeout to avoid race conditions? Example
> 
> toast.open = true -> triggers setTimeout()
> toast.open = false -> openChanged_() returns early, but previous timeout keeps
> going.
> toast.open = true -> triggers 2nd setTimeout(). The toast will be closed when
> 1st timeout expires though, not when the 2nd timeout expires.

Good catch!

Done.

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/plural_string_handler.cc (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/plural_string_handler.cc:40: DCHECK(name_to_id_.end() !=
message_id_it);
On 2017/06/01 08:25:00, tsergeant wrote:
> Should this be a CHECK instead of a DCHECK? What gets returned to JS if this
> happens? Surely passing a nonexistent message id is just as bad as passing no
> message id at all.

Done.

https://codereview.chromium.org/2898303004/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/toast_manager_test.js (right):

https://codereview.chromium.org/2898303004/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/toast_manager_test.js:14: test('simple
show/hide', function() {
On 2017/06/01 17:35:44, dpapad wrote:
> The test claims to test show/hide, but only toastManager.show() is called. Can
> we add additional assertions for hide?

Done.

[tsergeant, 2017-06-05 01:25:40]

https://codereview.chromium.org/2898303004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast_manager.html (right):

https://codereview.chromium.org/2898303004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.html:52: <div id="toast">
paper-toast integrates with iron-a11y-announcer to make their toasts accessible,
so that the screenreader announces the toast text when it is shown.

I think that adding aria-live="polite" either here on #toast or on #content
would probably achieve much the same thing, although you could also use
iron-a11y-announcer if that's better.

[dpapad, 2017-06-06 01:33:41]

https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/toast_manager_test.js (right):

https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/toast_manager_test.js:33:
toastManager.setTimeout_ = function(f) {
Is there an alternative way to test this functionality without replacing private
methods (and in general recferring to private properties of toastManager)? How
about the following?

1) change toastManager.show() to return the #toast div.
2) call toastMnaager.show() in this test.
3) Wait for duration + x (say 100ms + 10ms).
4) Call window.getComputedStyle(#toast div), assert that it is hidden (display
hidden).

The gist of the above approach is to test the observed behavior not this
particular implementation.

https://codereview.chromium.org/2898303004/diff/240001/third_party/polymer/v1...
File third_party/polymer/v1_0/bower.json (left):

https://codereview.chromium.org/2898303004/diff/240001/third_party/polymer/v1...
third_party/polymer/v1_0/bower.json:48: "paper-listbox":
"PolymerelEments/paper-listbox#1.1.2",
Can you move the change to bower.json and components_summary.txt to a separate
CL? It made more sense to fix those when this CL was touching those files anyway
(because of adding paper-toast dependency), but that is no longer the case.

[Dan Beam, 2017-06-06 01:50:13]

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

[calamity, 2017-06-06 04:44:05]

https://codereview.chromium.org/2898303004/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast_manager.html (right):

https://codereview.chromium.org/2898303004/diff/220001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.html:52: <div id="toast">
On 2017/06/05 01:25:40, tsergeant wrote:
> paper-toast integrates with iron-a11y-announcer to make their toasts
accessible,
> so that the screenreader announces the toast text when it is shown.
> 
> I think that adding aria-live="polite" either here on #toast or on #content
> would probably achieve much the same thing, although you could also use
> iron-a11y-announcer if that's better.

Done.

https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/toast_manager_test.js (right):

https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/toast_manager_test.js:33:
toastManager.setTimeout_ = function(f) {
On 2017/06/06 01:33:41, dpapad wrote:
> Is there an alternative way to test this functionality without replacing
private
> methods (and in general recferring to private properties of toastManager)? How
> about the following?
> 
> 1) change toastManager.show() to return the #toast div.
> 2) call toastMnaager.show() in this test.
> 3) Wait for duration + x (say 100ms + 10ms).
> 4) Call window.getComputedStyle(#toast div), assert that it is hidden (display
> hidden).
> 
> The gist of the above approach is to test the observed behavior not this
> particular implementation.

I'm extremely wary of adding sleeps to tests. This causes unnecessary and costly
test slowdown (100ms per show() call for all browser test runs forever) and is a
very common vector for flakes. Mocking out a fake setTimeout and clearTimeout is
a compromise between encapsulation and testability. We are able to assert
precise timing behaviors because we can mock out the sequence of events (e.g the
3rd block where a second show() is called before the first ends). These are
worth testing, and can't reliably be done when timers are introduced.

Testing for the computed style here is also more volatile, as the styles are
more likely to change than the behavior of the open_ property. I think ensuring
that the reason that the styles are applied (the state of open_) is a more
robust test of observed behavior than the particular CSS implementation.

I think that black box testing with intricate internals that are only subtly
expressed via public properties can miss a lot of cases, and exposing these
internals for the sake of testing would lead to a bloated public API. The way
we've been writing tests has aimed to balance clean public APIs and relatively
clean internal implementations, without compromising testability.

https://codereview.chromium.org/2898303004/diff/240001/third_party/polymer/v1...
File third_party/polymer/v1_0/bower.json (left):

https://codereview.chromium.org/2898303004/diff/240001/third_party/polymer/v1...
third_party/polymer/v1_0/bower.json:48: "paper-listbox":
"PolymerelEments/paper-listbox#1.1.2",
On 2017/06/06 01:33:41, dpapad wrote:
> Can you move the change to bower.json and components_summary.txt to a separate
> CL? It made more sense to fix those when this CL was touching those files
anyway
> (because of adding paper-toast dependency), but that is no longer the case.

Done.

[calamity, 2017-06-06 05:33:16]

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

[commit-bot: I haz the power, 2017-06-06 05:33:29]

Dry run: 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-06-06 06:22:43]

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

[commit-bot: I haz the power, 2017-06-06 06:22:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tsergeant, 2017-06-06 07:25:14]

Generally lg, but the CommandManager tests are failing because they need a
ToastManager to exist on the page.

[calamity, 2017-06-06 07:27:58]

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

[commit-bot: I haz the power, 2017-06-06 07:28:15]

Dry run: 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-06-06 09:53:43]

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

[commit-bot: I haz the power, 2017-06-06 09:53:44]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-06-06 18:18:41]

LGTM, with optional testing related suggestion/comment.

https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/toast_manager_test.js (right):

https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/toast_manager_test.js:33:
toastManager.setTimeout_ = function(f) {
> I'm extremely wary of adding sleeps to tests. This causes unnecessary and
costly test slowdown (100ms per show() call for all browser test runs forever)
and is a very common vector for flakes. Mocking out a fake setTimeout and
clearTimeout is a compromise between encapsulation and testability. We are able
to assert precise timing behaviors because we can mock out the sequence of
events (e.g the 3rd block where a second show() is called before the first
ends). These are worth testing, and can't reliably be done when timers are
introduced.
> 
> Testing for the computed style here is also more volatile, as the styles are
more likely to change than the behavior of the open_ property. I think ensuring
that the reason that the styles are applied (the state of open_) is a more
robust test of observed behavior than the particular CSS implementation.
> 
> I think that black box testing with intricate internals that are only subtly
expressed via public properties can miss a lot of cases, and exposing these
internals for the sake of testing would lead to a bloated public API. The way
we've been writing tests has aimed to balance clean public APIs and relatively
clean internal implementations, without compromising testability.

I only mentioned 100ms as an example. You could even specify as 0 or 1 and still
be able to test the code without slowing down the test.

Having said that I understand the argument about encapsulation and testability,
but I do think that current approach leans heavily on the encapsulation side of
the trade-off. Test-friendly code is written in a way that makes it easier to
test. In other words, it is OK to reasonably tweak the prod code to allow easier
testing. Current approach does not take tests into account, and relies on the
"bad parts" of JS, where there is no real "private" state enforcement.

So, here is an alternative approach (that still allows you to test the case of
calling show() twice):

In toast_manager.js, group the timing related functions to |timerApi_| object
and add a way to override it.

timerApi_: {
  clearTimeout: window.clearTimeout.bind(window),
  setTimeout: window.setTimeout.bind(window),
},

// Used only in tests.
setTimerApi: function(timerApi) { this.timerApi_ = timerApi; },

in toast_manager_test.js pass your own fake timerApi that does exactly what you
need for testing.

toastManager.setTimerApi({
  ...
});

That seems a better trade-off IMO. In my experience with JS tests, there is
almost always a reasonable way to avoid referring to private member variables
and still be able to test all code paths/cases. Consider my suggestion optional,
since I know that some aspects of what is considered "reasonable tweaks" are
subjective.

[calamity, 2017-06-07 04:35:49]

On 2017/06/06 18:18:41, dpapad wrote:
> LGTM, with optional testing related suggestion/comment.
> 
>
https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
> File chrome/test/data/webui/md_bookmarks/toast_manager_test.js (right):
> 
>
https://codereview.chromium.org/2898303004/diff/240001/chrome/test/data/webui...
> chrome/test/data/webui/md_bookmarks/toast_manager_test.js:33:
> toastManager.setTimeout_ = function(f) {
> > I'm extremely wary of adding sleeps to tests. This causes unnecessary and
> costly test slowdown (100ms per show() call for all browser test runs forever)
> and is a very common vector for flakes. Mocking out a fake setTimeout and
> clearTimeout is a compromise between encapsulation and testability. We are
able
> to assert precise timing behaviors because we can mock out the sequence of
> events (e.g the 3rd block where a second show() is called before the first
> ends). These are worth testing, and can't reliably be done when timers are
> introduced.
> > 
> > Testing for the computed style here is also more volatile, as the styles are
> more likely to change than the behavior of the open_ property. I think
ensuring
> that the reason that the styles are applied (the state of open_) is a more
> robust test of observed behavior than the particular CSS implementation.
> > 
> > I think that black box testing with intricate internals that are only subtly
> expressed via public properties can miss a lot of cases, and exposing these
> internals for the sake of testing would lead to a bloated public API. The way
> we've been writing tests has aimed to balance clean public APIs and relatively
> clean internal implementations, without compromising testability.
> 
> I only mentioned 100ms as an example. You could even specify as 0 or 1 and
still
> be able to test the code without slowing down the test.
> 
> Having said that I understand the argument about encapsulation and
testability,
> but I do think that current approach leans heavily on the encapsulation side
of
> the trade-off. Test-friendly code is written in a way that makes it easier to
> test. In other words, it is OK to reasonably tweak the prod code to allow
easier
> testing. Current approach does not take tests into account, and relies on the
> "bad parts" of JS, where there is no real "private" state enforcement.
> 
> So, here is an alternative approach (that still allows you to test the case of
> calling show() twice):
> 
> In toast_manager.js, group the timing related functions to |timerApi_| object
> and add a way to override it.
> 
> timerApi_: {
>   clearTimeout: window.clearTimeout.bind(window),
>   setTimeout: window.setTimeout.bind(window),
> },
> 
> // Used only in tests.
> setTimerApi: function(timerApi) { this.timerApi_ = timerApi; },
> 
> in toast_manager_test.js pass your own fake timerApi that does exactly what
you
> need for testing.
> 
> toastManager.setTimerApi({
>   ...
> });
> 
> That seems a better trade-off IMO. In my experience with JS tests, there is
> almost always a reasonable way to avoid referring to private member variables
> and still be able to test all code paths/cases. Consider my suggestion
optional,
> since I know that some aspects of what is considered "reasonable tweaks" are
> subjective.

This seems reasonable. Since we use a similar thing in the drag and drop tests,
it makes a lot of sense to factor this out. I'll do it in a follow-up, since
this patch is already horribly bloated.

[tsergeant, 2017-06-07 04:42:57]

lgtm, with two more little nits

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast_manager.html (right):

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.html:57: </divn>
Nit: Typo here

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/plural_string_handler.h (right):

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/plural_string_handler.h:8: #include
"base/callback_forward.h"
Nit: Is this needed? There are no callbacks in this file.

[calamity, 2017-06-07 04:59:06]

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

[commit-bot: I haz the power, 2017-06-07 04:59:18]

Dry run: CQ is trying da patch.

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

[calamity, 2017-06-07 05:51:40]

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/md_bookmarks/toast_manager.html (right):

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/resourc...
chrome/browser/resources/md_bookmarks/toast_manager.html:57: </divn>
On 2017/06/07 04:42:57, tsergeant wrote:
> Nit: Typo here

Done.

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/plural_string_handler.h (right):

https://codereview.chromium.org/2898303004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/plural_string_handler.h:8: #include
"base/callback_forward.h"
On 2017/06/07 04:42:57, tsergeant wrote:
> Nit: Is this needed? There are no callbacks in this file.

Removed.

[commit-bot: I haz the power, 2017-06-07 06:28:50]

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

[commit-bot: I haz the power, 2017-06-07 06:28:51]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-06-07 06:40:35]

The CQ bit was checked by calamity@chromium.org

[calamity, 2017-06-07 06:40:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2898303004/#ps320001
(title: "fix nits")

[commit-bot: I haz the power, 2017-06-07 06:40:56]

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-06-07 06:45:00]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1496817635403640,
"parent_rev": "121fec7bb384638df1cc3e26fda0bed886970a38", "commit_rev":
"efe47735cb33458311b6d71a3ff47ff65f4200e8"}

[commit-bot: I haz the power, 2017-06-07 06:45:10]

Description was changed from

==========
[MD Bookmarks] Add toasts.

This CL adds a bookmarks-toast-manager element which shows toasts for
the bookmark manager. Toasts currently show for item deletion,
copying urls, and sorting a folder. An undo button may also be shown
in the toast.

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

to

==========
[MD Bookmarks] Add toasts.

This CL adds a bookmarks-toast-manager element which shows toasts for
the bookmark manager. Toasts currently show for item deletion,
copying urls, and sorting a folder. An undo button may also be shown
in the toast.

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

Review-Url: https://codereview.chromium.org/2898303004
Cr-Commit-Position: refs/heads/master@{#477564}
Committed:
https://chromium.googlesource.com/chromium/src/+/efe47735cb33458311b6d71a3ff4...
==========

[commit-bot: I haz the power, 2017-06-07 06:45:12]

Committed patchset #14 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/efe47735cb33458311b6d71a3ff4...