chromeos: Make filesystem mount notifications clickable.

chromeos: Make filesystem mount notifications clickable.

Make the bodies of the filesystem mount and import
notifications clickable. Currently, clicking them dismisses
them without doing anything.

Also unify some duplicate code.

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

Review-Url: https://codereview.chromium.org/2645353005
Cr-Commit-Position: refs/heads/master@{#446477}
Committed: https://chromium.googlesource.com/chromium/src/+/0ec4093bbdec65e8fdc600a6b84c1e2a8c186fdc

[Daniel Erat, 2017-01-24 22:38:24]

Description was changed from

==========
chromeos: Make filesystem mount notifications clickable.

Make the bodies of the filesystem mount and import
notifications clickable. Currently, clicking them dismisses
them without doing anything.

Also unify some duplicate code.

BUG=684095
==========

to

==========
chromeos: Make filesystem mount notifications clickable.

Make the bodies of the filesystem mount and import
notifications clickable. Currently, clicking them dismisses
them without doing anything.

Also unify some duplicate code.

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

[Daniel Erat, 2017-01-24 22:38:38]

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

[commit-bot: I haz the power, 2017-01-24 22:39:27]

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

[Daniel Erat, 2017-01-24 22:39:40]

derat@chromium.org changed reviewers:
+ fukino@chromium.org, oka@chromium.org, yamaguchi@chromium.org

[Daniel Erat, 2017-01-24 22:39:40]

this is my first time looking at this code, so let me know if i'm doing anything
wrong. i'm also not sure how to run JS unit tests locally. :-/

[commit-bot: I haz the power, 2017-01-24 22:57:19]

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

[commit-bot: I haz the power, 2017-01-24 22:57:20]

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

[Daniel Erat, 2017-01-24 23:32:34]

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

[commit-bot: I haz the power, 2017-01-24 23:34:03]

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-01-25 01:24:23]

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

[commit-bot: I haz the power, 2017-01-25 01:24:24]

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

[Daniel Erat, 2017-01-25 02:13:44]

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

[Daniel Erat, 2017-01-25 02:14:16]

On 2017/01/24 22:39:40, Daniel Erat wrote:
> this is my first time looking at this code, so let me know if i'm doing
anything
> wrong. i'm also not sure how to run JS unit tests locally. :-/

i'd also like to add tests for my change, but i don't see any using
dispatchEvent() yet. i'll poke around more tomorrow...

[commit-bot: I haz the power, 2017-01-25 02:14:37]

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-01-25 02:49:57]

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

[commit-bot: I haz the power, 2017-01-25 02:49:58]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-01-25 05:51:24]

Thank you for fixing this!
lgtm so far.
If you are adding unit test, you can run the test by
out/Release/browser_tests --gtest_filter=FileManagerJsTest.DeviceHandlerTest

[Daniel Erat, 2017-01-26 00:39:43]

thanks, i've added a test for my change.

i don't have any experience with javascript tests, but i'm wondering if
something is wrong with this file's setup. when i intentionally break an
assertion within a promise (in an already-existing test), i see a failure get
logged, but browser_tests still exits with success:

----

% out/Debug/browser_tests --gtest_filter=FileManagerJsTest.DeviceHandlerTest
...
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FileManagerJsTest, where TypeParam = 
[ RUN      ] FileManagerJsTest.DeviceHandlerTest
...
[30268:30268:0125/172504.123060:INFO:CONSOLE(18)] "Uncaught (in promise) Error:
Assertion Failed
  Observed: blabbity
  Expected: blabbity-", source:  (18)
[30268:30268:0125/172504.720121:WARNING:url_request_context_getter.cc(43)]
URLRequestContextGetter leaking due to no owning thread.
[30268:30268:0125/172504.720251:WARNING:url_request_context_getter.cc(43)]
URLRequestContextGetter leaking due to no owning thread.
[       OK ] FileManagerJsTest.DeviceHandlerTest (2301 ms)
[----------] 1 test from FileManagerJsTest (2301 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (2301 ms total)
[  PASSED  ] 1 test.
[1/1] FileManagerJsTest.DeviceHandlerTest (2882 ms)
SUCCESS: all tests passed.

----

do we need to do something differently to make these failed assertions cause a
browser_tests failure? reportPromise() looks like it's defined in
ui/file_manager/file_manager/common/js/unittest_util.js. i'm not sure which
callback is getting passed into all of these test* functions, though.

[Daniel Erat, 2017-01-26 00:39:50]

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

[commit-bot: I haz the power, 2017-01-26 00:40:36]

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-01-26 01:31:33]

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

[commit-bot: I haz the power, 2017-01-26 01:31:34]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-01-26 06:07:55]

