Mohamed Amir Yosef (Gerrit)

unread,

6:27 AM (5 hours ago) 6:27 AM

to Alex Rudenko, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org

Attention needed from Alex Rudenko

Mohamed Amir Yosef voted and added 1 comment

Votes added by Mohamed Amir Yosef

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Mohamed Amir Yosef . resolved

Hi Alex,

Could you please review this CL?

Thank you,
Mohamed

Open in Gerrit

Related details

Attention is currently required from:

Alex Rudenko

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alex Rudenko (Gerrit)

unread,

6:59 AM (5 hours ago) 6:59 AM

to Mohamed Amir Yosef, Anushka Kulkarni, Tejas Dhagawkar, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org

Attention needed from Mohamed Amir Yosef

Alex Rudenko added 3 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Alex Rudenko . unresolved

Thanks, looks good to me but let's also add an inspector-protocol test to verify the behavior e2e, i.e., by querying the web API within the iframe and then checking that it did not leak to the frames in the same target.

File content/browser/devtools/protocol/digital_credentials_handler.h

Line 47, Patchset 3 (Latest): std::optional<String> in_frame_id = std::nullopt) override;

Alex Rudenko . unresolved

NIT: It is generally discouraged to add default arguments to overridden virtual methods (as per the Google C++ Style Guide). Consider removing the default argument here and updating the older tests in `digital_credentials_handler_unittest.cc` to explicitly pass `std::nullopt` instead.

File content/browser/devtools/protocol/digital_credentials_handler_unittest.cc

Line 66, Patchset 3 (Latest): return root_node->child_at(index);

Alex Rudenko . unresolved

NIT: `AppendChild` returns the newly created `TestRenderFrameHost*`, so you can simplify this method by using that return value directly:
```cpp
    TestRenderFrameHost* child_rfh = static_cast<TestRenderFrameHost*>(
        root_node->current_frame_host())->AppendChild(name);
    return child_rfh->frame_tree_node();
```

Open in Gerrit

Related details

Attention is currently required from:

Mohamed Amir Yosef

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mohamed Amir Yosef (Gerrit)

unread,

7:26 AM (4 hours ago) 7:26 AM

to Anushka Kulkarni, Tejas Dhagawkar, Alex Rudenko, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org

Attention needed from Alex Rudenko

Mohamed Amir Yosef added 4 comments

Patchset-level comments

File-level comment, Patchset 3:

Alex Rudenko . resolved

Thanks, looks good to me but let's also add an inspector-protocol test to verify the behavior e2e, i.e., by querying the web API within the iframe and then checking that it did not leak to the frames in the same target.

Mohamed Amir Yosef

Done

File-level comment, Patchset 5 (Latest):

Mohamed Amir Yosef . resolved

Thanks Alex

I hope this is what you had in mind wrt to the test!

File content/browser/devtools/protocol/digital_credentials_handler.h

Line 47, Patchset 3: std::optional<String> in_frame_id = std::nullopt) override;

Alex Rudenko . resolved

NIT: It is generally discouraged to add default arguments to overridden virtual methods (as per the Google C++ Style Guide). Consider removing the default argument here and updating the older tests in `digital_credentials_handler_unittest.cc` to explicitly pass `std::nullopt` instead.

Mohamed Amir Yosef

Done

File content/browser/devtools/protocol/digital_credentials_handler_unittest.cc

Line 66, Patchset 3: return root_node->child_at(index);

Alex Rudenko . resolved

NIT: `AppendChild` returns the newly created `TestRenderFrameHost*`, so you can simplify this method by using that return value directly:
```cpp
    TestRenderFrameHost* child_rfh = static_cast<TestRenderFrameHost*>(
        root_node->current_frame_host())->AppendChild(name);
    return child_rfh->frame_tree_node();
```

Mohamed Amir Yosef

Done

Open in Gerrit

Related details

Attention is currently required from:

Alex Rudenko

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alex Rudenko (Gerrit)

unread,

8:20 AM (4 hours ago) 8:20 AM

to Mohamed Amir Yosef, Anushka Kulkarni, Tejas Dhagawkar, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org

Attention needed from Mohamed Amir Yosef

Alex Rudenko added 1 comment

File third_party/blink/web_tests/http/tests/inspector-protocol/digital-credentials/digital-credentials-virtual_wallet-scoped-iframe.js

Line 93, Patchset 5 (Latest): await dp.DigitalCredentials.setVirtualWalletBehavior({action: 'clear'});

Alex Rudenko . unresolved

Since the mock was scoped to `iframe1Id` earlier, calling `clear` without a `frameId` here clears the main frame's virtual wallet, not the iframe's wallet.

Consider adding `frameId: iframe1Id` to ensure the iframe's state is properly cleaned up (even though it's the end of the test).

Open in Gerrit

Related details

Attention is currently required from:

Mohamed Amir Yosef

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alex Rudenko (Gerrit)

unread,

