feat(digital-credentials): support non-top-level contexts in setVirtualWalletBehavior - #4224
mohamedamir wants to merge 1 commit into
Conversation
2338c60 to
8efd285
Compare
8efd285 to
695dc99
Compare
64e78f3 to
1c620bc
Compare
| browsingContextStorage.addContext(context); | ||
| if (!context.isTopLevelContext()) { | ||
| context.parent!.addChild(context.id); | ||
| if (context.cdpTarget === context.parent!.cdpTarget) { |
There was a problem hiding this comment.
I think this might be not needed (and flaky). If the local frame is just being created, there should be no behavior targeting it directly by id yet. If there is a global target behavior, it should apply via CdpTarget. WDYT?
There was a problem hiding this comment.
OK, I have been investigating this.
The problem is the logic in Chromium inherits the behaviour from the parent which we decided against here.
I am sorry I overlooked this during the code review in Chromium.
What do you think is the best path forward now?
The spec doesn't define this parent-frame behaviour inheritance.
Should I update the logic in Chromium and then comeback to this PR?
Do you see any other path forward?
There was a problem hiding this comment.
Yeah I think in Chromium we should not inherit. To summarize, if Chromium does not inherit and the spec does not define inheriting:
- when a new target is created, the client could not have known the targetId so we only need to enable if we have userContext level configuration. The spec does not specify the userContext level configuration so we should not expect userContext configuration in the implementation.
- when frame exists, it can be targeted by context ID.
So indeed I think we should be good with just doing the configuration when unblocking CDP targets and removing this part. Does it sound correct?
…alWalletBehavior This CL expands support for digitalCredentials.setVirtualWalletBehavior to accept non-top-level contexts (subframes). If a subframe does not have an explicitly configured virtual wallet behavior, it falls back to the global configuration (or user context configuration), rather than inheriting from its parent context. This is achieved by explicitly sending the resolved behavior (which defaults to 'clear') to the subframe via CDP to override Chromium's internal parent-inheritance behavior. TAG=agy CONV=ce3d0ebd-0f95-4b7d-ab74-fc357eac15ed
1c620bc to
4584d30
Compare
| }), | ||
| ); | ||
| } | ||
| const dcConfig = this.contextConfigStorage.getActiveConfig( |
There was a problem hiding this comment.
do we always have a dcConfig? should not sometimes nothing be configured?
There was a problem hiding this comment.
It seems getAtiveConfig always returns something, even when nothing is configured.
But then the logic below
const dcBehavior = dcConfig.digitalCredentialsBehavior; might indeed be undefined, which is handled in the code AFAICT.
Am I overlooking something?
There was a problem hiding this comment.
I think it does kind of handle it but we might also save sending setVirtualWalletBehavior if nothing was configured at all (as opposed to clear action). Maybe it's something for a follow-up.
This CL expands support for digitalCredentials.setVirtualWalletBehavior
to accept non-top-level contexts (subframes).
If a subframe does not have an explicitly configured virtual wallet behavior,
it falls back to the global configuration (or user context configuration),
rather than inheriting from its parent context. This is achieved by explicitly
sending the resolved behavior (which defaults to 'clear') to the subframe via
CDP to override Chromium's internal parent-inheritance behavior.
TAG=agy
CONV=ce3d0ebd-0f95-4b7d-ab74-fc357eac15ed