Wallet passes: Deep-link private passes from desktop Autofill settings [chromium/src : main]
Florian Leimgruber (Gerrit)
Florian Leimgruber voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Norge Vizcay
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Norge Vizcay (Gerrit)
Norge Vizcay added 1 comment![]( "Open in Gerrit")
kAutofillAiWalletPrivatePassesDeepLink,Do we want to make this a param of `kAutofillAiWalletPrivatePasses` or its own flag to decouple both launches?
Related details
- Florian Leimgruber
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. |
Florian Leimgruber (Gerrit)
Florian Leimgruber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Florian Leimgruber
| Commit-Queue | +1 |
1 comment
Do we want to make this a param of `kAutofillAiWalletPrivatePasses` or its own flag to decouple both launches?
Good point. Made it a separate flag.
That said, Brady just [confirmed](http://shortn/_a1o068AurX) that he is ok with rolling out the deep link for private passes before the deep link for public passes.
Related details
- Norge Vizcay
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Norge Vizcay (Gerrit)
Norge Vizcay voted Code-Review+1![]( "Open in Gerrit")
- Florian Leimgruber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
PTAL at:
- dpapad: autofill_private.d.ts
- Tim: autofill_private.idl
- Mohamed: autofill_ai_entries_list.ts
- Side: settings_localized_strings_provider.cc
The changes in these files are all straightfoward, but unfortunately a separate owner is required for each of them.
Related details
- Demetrios Papadopoulos
- Mohamed Amir Yosef
- Side YILMAZ
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
PTAL at:
- dpapad: autofill_private.d.ts
- Tim: autofill_private.idl
- Mohamed: autofill_ai_entries_list.ts
- Side: settings_localized_strings_provider.cc
The changes in these files are all straightfoward, but unfortunately a separate owner is required for each of them.
Nevermind, turns out dpapad also owns autofill_ai_entries_list.ts and settings_localized_strings_provider.cc. PTAL at those files too, then. Thanks!
Related details
- Demetrios Papadopoulos
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 2 comments![]( "Open in Gerrit")
OpenWindowProxyImpl.getInstance().openUrl(e.model.item.walletEntityUrl!);Is this line exercised by any of the tests? If not, should there be a new test case for it?
OpenWindowProxyImpl.getInstance().openUrl(e.model.item.walletEntityUrl!);Why is this safe to do? In other words what guarantees that this field even though marked as optional is always populated in this case?
For example if this is always populated for a certain type of EntityType, can you add an assertion to make that explicit?
Related details
- Florian Leimgruber
- Tim
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. |
Tim (Gerrit)
Tim added 3 comments![]( "Open in Gerrit")
Thanks Florian. The IDL and api_util changes are mostly looking good, just some questions around adding tests.
return chrome::kWalletPassesPageURL;Do we want a test case to cover this as well just for completeness? It's currently unreached by tests.
return base::StringPrintf(
chrome::kWalletPrivatePassPageURL,
base::EscapeQueryParamValue(entity.guid().value(), /*use_plus=*/false));I'm not seeing a test-case that seems to cover what this ends up looking like (but the line does seem to be hit by tests, so there's probably an existing case we can add to for verifying).
Related details
- Florian Leimgruber
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. |
Vidhan Jain (Gerrit)
Vidhan Jain added 1 comment![]( "Open in Gerrit")
easier to move in the future.Could we add `GetWalletURL` to the EntityInstance data model, then it would be easier to reuse for other platforms?
Florian Leimgruber (Gerrit)
Florian Leimgruber voted and added 5 comments![]( "Open in Gerrit")
Votes added by Florian Leimgruber
| Commit-Queue | +1 |
Could we add `GetWalletURL` to the EntityInstance data model, then it would be easier to reuse for other platforms?
I was initially hesitant, because URLs are defined in `chrome/common/url_constants.h` and components/ cannot depend on chrome/.
In the end, I decided to move the logic go components/ after all, so it can be reused across platforms.
- kWalletPassesPageURL is temporarily duplicated there, until we launch deeps links for all pass types.
- kWalletPrivatePassPageURL is not part of the url_constants file and is not needed there.
Note that some Wallet and Autofill desktop saving UI still depends on `chrome::kWalletPassesPageURL`. The Autofill UI could be migrated to the new components/ constants, but for the Wallet one, I'm not sure what the right move it, since it probably shouldn't depend on Autofill.
Do we want a test case to cover this as well just for completeness? It's currently unreached by tests.
Done. As mentioned in the other comment, I moved the logic somewhere else. The tests are in autofill_ai_wallet_utils_unittest.cc.
return base::StringPrintf(
chrome::kWalletPrivatePassPageURL,
base::EscapeQueryParamValue(entity.guid().value(), /*use_plus=*/false));I'm not seeing a test-case that seems to cover what this ends up looking like (but the line does seem to be hit by tests, so there's probably an existing case we can add to for verifying).
Done. As mentioned in the other comment, I moved the logic somewhere else. The tests are in autofill_ai_wallet_utils_unittest.cc.
OpenWindowProxyImpl.getInstance().openUrl(e.model.item.walletEntityUrl!);Is this line exercised by any of the tests? If not, should there be a new test case for it?
Is this line exercised by any of the tests?
It seems like `onRemoteWalletPassesLinkClick_()` isn't tested at all right now. It was added without tests as `onGoToWalletClick_()` [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/6968624) ([reland](https://chromium-review.git.corp.google.com/c/chromium/src/+/7002760)) and later moved/renamed to `onRemoteWalletPassesLinkClick_()` [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7032102).
If not, should there be a new test case for it?
Modified an existing test to assert the button's behavior.
OpenWindowProxyImpl.getInstance().openUrl(e.model.item.walletEntityUrl!);Why is this safe to do? In other words what guarantees that this field even though marked as optional is always populated in this case?
For example if this is always populated for a certain type of EntityType, can you add an assertion to make that explicit?
Why is this safe to do? In other words what guarantees that this field even though marked as optional is always populated in this case?
As documented in the idl file:
> If the entity is `storedInWallet`, this string contains the URL to the management page of the pass on the Wallet website.
Looking at the [HTML file](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/settings/autofill_page/autofill_ai_entries_list.html;l=67-73;drc=b7409c8b043d83040608be9855552e4f1ccf36be) (which my CL doesn't modify), `onRemoteWalletPassesLinkClick_()` is only called if `storedInWallet`:
```
<template is="dom-if" if="[[item.storedInWallet]]" restamp>
<cr-icon-button class="icon-external" id="remoteWalletPassesLink"
title="$i18n{remoteWalletPassesLinkLabel}" role="link"
on-click="onRemoteWalletPassesLinkClick_"
aria-description="$i18n{opensInNewTab}">
</cr-icon-button>
</template>
```
The function name "RemoteWalletPasses" also indicates this.
For example if this is always populated for a certain type of EntityType, can you add an assertion to make that explicit?
- Demetrios Papadopoulos
- Norge Vizcay
- Tim
- Vidhan Jain
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. |
Tim (Gerrit)
Tim voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tim
| Code-Review | +1 |
1 comment
Thanks for adding the extra coverage in and I like that this was able to be slid into /components! IDL and extension API side LGTM!
Related details
- Demetrios Papadopoulos
- Florian Leimgruber
- Norge Vizcay
- Vidhan Jain
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
OpenWindowProxyImpl.getInstance().openUrl(e.model.item.walletEntityUrl!);Florian LeimgruberWhy is this safe to do? In other words what guarantees that this field even though marked as optional is always populated in this case?
For example if this is always populated for a certain type of EntityType, can you add an assertion to make that explicit?
Why is this safe to do? In other words what guarantees that this field even though marked as optional is always populated in this case?
As documented in the idl file:
> If the entity is `storedInWallet`, this string contains the URL to the management page of the pass on the Wallet website.Looking at the [HTML file](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/settings/autofill_page/autofill_ai_entries_list.html;l=67-73;drc=b7409c8b043d83040608be9855552e4f1ccf36be) (which my CL doesn't modify), `onRemoteWalletPassesLinkClick_()` is only called if `storedInWallet`:
```
<template is="dom-if" if="[[item.storedInWallet]]" restamp>
<cr-icon-button class="icon-external" id="remoteWalletPassesLink"
title="$i18n{remoteWalletPassesLinkLabel}" role="link"
on-click="onRemoteWalletPassesLinkClick_"
aria-description="$i18n{opensInNewTab}">
</cr-icon-button>
</template>
```The function name "RemoteWalletPasses" also indicates this.
For example if this is always populated for a certain type of EntityType, can you add an assertion to make that explicit?
Good point, added an assert for `storedInWallet`.
Good point, added an assert for storedInWallet.
Where was this added? I don't seet in in latest patch 15. Can you double check?
Once you add the assertion there should no longer be a need for the `!` cast. And more specifically this code makes an assumption about `walletEntityUrl` so the assertion should be about that, and not about `storedInWallet`. You can add both if that makes sense.
```suggestion
assert(e.model.item.storedInWallet);
assert(e.model.item.walletEntityUrl);
OpenWindowProxyImpl.getInstance().openUrl(e.model.item.walletEntityUrl);
```
Related details
Norge Vizcay (Gerrit)
Norge Vizcay voted Code-Review+1![]( "Open in Gerrit")
- Florian Leimgruber
- Vidhan Jain
Florian Leimgruber (Gerrit)
Florian Leimgruber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Florian Leimgruber
| Commit-Queue | +1 |
1 comment
OpenWindowProxyImpl.getInstance().openUrl(e.model.item.walletEntityUrl!);Where was this added? I don't seet in in latest patch 15. Can you double check?
I'm sorry, it must have gotten lost while I was working on the test.
Once you add the assertion there should no longer be a need for the ! cast. And more specifically this code makes an assumption about walletEntityUrl so the assertion should be about that, and not about storedInWallet. You can add both if that makes sense.
Done
Related details
- Demetrios Papadopoulos
- Vidhan Jain
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. |