Conversation
There was a problem hiding this comment.
There are many undocumented changes that require some further expounding on why certain codepaths are excluded, or why strings are hardcoded, along with the rationale on the source file list used for Android. Reviewed portions of the patch is nonexhaustive.
quh4gko8
left a comment
quh4gko8
left a comment
There was a problem hiding this comment.
Please squash the commits and consolidate the changes to allow for easier review.
|
Added a commit comment explaining the build structure and execution flow in some detail. Added some code comments.
Enabled all strings for android. initially i mistakenly thought figuring the build deps for those would be a chunk of work, which is not the case. Many of them should be unused, since we run only bookmarks path. Some of the .gn and .grd changes are somewhat awkward A brief look at Brave shows they are running the upstreams ContentBookmarkParser in the browser process. That's brave for sure! |
|
Tested to apply cleanly to 143.0.7499.52 |
Thanks, need to re-test and rebase it on 144.0.7559.90, the current latest Chromium version Vanadium is based on. |
$ git am -3 ../patches/0236-Import-export-bookmarks.patch Applying: Import export bookmarks Using index info to reconstruct a base tree... M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java M chrome/app/bookmarks_strings.grdp M chrome/app/generated_resources.grd M chrome/browser/BUILD.gn M chrome/browser/bookmarks/android/bookmark_bridge.cc M chrome/browser/bookmarks/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkToolbarMediator.java M chrome/browser/importer/in_process_importer_bridge.cc M chrome/browser/ui/android/strings/android_chrome_strings.grd M chrome/utility/services.cc Falling back to patching base and 3-way merge... Auto-merging chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java Auto-merging chrome/app/bookmarks_strings.grdp Auto-merging chrome/app/generated_resources.grd Auto-merging chrome/browser/BUILD.gn Auto-merging chrome/browser/bookmarks/android/bookmark_bridge.cc Auto-merging chrome/browser/bookmarks/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkToolbarMediator.java Auto-merging chrome/browser/importer/in_process_importer_bridge.cc Auto-merging chrome/browser/ui/android/strings/android_chrome_strings.grd Auto-merging chrome/utility/services.cc
|
62cd408 updates to 144.0.7559.90 |
|
It's currently treated as untested and it will require further testing for the current version. It may not necessarily work for the current version and may require further fixes. |
|
The latest version can be built in separate checkout, if space permits, to allow keeping both trees, otherwise it can wait until it has been further tested. It's also recommended to add tests to run to check if the current import/export infrastructure works as intended, but manual testing is acceptable given the hardware limitations. |
|
Now tested with a 144.0.7559.90 release build, its OK. |
|
There are currently issues on its functionality, such as importing bookmakers not supposed to request for Camera permission at all to pick the file for importing permission. It breaks the overall import/export functionality, turns no-op if the Camera permission is not granted. Can that be resolved first? |
… the camera permisions.
|
enableStrictModeWatch is here by mistake and can be removed. Initially I had an issue, which I later solved by passing the java window to the c++ helper. |
| + <item | ||
| + android:id="@+id/import_export_menu_id" | ||
| + android:icon="@drawable/ic_more_vert_24dp" | ||
| + android:title="@string/create_new_folder" |
There was a problem hiding this comment.
this title seems incorrect (new folder vs import/export)
3 participants
This change adds two menu items to the bookmarks tab for importing / exporting bookmarks (#510). Applies to chromium 143.0.7499.34
A minimal implementation in bookmark_import_export_helper.cc based on bookmark_manager_private_api.cc where we run a native
chrome:mojo::ProfileImportservice in a utility process to do the reading of the bookmarks file. The java side is responsible only for handling the menu clicks and fetching the underlygingWindowAndroid.@vanadium happy to receive feedback and iterate.