ServiceWorker: mojofy ResumeAfterDownload and AddMessageToConsole

ServiceWorker: mojofy ResumeAfterDownload and AddMessageToConsole

This patch mojofies the following two messages:
 - EmbeddedWorkerMsg_ResumeAfterDownload
 - EmbeddedWorkerMsg_AddMessageToConsole

EmbeddedWorkerDispatcher still holds the WorkerWrapper for StopWorker.
I'm planning to remove EWDispatcher when removing the StopWorker message.

BUG=629701
Review-Url: https://codereview.chromium.org/2630273002
Cr-Commit-Position: refs/heads/master@{#444296}
Committed: https://chromium.googlesource.com/chromium/src/+/5d804c8487702e7a56e7b6f8370a0d10a756df6c

[shimazu, 2017-01-16 09:25:31]

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

[commit-bot: I haz the power, 2017-01-16 09:25:41]

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-16 09:27:11]

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

[commit-bot: I haz the power, 2017-01-16 09:27:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shimazu, 2017-01-16 09:27:20]

shimazu@chromium.org changed reviewers:
+ horo@chromium.org

[shimazu, 2017-01-16 09:27:21]

PTAL ٩(●˙▿˙●)۶

[horo, 2017-01-16 11:24:12]

lgtm

https://codereview.chromium.org/2630273002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/ConsoleMessageTraits.cpp (right):

https://codereview.chromium.org/2630273002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/ConsoleMessageTraits.cpp:36: // static
nit: line break.

[shimazu, 2017-01-16 23:17:52]

shimazu@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[shimazu, 2017-01-16 23:17:53]

dcheng@chromium.org: Please review changes in messages.h and mojom file

haraken@chromium.org: Please review changes in third_party/WebKit

https://codereview.chromium.org/2630273002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/ConsoleMessageTraits.cpp (right):

https://codereview.chromium.org/2630273002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/ConsoleMessageTraits.cpp:36: // static
On 2017/01/16 11:24:12, horo wrote:
> nit: line break.

Done.

[haraken, 2017-01-16 23:48:06]

(mojom changes in WebKit/public/ need to be reviewed by dcheng@.)

https://codereview.chromium.org/2630273002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/console_message.mojom (right):

https://codereview.chromium.org/2630273002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/console_message.mojom:13: Last = Info

Nit: Shall we sort by the values?

[shimazu, 2017-01-17 01:22:04]

https://codereview.chromium.org/2630273002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/console_message.mojom (right):

https://codereview.chromium.org/2630273002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/console_message.mojom:13: Last = Info
On 2017/01/16 23:48:06, haraken wrote:
> 
> Nit: Shall we sort by the values?

It's the same order with the definition:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebConsole...

It looks sorted by the importance, and using the same order is also reasonable
for me. WDYT? (In this case, I should add a comment to mention it.)
Is it better to sort by the values?

[haraken, 2017-01-17 01:23:00]

On 2017/01/17 01:22:04, shimazu wrote:
>
https://codereview.chromium.org/2630273002/diff/40001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/console_message.mojom (right):
> 
>
https://codereview.chromium.org/2630273002/diff/40001/third_party/WebKit/publ...
> third_party/WebKit/public/web/console_message.mojom:13: Last = Info
> On 2017/01/16 23:48:06, haraken wrote:
> > 
> > Nit: Shall we sort by the values?
> 
> It's the same order with the definition:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebConsole...
> 
> It looks sorted by the importance, and using the same order is also reasonable
> for me. WDYT? (In this case, I should add a comment to mention it.)
> Is it better to sort by the values?

Fair enough.

[dcheng, 2017-01-17 23:08:22]

mojo lgtm with nit

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/BUILD.gn:753: "web/ConsoleMessageTraits.cpp",
Nit: let's still call these files StructTraits (the OWNERS rules that make sure
these files are reviewed by security owners depends on this naming)

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/console_message.mojom (right):

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/console_message.mojom:15: Last = Info
Nit: we can omit Last for mojo

[falken, 2017-01-18 01:12:06]

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

[falken, 2017-01-18 01:12:07]

drive-by comments

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:532: void
ResumeAfterDownload() override { NOTREACHED(); }
Can you avoid the NOTREACHED by keeping state and checking it in the test as an
EXPECT()?

From style guide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
"Don't use these macros in tests, as they crash the test binary and leave bots
in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which
report failures gracefully and can continue running other tests."

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:107:
NOTIMPLEMENTED();
Do we need a TODO here? Why is it not implemented?

If it does nothing by design, I'd have a comment saying that instead of
NOTIMPLEMENTED().

[shimazu, 2017-01-18 03:45:59]

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

[commit-bot: I haz the power, 2017-01-18 03:46:11]

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

[shimazu, 2017-01-18 03:47:46]

Thanks for your reviews!
Updated. 

falken@: PTAL again?

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:532: void
ResumeAfterDownload() override { NOTREACHED(); }
On 2017/01/18 01:12:06, falken wrote:
> Can you avoid the NOTREACHED by keeping state and checking it in the test as
an
> EXPECT()?
> 
> From style guide:
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
> "Don't use these macros in tests, as they crash the test binary and leave bots
> in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which
> report failures gracefully and can continue running other tests."

Thanks, done.

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2630273002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:107:
NOTIMPLEMENTED();
On 2017/01/18 01:12:07, falken wrote:
> Do we need a TODO here? Why is it not implemented?
> 
> If it does nothing by design, I'd have a comment saying that instead of
> NOTIMPLEMENTED().

It's just because there is no test to use the result of this mock and currently
there is no need to implement it.
Added a comment instead of NOTIMPLEMENTED().

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/BUILD.gn (right):

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/BUILD.gn:753: "web/ConsoleMessageTraits.cpp",
On 2017/01/17 23:08:22, dcheng wrote:
> Nit: let's still call these files StructTraits (the OWNERS rules that make
sure
> these files are reviewed by security owners depends on this naming)

Oops, I didn't know that, thanks. Renamed these files.

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/console_message.mojom (right):

https://codereview.chromium.org/2630273002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/console_message.mojom:15: Last = Info
On 2017/01/17 23:08:22, dcheng wrote:
> Nit: we can omit Last for mojo

Done.

[falken, 2017-01-18 03:57:39]

lgtm

https://codereview.chromium.org/2630273002/diff/100001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2630273002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:538: bool*
const is_resume_after_download_called_;
nit: "was_" seems like better grammar

https://codereview.chromium.org/2630273002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (left):

https://codereview.chromium.org/2630273002/diff/100001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_dispatcher.cc:26: #include
"third_party/WebKit/public/web/WebConsoleMessage.h"
nit: I think this WebConsoleMessage can be removed now

[commit-bot: I haz the power, 2017-01-18 04:22:25]

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

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

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

[shimazu, 2017-01-18 04:54:41]

Patchset #7 (id:120001) has been deleted

[shimazu, 2017-01-18 05:00:47]

https://codereview.chromium.org/2630273002/diff/100001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2630273002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:538: bool*
const is_resume_after_download_called_;
On 2017/01/18 03:57:38, falken wrote:
> nit: "was_" seems like better grammar

Done.

https://codereview.chromium.org/2630273002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (left):

https://codereview.chromium.org/2630273002/diff/100001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_dispatcher.cc:26: #include
"third_party/WebKit/public/web/WebConsoleMessage.h"
On 2017/01/18 03:57:38, falken wrote:
> nit: I think this WebConsoleMessage can be removed now

Done.

[shimazu, 2017-01-18 05:00:53]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-01-18 05:00:54]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
dcheng@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2630273002/#ps140001
(title: "s/is/was/ , add :: before blink and remove unnecessary include")

[commit-bot: I haz the power, 2017-01-18 05:01:11]

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-18 06:47:05]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484715653760670,
"parent_rev": "cbaeb8aa76be268c5179deb56e67a765303e34cc", "commit_rev":
"5d804c8487702e7a56e7b6f8370a0d10a756df6c"}

[commit-bot: I haz the power, 2017-01-18 06:47:48]

Description was changed from

==========
ServiceWorker: mojofy ResumeAfterDownload and AddMessageToConsole

This patch mojofies the following two messages:
 - EmbeddedWorkerMsg_ResumeAfterDownload
 - EmbeddedWorkerMsg_AddMessageToConsole

EmbeddedWorkerDispatcher still holds the WorkerWrapper for StopWorker.
I'm planning to remove EWDispatcher when removing the StopWorker message.

BUG=629701
==========

to

==========
ServiceWorker: mojofy ResumeAfterDownload and AddMessageToConsole

This patch mojofies the following two messages:
 - EmbeddedWorkerMsg_ResumeAfterDownload
 - EmbeddedWorkerMsg_AddMessageToConsole

EmbeddedWorkerDispatcher still holds the WorkerWrapper for StopWorker.
I'm planning to remove EWDispatcher when removing the StopWorker message.

BUG=629701

Review-Url: https://codereview.chromium.org/2630273002
Cr-Commit-Position: refs/heads/master@{#444296}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d804c8487702e7a56e7b6f8370a...
==========

[commit-bot: I haz the power, 2017-01-18 06:47:50]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/5d804c8487702e7a56e7b6f8370a...