The Push-Messaging IPC interfaces are converted into mojofication by Issue 2690203003 (https://codereview.chromium.org/2690203003/), while we have some TODO tasks in next step.
1) In push_messaging_manager.cc, try to store |callback| in |data| (RegisterData) instead of passing it everywhere needs it. one example: when PushMessagingManager::Subscribe() calls SendSubscriptionError(), the callback is passed as paremeter.
2) Considering to re-design the mojo callback interface. Example: adding a blink::WebPushError::ErrorTypeNone would make the success boolean redundant in GetPermissionStatusCallback.
3)Naming the functions like DidFoo as opposed to FooCallback in Push_Provider{.h|.cc}. which conveys information about the fact that something happened.
4)Extract {endpoint, options, p256dh, auth} in a separate Mojo structure to avoid these four optional arguments set both for subscribing and getting the active subscription. see function "Push_provider::GetSubscriptionCallback(...)".
5)In PushProvider::SubscribeCallback(), to replace "if (!callbacks) return;" by DCHECK(callbacks).
6)In PushMessaging.mojom, there is a TODO:
"TODO(heke): The type-mapping brings the struct and enums are duplicated defined. Need to remove/replace those defined in content or blink namespace."
I'll use one "code clean" patch to resolve 1) 3) and 5) first. After that I will use a "re-design mojo-interface" patch to resolve 2) and 4).
After above done, we can continue the "Content Modularization Project of push-messaging"(612312), and find a right time to resolve 6).
Hi, Peter, About the 4th task above:
I guess you want to create a new struct in mojo like:
struct PushSubscriptionInfo {
url.mojom.Url endpoint;
PushSubscriptionOptions options;
array<uint8> p256dh;
array<uint8> auth;
};
and change the mojo inteface into:
Subscribe(...) => (..., PushSubscriptionInfo? info);
If we do this, I found we have to construct PushSubscriptionInfo which will introduce some memory copy.
Besides, I guess we also need to add a new content::PushSubscriptionInfo struct definition, add traits to typemap it with mojo::PushSubscriptionInfo.
I prefer leave it(the 4th task) alone. Any comments? Thanks.
Comment 1 by ke...@intel.com
, Feb 17 20171) In push_messaging_manager.cc, try to store |callback| in |data| (RegisterData) instead of passing it everywhere needs it. one example: when PushMessagingManager::Subscribe() calls SendSubscriptionError(), the callback is passed as paremeter. 2) Considering to re-design the mojo callback interface. Example: adding a blink::WebPushError::ErrorTypeNone would make the success boolean redundant in GetPermissionStatusCallback. 3)Naming the functions like DidFoo as opposed to FooCallback in Push_Provider{.h|.cc}. which conveys information about the fact that something happened. 4)Extract {endpoint, options, p256dh, auth} in a separate Mojo structure to avoid these four optional arguments set both for subscribing and getting the active subscription. see function "Push_provider::GetSubscriptionCallback(...)". 5)In PushProvider::SubscribeCallback(), to replace "if (!callbacks) return;" by DCHECK(callbacks). 6)In PushMessaging.mojom, there is a TODO: "TODO(heke): The type-mapping brings the struct and enums are duplicated defined. Need to remove/replace those defined in content or blink namespace."