[iOS] Remove the need for exit-time destructors in some locations [chromium/src : main]
0 views
Skip to first unread message
Sylvain Defresne (Gerrit)
8:39 AM (11 hours ago) 8:39 AM
to Alexis Hétu, Chromium LUCI CQ, chromium...@chromium.org, ayman...@chromium.org, browser-comp...@chromium.org, dtraino...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-rev...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, mdjone...@chromium.org, net-r...@chromium.org, sloboda...@chromium.org, tmartino+tran...@chromium.org, yuezhang...@chromium.org
Attention needed from Alexis Hétu
Sylvain Defresne voted and added 5 comments![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
5 comments
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Sylvain Defresne . resolved
lgtm
Thank you for the cleanup :-)
File ios/chrome/browser/autofill/model/credit_card/autofill_save_card_infobar_delegate_unittest.mm
Line 39, Patchset 2 (Latest):
constexpr char kSaveCreditCardPromptResultIOSPrefix[] =Sylvain Defresne . unresolved
nit: I think you can use `std::string_view` here
```
constexpr std::string_view kSaveCreditCardPromptResultIOSPrefix =
"Autofill.SaveCreditCardPromptResult.IOS";
```
File ios/chrome/browser/autofill/ui_bundled/bottom_sheet/save_card_bottom_sheet_mediator_unittest.mm
Line 51, Patchset 2 (Latest):
constexpr char kSaveCreditCardPromptResultIOSPrefix[] =Sylvain Defresne . unresolved
ditto, I think we can use `constexpr std::string_view` for all those constants
File ios/chrome/browser/badges/ui_bundled/badge_mediator_unittest.mm
Line 43, Patchset 2 (Latest):
constexpr char16_t kFirstInfobarMessageText[] = u"FakeInfobarDelegate1";Sylvain Defresne . unresolved
nit: you can use `constexpr std::u16string_view` here by changing `AddInfobar(...)` into
```
InfoBarIOS* AddInfobar(InfobarType type, std::u16string_view message_text) {
std::unique_ptr<InfoBarIOS> added_infobar =
std::make_unique<FakeInfobarIOS>(type, std::u16string(message_text));
...
}
```
File ios/chrome/browser/bring_android_tabs/ui_bundled/tab_list_from_android_mediator_unittest.mm
Line 25, Patchset 2 (Latest):
const char* const kTestUrls[] = {"http://chromium.org", "http://google.com",Sylvain Defresne . unresolved
Please change to `std::array<std::string_view>` which is better. This will require a small change where `kTestUrls` is used.
```
constexpr auto kTestUrls = std::to_array<std::string_view>({
"http://chromium.org",
"http://google.com",
});
```
and then
```
for (std::string_view url : kTestUrls) {
std::unique_ptr<synced_sessions::DistantTab> tab =
std::make_unique<synced_sessions::DistantTab>();
tab->virtual_url = GURL(url);
tabs.push_back(std::move(tab));
}
```
Related details
Attention is currently required from:
- Alexis Hétu
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. |
Sylvain Defresne (Gerrit)
8:39 AM (11 hours ago) 8:39 AM
to Alexis Hétu, Chromium LUCI CQ, chromium...@chromium.org, ayman...@chromium.org, browser-comp...@chromium.org, dtraino...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-rev...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, mdjone...@chromium.org, net-r...@chromium.org, sloboda...@chromium.org, tmartino+tran...@chromium.org, yuezhang...@chromium.org
Attention needed from Alexis Hétu
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Sylvain Defresne . resolved
Note: please ignore my comments if they require changing other files (I don't want you to lose my +1) but some of them can be done just locally, so may be worth doing
Related details
Attention is currently required from:
- Alexis Hétu
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. |
Alexis Hétu (Gerrit)
9:17 AM (10 hours ago) 9:17 AM
to Sylvain Defresne, Chromium LUCI CQ, chromium...@chromium.org, ayman...@chromium.org, browser-comp...@chromium.org, dtraino...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-rev...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, mdjone...@chromium.org, net-r...@chromium.org, sloboda...@chromium.org, tmartino+tran...@chromium.org, yuezhang...@chromium.org
Alexis Hétu voted and added 5 comments![]( "Open in Gerrit")
Votes added by Alexis Hétu
| Commit-Queue | +2 |
Sylvain Defresne . resolved
Note: please ignore my comments if they require changing other files (I don't want you to lose my +1) but some of them can be done just locally, so may be worth doing
Alexis Hétu
All done, thanks!
File ios/chrome/browser/autofill/model/credit_card/autofill_save_card_infobar_delegate_unittest.mm
Line 39, Patchset 2:
constexpr char kSaveCreditCardPromptResultIOSPrefix[] =Sylvain Defresne . resolved
nit: I think you can use `std::string_view` here
```
constexpr std::string_view kSaveCreditCardPromptResultIOSPrefix =
"Autofill.SaveCreditCardPromptResult.IOS";
```
Alexis Hétu
Done
File ios/chrome/browser/autofill/ui_bundled/bottom_sheet/save_card_bottom_sheet_mediator_unittest.mm
Line 51, Patchset 2:
constexpr char kSaveCreditCardPromptResultIOSPrefix[] =Sylvain Defresne . resolved
ditto, I think we can use `constexpr std::string_view` for all those constants
Alexis Hétu
Done
File ios/chrome/browser/badges/ui_bundled/badge_mediator_unittest.mm
Line 43, Patchset 2:
constexpr char16_t kFirstInfobarMessageText[] = u"FakeInfobarDelegate1";Sylvain Defresne . resolved
nit: you can use `constexpr std::u16string_view` here by changing `AddInfobar(...)` into
```
InfoBarIOS* AddInfobar(InfobarType type, std::u16string_view message_text) {
std::unique_ptr<InfoBarIOS> added_infobar =
std::make_unique<FakeInfobarIOS>(type, std::u16string(message_text));
...
}
```
Alexis Hétu
Done
File ios/chrome/browser/bring_android_tabs/ui_bundled/tab_list_from_android_mediator_unittest.mm
Line 25, Patchset 2:
const char* const kTestUrls[] = {"http://chromium.org", "http://google.com",Sylvain Defresne . resolved
Please change to `std::array<std::string_view>` which is better. This will require a small change where `kTestUrls` is used.
```
constexpr auto kTestUrls = std::to_array<std::string_view>({
"http://chromium.org",
"http://google.com",
});
```and then
```
for (std::string_view url : kTestUrls) {
std::unique_ptr<synced_sessions::DistantTab> tab =
std::make_unique<synced_sessions::DistantTab>();
tab->virtual_url = GURL(url);
tabs.push_back(std::move(tab));
}
```
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
10:26 AM (9 hours ago) 10:26 AM
to Alexis Hétu, Sylvain Defresne, chromium...@chromium.org, ayman...@chromium.org, browser-comp...@chromium.org, dtraino...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-rev...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org, mdjone...@chromium.org, net-r...@chromium.org, sloboda...@chromium.org, tmartino+tran...@chromium.org, yuezhang...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ios/chrome/browser/autofill/ui_bundled/bottom_sheet/save_card_bottom_sheet_mediator_unittest.mm
Insertions: 6, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/bring_android_tabs/ui_bundled/tab_list_from_android_mediator_unittest.mm
Insertions: 3, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/badges/ui_bundled/badge_mediator_unittest.mm
Insertions: 8, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/autofill/model/credit_card/autofill_save_card_infobar_delegate_unittest.mm
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Commit message:
[iOS] Remove the need for exit-time destructors in some locations
This CL removes the need for exit time destructors in iOS code
in a few locations where it is trivial to do so.
Bug: 40031409
Change-Id: Id5a22fc924e6502c27ad03c7742c1aeb87630e91
Reviewed-by: Sylvain Defresne <sdef...@chromium.org>
Commit-Queue: Alexis Hétu <su...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1571196}
Files:
- M ios/chrome/app/spotlight/BUILD.gn
- M ios/chrome/app/spotlight/bookmark_spotlight_manager_unittest.mm
- M ios/chrome/browser/autofill/model/credit_card/BUILD.gn
- M ios/chrome/browser/autofill/model/credit_card/autofill_save_card_infobar_delegate_unittest.mm
- M ios/chrome/browser/autofill/ui_bundled/bottom_sheet/BUILD.gn
- M ios/chrome/browser/autofill/ui_bundled/bottom_sheet/save_card_bottom_sheet_mediator_unittest.mm
- M ios/chrome/browser/badges/ui_bundled/BUILD.gn
- M ios/chrome/browser/badges/ui_bundled/badge_mediator_unittest.mm
- M ios/chrome/browser/bring_android_tabs/ui_bundled/BUILD.gn
- M ios/chrome/browser/bring_android_tabs/ui_bundled/tab_list_from_android_mediator_unittest.mm
- M ios/chrome/browser/commerce/model/push_notification/BUILD.gn
- M ios/chrome/browser/commerce/model/push_notification/commerce_push_notification_client_unittest.mm
- M ios/chrome/browser/download/model/BUILD.gn
- M ios/chrome/browser/download/model/download_filter_util.cc
- M ios/chrome/browser/infobars/model/overlays/BUILD.gn
- M ios/chrome/browser/infobars/model/overlays/infobar_overlay_request_inserter_unittest.mm
- M ios/chrome/browser/overlays/ui_bundled/infobar_banner/sync_error/BUILD.gn
- M ios/chrome/browser/overlays/ui_bundled/infobar_banner/sync_error/sync_error_infobar_banner_overlay_mediator_unittest.mm
- M ios/chrome/browser/price_insights/model/BUILD.gn
- M ios/chrome/browser/price_insights/model/price_insights_model_unittest.mm
- M ios/chrome/browser/price_insights/ui/BUILD.gn
- M ios/chrome/browser/price_insights/ui/price_insights_cell_unittest.mm
- M ios/chrome/browser/sessions/model/BUILD.gn
- M ios/chrome/browser/snapshots/model/BUILD.gn
- M ios/chrome/browser/snapshots/model/snapshot_generator_unittest.mm
- M ios/web/BUILD.gn
- M ios/web/js_messaging/BUILD.gn
- M ios/web/js_messaging/web_frames_manager_impl_unittest.mm
- M ios/web/net/cookies/BUILD.gn
Change size: M
Delta: 29 files changed, 46 insertions(+), 111 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Sylvain Defresne
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages