| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hi,
The previous failure was caused by an IDL compiler issue[1], which has been fixed.
After rebasing, the patch passes successfully now. Could you please take another look and vote again?
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7109359
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
public Supplement<T> {Drive-by: Supplement<ExecutionContext> is a good option instead of templating, as it's a parent class of both `LocalDOMWindow` and `WorkerGlobalScope`. You end up implicitly being a Supplement of some other types you don't need (e.g., worklet), but you can use CHECKs to verify that you're only actually registering for the intended `ExecutionContext` subclasses.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
public Supplement<T> {Drive-by: Supplement<ExecutionContext> is a good option instead of templating, as it's a parent class of both `LocalDOMWindow` and `WorkerGlobalScope`. You end up implicitly being a Supplement of some other types you don't need (e.g., worklet), but you can use CHECKs to verify that you're only actually registering for the intended `ExecutionContext` subclasses.
Thanks for the suggestion!
I want to confirm that I understood your point correctly. My understanding is that you are recommending Supplement<ExecutionContext> to keep the structure simple, while using CHECKs to ensure that it is only used with the intended subclasses such as LocalDOMWindow and WorkerGlobalScope, even though it technically becomes attachable to other ExecutionContext subclasses.
I also wanted to ask whether this approach is generally preferred in Chromium for this type of supplement. I referenced existing partial mixin implementations that use a templated Supplement<T> pattern, and some of my patches following that style have already landed. If the ExecutionContext-based approach is preferred, I can adjust my upcoming patches accordingly.
Thanks again!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
public Supplement<T> {Suyeon JiDrive-by: Supplement<ExecutionContext> is a good option instead of templating, as it's a parent class of both `LocalDOMWindow` and `WorkerGlobalScope`. You end up implicitly being a Supplement of some other types you don't need (e.g., worklet), but you can use CHECKs to verify that you're only actually registering for the intended `ExecutionContext` subclasses.
Thanks for the suggestion!
I want to confirm that I understood your point correctly. My understanding is that you are recommending Supplement<ExecutionContext> to keep the structure simple, while using CHECKs to ensure that it is only used with the intended subclasses such as LocalDOMWindow and WorkerGlobalScope, even though it technically becomes attachable to other ExecutionContext subclasses.I also wanted to ask whether this approach is generally preferred in Chromium for this type of supplement. I referenced existing partial mixin implementations that use a templated Supplement<T> pattern, and some of my patches following that style have already landed. If the ExecutionContext-based approach is preferred, I can adjust my upcoming patches accordingly.
Thanks again!
I think, between the two approaches, I would lean toward `Supplement<ExecutionContext>`.
However, an effort just started to get rid of Supplement/Supplementable entirely (https://groups.google.com/a/chromium.org/g/blink-dev/c/nbTIoUZW6n0). So probably the optimal thing to do would be to follow the pattern that is being put in to place. Which means you may be able to put this class on `WindowOrWorkerGlobalScope` in C++?(https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/window_or_worker_global_scope.h)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Set Ready For Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
lgtm but let's wait for Nate's comment.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |