[fuchsia] Finish push messaging api implementation [chromium/src : main]
Peter Beverloo (Gerrit)
Peter Beverloo added 5 comments![]( "Open in Gerrit")
I think this looks fine and you don't have to block on me, but these are a lot of TODOs, including questionable, insecure code, that you're planning to check in. Please ask one of the //fuchsia_web(/web engine) OWNERS to approve that.
const std::string& notification_id) {}nit: I'd suggest putting `NOTIMPLEMENTED()` in these implementations, they don't hit very often but it makes it much easier to diagnose why something isn't working later on.
return 0;fyi: Always returning the same value will mean that new notifications may overwrite data for older notifications in the //content-level NotificationDatabase.
base::FilePath("/tmp"),This is a bit of a strange decision - the GCM Driver will store e.g. public/private key pairs in this directory, which in /tmp may become readable (or at least observable) by other users. Is this important?
break;
case blink::mojom::PushEventStatus::SERVICE_WORKER_ERROR:
// Do nothing, and hope the error is transient.
break;
case blink::mojom::PushEventStatus::NO_APP_LEVEL_PERMISSION_IGNORE:
// Do nothing, ignore push messages during the grace period.
break;
case blink::mojom::PushEventStatus::NO_APP_LEVEL_PERMISSION_UNSUBSCRIBE:
unsubscribe_reason =
blink::mojom::PushUnregistrationReason::NO_APP_LEVEL_PERMISSION;
break;
case blink::mojom::PushEventStatus::UNKNOWN_APP_ID:
unsubscribe_reason =
blink::mojom::PushUnregistrationReason::DELIVERY_UNKNOWN_APP_ID;
break;
case blink::mojom::PushEventStatus::PERMISSION_DENIED:
unsubscribe_reason =
blink::mojom::PushUnregistrationReason::DELIVERY_PERMISSION_DENIED;
break;
case blink::mojom::PushEventStatus::NO_SERVICE_WORKER:
unsubscribe_reason =
blink::mojom::PushUnregistrationReason::DELIVERY_NO_SERVICE_WORKER;
break;
case blink::mojom::PushEventStatus::PERMISSION_REVOKED_ABUSIVE:
unsubscribe_reason =
blink::mojom::PushUnregistrationReason::PERMISSION_REVOKED_ABUSIVE;
break;
case blink::mojom::PushEventStatus::PERMISSION_REVOKED_DISRUPTIVE:
unsubscribe_reason =
blink::mojom::PushUnregistrationReason::PERMISSION_REVOKED_DISRUPTIVE;
break;
}Please fix this WARNING reported by Code Duplication: Duplicate code found. Review for potential refactoring.
This block matches lines...
Duplicate code found. Review for potential refactoring.
This block matches lines 617-648 in [push_messaging_service_impl.cc](https://source.corp.google.com/h/chromium/chromium/src/+/main:chrome/browser/push_messaging/push_messaging_service_impl.cc;l=617-648;drc=95f912cedcfed49b1360d308897f441882b57f13).
----------------
Was this useful? [Yes 👍](http://go/gerrit-code-duplication-thumbs-up?title=[Yes%20👍]%20https://chromium-review.googlesource.com/c/chromium/src/+/7047169/20) / [No 👎](http://go/gerrit-code-duplication-thumbs-down?title=[No%20👎]%20https://chromium-review.googlesource.com/c/chromium/src/+/7047169/20)
Please provide additional feedback at go/pdeio-code-duplication-bug
Related details
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 5 comments![]( "Open in Gerrit")
Also add Sergey as the fuchsia_web owner.
nit: I'd suggest putting `NOTIMPLEMENTED()` in these implementations, they don't hit very often but it makes it much easier to diagnose why something isn't working later on.
Done
fyi: Always returning the same value will mean that new notifications may overwrite data for older notifications in the //content-level NotificationDatabase.
Thank you for the insight (again 😂).
Returning self-incremental integer.
This is a bit of a strange decision - the GCM Driver will store e.g. public/private key pairs in this directory, which in /tmp may become readable (or at least observable) by other users. Is this important?
The file system on fuchsia is significantly different (one of the security features), the /tmp is only visible to the component (chrome in the context), no other components (other applications) can read it.
The reason I am using /tmp here is because the /data/ isn't available in the web_engine_shell which is different than the end user facing web_engine (another fuchsia specific thing).
But yes, it will be addressed soon, the web_engine should use persistent storage for the purpose.
break;Please fix this WARNING reported by Code Duplication: Duplicate code found. Review for potential refactoring.
This block matches lines...Duplicate code found. Review for potential refactoring.
This block matches lines 617-648 in [push_messaging_service_impl.cc](https://source.corp.google.com/h/chromium/chromium/src/+/main:chrome/browser/push_messaging/push_messaging_service_impl.cc;l=617-648;drc=95f912cedcfed49b1360d308897f441882b57f13).----------------
Was this useful? [Yes 👍](http://go/gerrit-code-duplication-thumbs-up?title=[Yes%20👍]%20https://chromium-review.googlesource.com/c/chromium/src/+/7047169/20) / [No 👎](http://go/gerrit-code-duplication-thumbs-down?title=[No%20👎]%20https://chromium-review.googlesource.com/c/chromium/src/+/7047169/20)
Please provide additional feedback at go/pdeio-code-duplication-bug
- Peter Beverloo
- Sergey Ulanov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergey Ulanov (Gerrit)
Sergey Ulanov added 6 comments![]( "Open in Gerrit")
parent_context_.DeliverPushMessage(How does this work? IIUC push messages are normally delivered to service workers, see https://source.chromium.org/chromium/chromium/src/+/main:content/browser/push_messaging/push_messaging_router.cc;drc=309e7aa9204b3a6d3c0770254499dd0044166550;l=129 .
So far we didn't support service workers in WebEngine, and I'm not sure we want to support it.
base::FilePath("/tmp"),What is stored there? can we use a different `GCMStore` implementation that keeps all data in memory?
network_connection_tracker_.reset(content::GetNetworkConnectionTracker());`connect::GetNetworkConnectionTracker()` keeps the ownership of the result, so we cannot put it in a `unique_ptr`. So we don't need `network_connection_tracker_` - just call GetNetworkConnectionTracker() when you need it.
blink::mojom::PermissionStatus WebEnginePermissionDelegate::GetPermissionStatus(does this function get called for NOTIFICATION permission? Maybe the change in `GetPermissionResultForCurrentDocument` is enough?
// TODO(crbug.com/424479300): This permission needs to be checked against thePlease add a comment to make it clear why we need to allow this permission for all frames.
Would it make sense to add this permission in fuchsia.web API and then allow it only for pages that actually need? (i.e. cast_runner would need to call `SetPermissionState`).
Related details
- Peter Beverloo
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 5 comments![]( "Open in Gerrit")
Thank you Sergey, I am addressing other comments now, but would like to explain the support of service worker and GCMStore first.
parent_context_.DeliverPushMessage(How does this work? IIUC push messages are normally delivered to service workers, see https://source.chromium.org/chromium/chromium/src/+/main:content/browser/push_messaging/push_messaging_router.cc;drc=309e7aa9204b3a6d3c0770254499dd0044166550;l=129 .
So far we didn't support service workers in WebEngine, and I'm not sure we want to support it.
Yes, web engine does support service workers now, and it's used by Google meet 👍
base::FilePath("/tmp"),What is stored there? can we use a different `GCMStore` implementation that keeps all data in memory?
I think currently they are key pairs and some other connection related information.
I do agree this should be optimized, but I left a TODO here for now. We should either persistently store the data, or use some in memory implementation. Not sure if GCMStore supports it yet.
blink::mojom::PermissionStatus WebEnginePermissionDelegate::GetPermissionStatus(does this function get called for NOTIFICATION permission? Maybe the change in `GetPermissionResultForCurrentDocument` is enough?
Very likely, I may mistakenly forget to remove one of these two. I will confirm it later.
Related details
- Peter Beverloo
- Sergey Ulanov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He voted and added 4 comments![]( "Open in Gerrit")
Votes added by Zijie He
| Commit-Queue | +1 |
4 comments
Also talked offline with Sergey, I will prepare a document to note the expectation of service worker in web engine.
network_connection_tracker_.reset(content::GetNetworkConnectionTracker());`connect::GetNetworkConnectionTracker()` keeps the ownership of the result, so we cannot put it in a `unique_ptr`. So we don't need `network_connection_tracker_` - just call GetNetworkConnectionTracker() when you need it.
Ha, interesting, thank you for pointing it out. Yes, we should use raw_ptr. The functionality of the network_connection_tracker_ is to create the NetworkConnectionTracker in the right thread (UI vs IO).
blink::mojom::PermissionStatus WebEnginePermissionDelegate::GetPermissionStatus(Zijie Hedoes this function get called for NOTIFICATION permission? Maybe the change in `GetPermissionResultForCurrentDocument` is enough?
Very likely, I may mistakenly forget to remove one of these two. I will confirm it later.
Ah, no, the permission control of service worker uses this function instead.
On the other hand, very likely the one ForCurrentDocument is not a must-have for service worker use scenarios. But it may break the assumptions of the users, i.e. page does not get the permission, but service worker does.
// TODO(crbug.com/424479300): This permission needs to be checked against thePlease add a comment to make it clear why we need to allow this permission for all frames.
Would it make sense to add this permission in fuchsia.web API and then allow it only for pages that actually need? (i.e. cast_runner would need to call `SetPermissionState`).
Added comments.
It's very unlikely a cast_runner would need to use the push messaging api or notification, but only UC meet. Though UC meet is also a cast app, we may wanna allow only a managed set of origins like the list in push_messaging_service_impl.cc/IsPermissionGranted.
Related details
- Peter Beverloo
- Sergey Ulanov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergey Ulanov (Gerrit)
Sergey Ulanov added 1 comment![]( "Open in Gerrit")
network_connection_tracker_.reset(content::GetNetworkConnectionTracker());Zijie He`connect::GetNetworkConnectionTracker()` keeps the ownership of the result, so we cannot put it in a `unique_ptr`. So we don't need `network_connection_tracker_` - just call GetNetworkConnectionTracker() when you need it.
Ha, interesting, thank you for pointing it out. Yes, we should use raw_ptr. The functionality of the network_connection_tracker_ is to create the NetworkConnectionTracker in the right thread (UI vs IO).
I believe `HandleContextRequest` should be called on the main/UI thread, so it should be safe to call GetNetworkConnectionTracker() there - does that not work?
Related details
- Peter Beverloo
- Zijie He
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 1 comment![]( "Open in Gerrit")
network_connection_tracker_.reset(content::GetNetworkConnectionTracker());Zijie He`connect::GetNetworkConnectionTracker()` keeps the ownership of the result, so we cannot put it in a `unique_ptr`. So we don't need `network_connection_tracker_` - just call GetNetworkConnectionTracker() when you need it.
Sergey UlanovHa, interesting, thank you for pointing it out. Yes, we should use raw_ptr. The functionality of the network_connection_tracker_ is to create the NetworkConnectionTracker in the right thread (UI vs IO).
I believe `HandleContextRequest` should be called on the main/UI thread, so it should be safe to call GetNetworkConnectionTracker() there - does that not work?
- Sergey Ulanov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |