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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by ke.he
Modified:
4 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 72 (45 generated)
ke.he
Use a new CL :)
4 months, 2 weeks 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 ...
4 months, 2 weeks 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 ...
4 months, 2 weeks 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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/, ...
4 months, 1 week 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. ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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: ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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:)
4 months, 1 week ago (2017-02-20 09:22:53 UTC) #41
Tom Sepez
lgtm
4 months, 1 week 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
4 months, 1 week 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
4 months, 1 week 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 ...
4 months, 1 week 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
4 months, 1 week 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)
4 months, 1 week 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
4 months, 1 week 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: ...
4 months, 1 week 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
4 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: ...
4 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
4 months ago (2017-02-22 11:34:45 UTC) #69
commit-bot: I haz the power
4 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589