[fedcm] Extend the LocalFrame to query/notify <login>s elements [chromium/src : main]
Sam Goto (Gerrit)
Sam Goto added 8 comments![]( "Open in Gerrit")
methods to the LocalFrame renderer mojom service, so that the browserSam Goto```suggestion
methods to the LocalFrame renderer mojom interface, so that the browser
```
Done
GetFederations() => (array<Federation> federations);Sam GotoTechnically per https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit?tab=t.0#heading=h.jv90dicow9kc, these new methods need to have at least one non-test usage, but they're only really exercised in browser tests and no production path.
https://chromium-review.googlesource.com/c/chromium/src/+/7539587 probably has some "production"-paths. Would it be possible to chain the next CL on top of this one that shows how these are really going to be used? (or point out which parts of that aforementioned CL I should look at to get a feel for how these mojo methods would be used outside of tests)?
I'm working on a CL that branches of this one that can give a concrete sense of how this CL is intended to be used.
I'll report back momentarily when I set that up.
const base::UnguessableToken id_;Sam Gotoafter looking around a bit, this should probably go away in favor of DOMNodeId.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/dom_node_ids.h;l=25;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1?q=renderer%2Fcore%2Fdom%2Fdom_node_ids.h&ss=chromium%2Fchromium%2Fsrc
Oh yeah, that's so much better! Done!
options->config->client_id = "";Sam Goto```suggestion
options->config->client_id = g_empty_atom;
```Maybe?
Done
options->nonce = "";Sam GotoMaybe same for now.
Done
options->params_json = params_attr;Sam GotoMaybe some error handling like https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_iframe_element.cc;l=564-576;drc=976a71ef2a509d4391d814b52b9f4700fdf35789?
Ah, yes, good point, done [1].
[1] Per your guidance, I broke this CL into two and moved the dispatching here: https://chromium-review.googlesource.com/c/chromium/src/+/7552493
token_ = token;Christian BiesingerCan you `DCHECK(isConnected());` first?
Dominic Farolinojust curious -- what's wrong with dispatching an event on a disconnected element? (I know this can't actually happen here, because of how we find the element)
Sam GotoNothing, it's just that this event is not intended to ever be dispatched on a disconnected federation element (at least through this path), so I figured we'd "enforce" that here.
SGTM. Done.
- Also, per your guidance, I broke this CL into two and addressed this feedback here https://chromium-review.googlesource.com/c/chromium/src/+/7552493
attribute EventHandler ontoken;Sam GotoPer https://github.com/w3ctag/design-principles/pull/612, I think you'll want to add this to global_event_handlers.idl. I'd go with that approach in the meantime, and revert back to this one if my PR turns out to be in the wrong direction.
In the process of generalizing <login> to be able to handle other <credential> types I had to use a more generic event rather than "token" (which is more federation specific) and ended up reusing "onselect" which I think is already in the global event handlers. We can choose a different event type in the future, but for now, I think this works. WDYT?
Also, addressed in this CL: https://chromium-review.googlesource.com/c/chromium/src/+/7552493
Related details
- Christian Biesinger
- Dominic Farolino
- Nasko Oskov
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. |