[ios] Migrate bookmark folder chooser away from LegacyBookmarkModel [chromium/src : main]
0 views
Skip to first unread message
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 1, 2024, 5:12:52 PM (3 days ago) Jul 1
to Mikel Astiz, Chromium LUCI CQ, chromium...@chromium.org, browser-comp...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Mikel Astiz
findit...@appspot.gserviceaccount.com voted Code-Coverage-1![]( "Open in Gerrit")
This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.
- //ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.mm
- //ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_coordinator.mm
- //ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_mediator.mm
Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe
| Code-Coverage | -1 |
Related details
Attention is currently required from:
- Mikel Astiz
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 3, 2024, 3:36:04 PM (2 days ago) Jul 3
to Mikel Astiz, Chromium LUCI CQ, chromium...@chromium.org, browser-comp...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Mikel Astiz
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Attention is currently required from:
- Mikel Astiz
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jérôme Lebel (Gerrit)
Jul 4, 2024, 4:20:36 AM (yesterday) Jul 4
to Mikel Astiz, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, browser-comp...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Attention needed from Mikel Astiz
Jérôme Lebel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jérôme Lebel
| Code-Review | +1 |
2 comments
File ios/chrome/browser/bookmarks/model/bookmarks_utils.cc
Line 72, Patchset 3 (Latest):
DCHECK(model);Jérôme Lebel . unresolved
Shouldn't we `CHECK` everywhere?
File ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl.h
Line 47, Patchset 3 (Latest):
withType:(BookmarkModelType)typeJérôme Lebel . unresolved
Only one `with` should exist in a method name.
Related details
Attention is currently required from:
- Mikel Astiz
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Jul 4, 2024, 1:12:14 PM (15 hours ago) Jul 4
to Jérôme Lebel, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, browser-comp...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Mikel Astiz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +2 |
3 comments
Patchset-level comments
File ios/chrome/browser/bookmarks/model/bookmarks_utils.cc
Shouldn't we `CHECK` everywhere?
Mikel Astiz
Done
File ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl.h
Only one `with` should exist in a method name.
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 4, 2024, 2:04:53 PM (14 hours ago) Jul 4
to Mikel Astiz, Jérôme Lebel, Chromium LUCI CQ, chromium...@chromium.org, browser-comp...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jul 4, 2024, 2:05:08 PM (14 hours ago) Jul 4
to Mikel Astiz, Jérôme Lebel, findit...@appspot.gserviceaccount.com, chromium...@chromium.org, browser-comp...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 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/bookmarks/model/bookmarks_utils.cc
Insertions: 6, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h
Insertions: 1, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_mediator.mm
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl.mm
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl_unittest.mm
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Commit message:
[ios] Migrate bookmark folder chooser away from LegacyBookmarkModel
Usage of LegacyBookmarkModel is getting replaced with
bookmarks::BookmarkModel. This patch migrates occurrences related to
the bookmark folder chooser, requiring a few simple changes to in the
utility libraries it relies on.
The change is non-trivial because the folder chooser UI shows local and
account bookmarks separately, and has special logic for each. Among
other considerations, this manifests in code as PrimaryPermanentNodes()
being used in BookmarksFolderChooserSubDataSourceImpl, the latter being
instantiated twice, once for local bookmarks and once for account ones.
Before this patch, the selection of relevant nodes was achieved
seamlessly by LegacyBookmarkModel. After this patch, the selection is
done explicitly in PrimaryPermanentNodes(), which is extended to take
one more parameter, enum BookmarkModelType, to distinguish the two
cases.
Bug: 346918509
Change-Id: I9c352bab63814a19bf4891ba40b9248c38ed5d1e
Reviewed-by: Jérôme Lebel <jle...@chromium.org>
Commit-Queue: Mikel Astiz <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1323425}
Files:
- M ios/chrome/browser/bookmarks/model/bookmark_model_type.h
- M ios/chrome/browser/bookmarks/model/bookmarks_utils.cc
- M ios/chrome/browser/bookmarks/model/bookmarks_utils.h
- M ios/chrome/browser/bookmarks/model/bookmarks_utils_unittest.mm
- M ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.h
- M ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios.mm
- M ios/chrome/browser/bookmarks/ui_bundled/bookmark_utils_ios_unittest.mm
- M ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/BUILD.gn
- M ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_coordinator.mm
- M ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_mediator.h
- M ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_mediator.mm
- A ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_mediator_unittest.mm
- M ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl.h
- M ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl.mm
- M ios/chrome/browser/bookmarks/ui_bundled/folder_chooser/bookmarks_folder_chooser_sub_data_source_impl_unittest.mm
Change size: L
Delta: 15 files changed, 374 insertions(+), 215 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Jérôme Lebel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages