Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2690203003: Convert push_messaging IPC msgs into mojo interfaces (Closed)

Created:
9 months, 1 week ago by ke.he
Modified:
9 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, Peter Beverloo, johnme+watch_chromium.org, nasko+codewatch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, darin (slow to review), harkness+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert push_messaging IPC msgs into mojo interfaces In this patch, We added a push_messaging.mojom file and converted the old push messaging IPC msgs into mojo interfaces. We also added the typemap and traits files for those data structures used as parameters of IPC messages of push_messaging. We temporarily place the mojom and traits files alongside the original push_messaging_messages.h. They will be moved into blink when the push_messaging related {.h|cc} files are moved into blink in following steps. BUG=612312 Review-Url: https://codereview.chromium.org/2690203003 Cr-Commit-Position: refs/heads/master@{#452011} Committed: https://chromium.googlesource.com/chromium/src/+/5f426e32c2b82c6ef89c15934907652cadb5c349

Patch Set 1 #

Patch Set 2 : remove DCHECK(ChildThreadImpl::Current()) #

Total comments: 48

Patch Set 3 : addressed the comments from peter, follow-ups is recored by 693366 #

Patch Set 4 : code rebase #

Total comments: 4

Patch Set 5 : format push_messaging.mojom, less than 80 col #

Total comments: 9

Patch Set 6 : remove CHECK in traits #

Patch Set 7 : add static_assert() on each enum value. #

Patch Set 8 : code rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1129 lines, -2121 lines) Patch
M content/browser/BUILD.gn View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
A + content/browser/push_messaging/push_messaging_manager.h View 5 chunks +48 lines, -52 lines 0 comments Download
A + content/browser/push_messaging/push_messaging_manager.cc View 1 2 40 chunks +194 lines, -199 lines 0 comments Download
D content/browser/push_messaging/push_messaging_message_filter.h View 1 chunk +0 lines, -164 lines 0 comments Download
D content/browser/push_messaging/push_messaging_message_filter.cc View 1 chunk +0 lines, -917 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.h View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 chunks +8 lines, -3 lines 0 comments Download
M content/child/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M content/child/blink_platform_impl.h View 2 chunks +0 lines, -2 lines 0 comments Download
M content/child/blink_platform_impl.cc View 3 chunks +1 line, -7 lines 0 comments Download
M content/child/child_thread_impl.h View 3 chunks +0 lines, -7 lines 0 comments Download
M content/child/child_thread_impl.cc View 2 chunks +0 lines, -3 lines 0 comments Download
D content/child/push_messaging/push_dispatcher.h View 1 chunk +0 lines, -45 lines 0 comments Download
D content/child/push_messaging/push_dispatcher.cc View 1 chunk +0 lines, -62 lines 0 comments Download
M content/child/push_messaging/push_provider.h View 1 2 4 chunks +38 lines, -53 lines 0 comments Download
M content/child/push_messaging/push_provider.cc View 1 2 7 chunks +143 lines, -165 lines 0 comments Download
M content/common/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +1 line, -1 line 0 comments Download
M content/common/content_message_generator.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
A content/common/push_messaging.mojom View 1 2 3 4 5 1 chunk +161 lines, -0 lines 0 comments Download
A content/common/push_messaging.typemap View 1 chunk +28 lines, -0 lines 0 comments Download
D content/common/push_messaging_messages.h View 1 chunk +0 lines, -111 lines 0 comments Download
A content/common/push_messaging_param_traits.h View 1 chunk +67 lines, -0 lines 0 comments Download
A content/common/push_messaging_param_traits.cc View 1 2 3 4 5 6 1 chunk +350 lines, -0 lines 0 comments Download
M content/common/typemaps.gni View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/push_messaging_service.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/public/common/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
A + content/renderer/push_messaging/push_messaging_client.h View 1 2 4 chunks +17 lines, -24 lines 0 comments Download
A + content/renderer/push_messaging/push_messaging_client.cc View 1 2 5 chunks +53 lines, -55 lines 0 comments Download
D content/renderer/push_messaging/push_messaging_dispatcher.h View 1 chunk +0 lines, -84 lines 0 comments Download
D content/renderer/push_messaging/push_messaging_dispatcher.cc View 1 chunk +0 lines, -150 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 6 7 2 chunks +4 lines, -4 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -5 lines 0 comments Download

Messages