8:22 AM (4 hours ago) 8:22 AM

to Mohamed Amir Yosef, Anushka Kulkarni, Tejas Dhagawkar, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org

Attention needed from Mohamed Amir Yosef

Alex Rudenko voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Mohamed Amir Yosef

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mohamed Amir Yosef (Gerrit)

unread,

8:41 AM (3 hours ago) 8:41 AM

to Alex Rudenko, Anushka Kulkarni, Tejas Dhagawkar, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org

Mohamed Amir Yosef voted and added 2 comments

Votes added by Mohamed Amir Yosef

Commit-Queue

+2

2 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Mohamed Amir Yosef . resolved

Thanks a lot, Alex!

File third_party/blink/web_tests/http/tests/inspector-protocol/digital-credentials/digital-credentials-virtual_wallet-scoped-iframe.js

Line 93, Patchset 5: await dp.DigitalCredentials.setVirtualWalletBehavior({action: 'clear'});

Alex Rudenko . resolved

Since the mock was scoped to `iframe1Id` earlier, calling `clear` without a `frameId` here clears the main frame's virtual wallet, not the iframe's wallet.
Consider adding `frameId: iframe1Id` to ensure the iframe's state is properly cleaned up (even though it's the end of the test).

Mohamed Amir Yosef

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

9:55 AM (2 hours ago) 9:55 AM

to Mohamed Amir Yosef, Alex Rudenko, Anushka Kulkarni, Tejas Dhagawkar, chromium...@chromium.org, devtools...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: third_party/blink/web_tests/http/tests/inspector-protocol/digital-credentials/digital-credentials-virtual_wallet-scoped-iframe.js
Insertions: 4, Deletions: 1.

@@ -90,7 +90,10 @@
     data: {},
   })`);
 
-  await dp.DigitalCredentials.setVirtualWalletBehavior({action: 'clear'});
+  await dp.DigitalCredentials.setVirtualWalletBehavior({
+    action: 'clear',
+    frameId: iframe1Id,
+  });
 
   testRunner.log('iframe1 result: ' + JSON.stringify(iframe1Result));
   testRunner.log('iframe2 result: ' + JSON.stringify(iframe2Result));
```

Change information

Commit message:

[DC] Add frameId param to DigitalCredentials.setVirtualWalletBehavior

Currently, DigitalCredentials.setVirtualWalletBehavior only applies
virtual wallet behavior globally or at the top-level WebContents/main
frame level. In automated browser testing (via WebDriverBiDi, Puppeteer,
or DevTools clients), test scripts frequently need to test cross-origin
iframes or embedded contexts requesting digital credentials.

This CL adds an optional Page.FrameId frameId parameter to the command
definition in DigitalCredentials.pdl. When provided, the DevTools
handler resolves the token using FrameTreeNodeFromDevToolsFrameToken
within the current session's frame subtree and scopes the virtual wallet
behavior to the target frame tree node. If frameId is omitted, behavior
defaults to the main frame of the session.

BUG=470685266
TEST=DigitalCredentialsHandlerTest.*
AG=agy
CONV=2f51e7d6-fb1e-48e1-a310-2d19aaac61df

Change-Id: I55b5b97dfca7ce3941d155ce909077edcca6aea7

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8045193

Commit-Queue: Mohamed Amir Yosef <ma...@chromium.org>

Reviewed-by: Alex Rudenko <alexr...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1657156}

Files:

M content/browser/devtools/protocol/digital_credentials_handler.cc
M content/browser/devtools/protocol/digital_credentials_handler.h
M content/browser/devtools/protocol/digital_credentials_handler_unittest.cc
M third_party/blink/public/devtools_protocol/domains/DigitalCredentials.pdl
A third_party/blink/web_tests/http/tests/inspector-protocol/digital-credentials/digital-credentials-virtual_wallet-scoped-iframe-expected.txt
A third_party/blink/web_tests/http/tests/inspector-protocol/digital-credentials/digital-credentials-virtual_wallet-scoped-iframe.js

Change size: L

Delta: 6 files changed, 247 insertions(+), 22 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Alex Rudenko

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[DC] Add frameId param to DigitalCredentials.setVirtualWalletBehavior [chromium/src : main]

Mohamed Amir Yosef (Gerrit)

Mohamed Amir Yosef voted and added 1 comment

Votes added by Mohamed Amir Yosef

1 comment

Related details

Alex Rudenko (Gerrit)

Alex Rudenko added 3 comments

Related details

Mohamed Amir Yosef (Gerrit)

Mohamed Amir Yosef added 4 comments

Related details

Alex Rudenko (Gerrit)

Alex Rudenko added 1 comment

Related details

Alex Rudenko (Gerrit)

Alex Rudenko voted Code-Review+1

Related details

Mohamed Amir Yosef (Gerrit)

Mohamed Amir Yosef voted and added 2 comments

Votes added by Mohamed Amir Yosef

2 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information