On 2017/01/26 00:39:43, Daniel Erat wrote:
> thanks, i've added a test for my change.
> 
> i don't have any experience with javascript tests, but i'm wondering if
> something is wrong with this file's setup. when i intentionally break an
> assertion within a promise (in an already-existing test), i see a failure get
> logged, but browser_tests still exits with success:
> 
> ----
> 
> % out/Debug/browser_tests --gtest_filter=FileManagerJsTest.DeviceHandlerTest
> ...
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from FileManagerJsTest, where TypeParam = 
> [ RUN      ] FileManagerJsTest.DeviceHandlerTest
> ...
> [30268:30268:0125/172504.123060:INFO:CONSOLE(18)] "Uncaught (in promise)
Error:
> Assertion Failed
>   Observed: blabbity
>   Expected: blabbity-", source:  (18)
> [30268:30268:0125/172504.720121:WARNING:url_request_context_getter.cc(43)]
> URLRequestContextGetter leaking due to no owning thread.
> [30268:30268:0125/172504.720251:WARNING:url_request_context_getter.cc(43)]
> URLRequestContextGetter leaking due to no owning thread.
> [       OK ] FileManagerJsTest.DeviceHandlerTest (2301 ms)
> [----------] 1 test from FileManagerJsTest (2301 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (2301 ms total)
> [  PASSED  ] 1 test.
> [1/1] FileManagerJsTest.DeviceHandlerTest (2882 ms)
> SUCCESS: all tests passed.
> 
> ----
> 
> do we need to do something differently to make these failed assertions cause a
> browser_tests failure? reportPromise() looks like it's defined in
> ui/file_manager/file_manager/common/js/unittest_util.js. i'm not sure which
> callback is getting passed into all of these test* functions, though.

It sounds weird. Failing assertion in a promise should cause the browser_tests's
failure.
It might be because you call reportPromise twice in one test case?
If one of the promises which is passed to reportPromise() is resolved first, the
test will be reported as SUCCESS even if the later promise is rejected.

[Daniel Erat, 2017-01-26 16:45:42]

On 2017/01/26 06:07:55, fukino wrote:
> On 2017/01/26 00:39:43, Daniel Erat wrote:
> > thanks, i've added a test for my change.
> > 
> > i don't have any experience with javascript tests, but i'm wondering if
> > something is wrong with this file's setup. when i intentionally break an
> > assertion within a promise (in an already-existing test), i see a failure
get
> > logged, but browser_tests still exits with success:
> > 
> > ----
> > 
> > % out/Debug/browser_tests --gtest_filter=FileManagerJsTest.DeviceHandlerTest
> > ...
> > [==========] Running 1 test from 1 test case.
> > [----------] Global test environment set-up.
> > [----------] 1 test from FileManagerJsTest, where TypeParam = 
> > [ RUN      ] FileManagerJsTest.DeviceHandlerTest
> > ...
> > [30268:30268:0125/172504.123060:INFO:CONSOLE(18)] "Uncaught (in promise)
> Error:
> > Assertion Failed
> >   Observed: blabbity
> >   Expected: blabbity-", source:  (18)
> > [30268:30268:0125/172504.720121:WARNING:url_request_context_getter.cc(43)]
> > URLRequestContextGetter leaking due to no owning thread.
> > [30268:30268:0125/172504.720251:WARNING:url_request_context_getter.cc(43)]
> > URLRequestContextGetter leaking due to no owning thread.
> > [       OK ] FileManagerJsTest.DeviceHandlerTest (2301 ms)
> > [----------] 1 test from FileManagerJsTest (2301 ms total)
> > 
> > [----------] Global test environment tear-down
> > [==========] 1 test from 1 test case ran. (2301 ms total)
> > [  PASSED  ] 1 test.
> > [1/1] FileManagerJsTest.DeviceHandlerTest (2882 ms)
> > SUCCESS: all tests passed.
> > 
> > ----
> > 
> > do we need to do something differently to make these failed assertions cause
a
> > browser_tests failure? reportPromise() looks like it's defined in
> > ui/file_manager/file_manager/common/js/unittest_util.js. i'm not sure which
> > callback is getting passed into all of these test* functions, though.
> 
> It sounds weird. Failing assertion in a promise should cause the
browser_tests's
> failure.
> It might be because you call reportPromise twice in one test case?
> If one of the promises which is passed to reportPromise() is resolved first,
the
> test will be reported as SUCCESS even if the later promise is rejected.

i'll double-check, but i think the test that i tried this with only calls
reportPromise once (i was also concerned about calling reportPromise twice). :-)

[Daniel Erat, 2017-01-26 21:22:27]

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

[Daniel Erat, 2017-01-26 21:22:56]

i've sent https://codereview.chromium.org/2650403005/ to fix what i think is the
issue with test failures not being reported correctly.

[commit-bot: I haz the power, 2017-01-26 21:23:03]

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-01-26 21:59:21]

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

[commit-bot: I haz the power, 2017-01-26 21:59:22]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-01-26 22:35:47]

On 2017/01/26 21:22:56, Daniel Erat wrote:
> i've sent https://codereview.chromium.org/2650403005/ to fix what i think is
the
> issue with test failures not being reported correctly.

Thank you for fixing the existing issue!
lgtm for patch set 5.

[Daniel Erat, 2017-01-26 22:39:59]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2017-01-26 22:41:01]

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-01-26 22:49:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485470399190400,
"parent_rev": "e937aad8f28e9973a1fd947833f76a53a207ec97", "commit_rev":
"0ec4093bbdec65e8fdc600a6b84c1e2a8c186fdc"}

[commit-bot: I haz the power, 2017-01-26 22:50:21]

Description was changed from

==========
chromeos: Make filesystem mount notifications clickable.

Make the bodies of the filesystem mount and import
notifications clickable. Currently, clicking them dismisses
them without doing anything.

Also unify some duplicate code.

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

to

==========
chromeos: Make filesystem mount notifications clickable.

Make the bodies of the filesystem mount and import
notifications clickable. Currently, clicking them dismisses
them without doing anything.

Also unify some duplicate code.

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

Review-Url: https://codereview.chromium.org/2645353005
Cr-Commit-Position: refs/heads/master@{#446477}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ec4093bbdec65e8fdc600a6b84c...
==========

[commit-bot: I haz the power, 2017-01-26 22:50:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0ec4093bbdec65e8fdc600a6b84c...