Total messages: 72 (45 generated)
ke.he
Use a new CL :)
9 months, 1 week ago (2017-02-14 07:52:25 UTC) #4
ke.he
On 2017/02/14 07:52:25, ke.he wrote: > Use a new CL :) Hi, Peter, Please add ...
9 months, 1 week ago (2017-02-14 07:53:18 UTC) #5
ke.he
https://codereview.chromium.org/2690203003/diff/20001/content/renderer/push_messaging/push_messaging_client.cc File content/renderer/push_messaging/push_messaging_client.cc (right): https://codereview.chromium.org/2690203003/diff/20001/content/renderer/push_messaging/push_messaging_client.cc#newcode32 content/renderer/push_messaging/push_messaging_client.cc:32: if (ChildThreadImpl::current()) { ChildThread may not exist in some ...
9 months, 1 week ago (2017-02-14 08:31:11 UTC) #10
Peter Beverloo
lgtm % BelongsToCurrentThread call comment in the PushProvider I left a whole bunch of nits ...
9 months ago (2017-02-16 16:08:33 UTC) #13
ke.he
Hi, Peter, Thanks so much for your great reviewing! I updated this CL based on ...
9 months ago (2017-02-17 08:22:40 UTC) #18
ke.he
Hi, kinuko@, Peter has helped review the whole CL. He is the owner of content/browser/push-messaging/, ...
9 months ago (2017-02-17 10:14:26 UTC) #24
kinuko
On 2017/02/17 10:14:26, ke.he wrote: > Hi, kinuko@, Peter has helped review the whole CL. ...
9 months ago (2017-02-17 12:47:05 UTC) #25
kinuko
https://codereview.chromium.org/2690203003/diff/60001/content/browser/push_messaging/push_messaging_manager.h File content/browser/push_messaging/push_messaging_manager.h (right): https://codereview.chromium.org/2690203003/diff/60001/content/browser/push_messaging/push_messaging_manager.h#newcode41 content/browser/push_messaging/push_messaging_manager.h:41: void Subscribe(int32_t render_frame_id, Hmm, it's a bit sad that ...
9 months ago (2017-02-17 12:47:24 UTC) #26
ke.he
Kinuko@, Thank you :) https://codereview.chromium.org/2690203003/diff/60001/content/browser/push_messaging/push_messaging_manager.h File content/browser/push_messaging/push_messaging_manager.h (right): https://codereview.chromium.org/2690203003/diff/60001/content/browser/push_messaging/push_messaging_manager.h#newcode41 content/browser/push_messaging/push_messaging_manager.h:41: void Subscribe(int32_t render_frame_id, On 2017/02/17 ...
9 months ago (2017-02-17 13:10:54 UTC) #27
Tom Sepez
https://codereview.chromium.org/2690203003/diff/80001/content/common/push_messaging.mojom File content/common/push_messaging.mojom (right): https://codereview.chromium.org/2690203003/diff/80001/content/common/push_messaging.mojom#newcode9 content/common/push_messaging.mojom:9: // TODO(heke): The type-mapping brings the struct and enums ...
9 months ago (2017-02-17 19:27:38 UTC) #32
Peter Beverloo
https://codereview.chromium.org/2690203003/diff/80001/content/common/push_messaging_param_traits.cc File content/common/push_messaging_param_traits.cc (right): https://codereview.chromium.org/2690203003/diff/80001/content/common/push_messaging_param_traits.cc#newcode47 content/common/push_messaging_param_traits.cc:47: CHECK(input >= content::PushRegistrationStatus:: On 2017/02/17 19:27:37, Tom Sepez wrote: ...
9 months ago (2017-02-17 19:40:43 UTC) #33
Tom Sepez
> Unless you feel strongly, I highly prefer this over hundreds of lines if switch ...
9 months ago (2017-02-17 22:14:36 UTC) #34
ke.he
https://codereview.chromium.org/2690203003/diff/80001/content/common/push_messaging.mojom File content/common/push_messaging.mojom (right): https://codereview.chromium.org/2690203003/diff/80001/content/common/push_messaging.mojom#newcode9 content/common/push_messaging.mojom:9: // TODO(heke): The type-mapping brings the struct and enums ...
9 months ago (2017-02-18 01:59:57 UTC) #39
ke.he
On 2017/02/17 22:14:36, Tom Sepez wrote: > > Unless you feel strongly, I highly prefer ...
9 months ago (2017-02-18 02:17:46 UTC) #40
ke.he
Hi, Tom, I have added the static_assert() on each enum value. PTAL. Thanks:)
9 months ago (2017-02-20 09:22:53 UTC) #41
Tom Sepez
lgtm
9 months ago (2017-02-21 18:03:05 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690203003/140001
9 months ago (2017-02-22 01:52:25 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690203003/140001
9 months ago (2017-02-22 03:04:36 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on ...
9 months ago (2017-02-22 03:54:32 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690203003/140001
9 months ago (2017-02-22 04:39:01 UTC) #56
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/315579)
9 months ago (2017-02-22 07:38:45 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690203003/140001
9 months ago (2017-02-22 08:50:54 UTC) #60
commit-bot: I haz the power
Failed to apply patch for content/common/typemaps.gni: While running git apply --index -p1; error: patch failed: ...
9 months ago (2017-02-22 10:35:39 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690203003/140001
9 months ago (2017-02-22 11:19:25 UTC) #64
commit-bot: I haz the power
Failed to apply patch for content/common/typemaps.gni: While running git apply --index -p1; error: patch failed: ...
9 months ago (2017-02-22 11:23:45 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690203003/160001
9 months ago (2017-02-22 11:34:45 UTC) #69
commit-bot: I haz the power
9 months ago (2017-02-22 12:38:11 UTC) #72
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/5f426e32c2b82c6ef89c15934907...

Powered by Google App Engine
This is Rietveld efc10ee0f