Remove cookie field from alerts
Categories
(Toolkit :: Alerts Service, task)
Tracking
()
People
(Reporter: saschanaz, Unassigned)
References
Details
The cookie parameter/field has been used to map between parent process listener and content process listener. Now the listener only exists in the parent process and thus it can go.
|
||
Comment 1•1 year ago
|
||
FYI we're using the cookie field in Thunderbird to identify which message an alert belongs to. There's probably some other way, but that's what we're doing at the moment.
|
Reporter | |
Comment 2•1 year ago
|
||
It's also passing the same value to the name parameter too, maybe that's enough for Thunderbird? (Not sure why we would need two parameters having the same value.)
|
|
||
Comment 3•1 year ago
|
||
We're using it to tell notifications apart where the same observer is used for multiple notifications. It looks like the extensions API does this as well. We could convert our code to use a new observer for each notification, I suppose.
|
|
Reporter | |
Comment 4•1 year ago
|
||
Oh hmm. We sorta assume that each notification has its own observer right now (example), as that's how notification action is implemented for Windows (or otherwise we don't have a field to indicate which action is triggered). But we may need to reconsider that. Thank you!
|
|
Reporter | |
Comment 5•1 year ago
|
||
Hello Desktop Integration people, any opinion about comment #3 and comment #4?
|
||
Comment 6•1 year ago
|
||
(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #5)
Hello Desktop Integration people, any opinion about comment #3 and comment #4?
I vaguely remember being confused as to why there was cookie and name. IIRC, the name is used for deduping. Generally, I would like some unique identifier attached to a callback of this kind, but I don't have any opinion about whether it should be the existing cookie. My expectation is that cleaning this up will be better than any existing pattern. Nicholas may have deeper thoughts here: I can't page this back in right now.
|
|
||
Comment 7•1 year ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #6)
(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #5)
Hello Desktop Integration people, any opinion about comment #3 and comment #4?
I vaguely remember being confused as to why there was
cookieandname. IIRC, thenameis used for deduping. Generally, I would like some unique identifier attached to a callback of this kind, but I don't have any opinion about whether it should be the existingcookie. My expectation is that cleaning this up will be better than any existing pattern. Nicholas may have deeper thoughts here: I can't page this back in right now.
Oh, another wrinkle -- maybe some of these fields are client-controlled (i.e., untrusted) and some are not. Losing the browser-controleld, trusted value sounds bad for deduping and avoiding confusion errors.
|
|
Reporter | |
Comment 8•1 year ago
|
||
I believe cookie is also client-controlled as it's from the constructor too. Maybe bug 1932393 will handle the "trusted" ID use case.
|
||
Comment 9•1 year ago
|
||
Off top of head and after a minute of investigation, I can't find any reason to object to removing the cookie field wrt Desktop Integrations uses. I think once upon a time we used it as part of the tag to differentiate the notification to Windows, but if that was ever the case it has since been removed.
|
||
Comment 10•15 days ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #3)
We're using it to tell notifications apart where the same observer is used for multiple notifications. It looks like the extensions API does this as well. We could convert our code to use a new observer for each notification, I suppose.
Bug 2022705 shows a regression on Android caused by the repurposing of the data field in observer notifications (analysis in https://bugzilla.mozilla.org/show_bug.cgi?id=2022705#c1 ).
I'm going to fix that regression by switching away from the data parameter, and will also update the IDL file to emphasize that the cookie data is deprecated as a warning, referencing this bug.
|
|
||
Comment 11•14 days ago
|
||
I'll remove the uses in Thunderbird. The one I linked to in this bug has gone already, but there is another.
Description
•