import export bookmarks android #17821

tapanmodh · 2023-03-29T15:14:49Z

Resolves brave/brave-browser#6378

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Import Bookmarks:
Users can import an HTML file containing bookmarks via the bookmarks screen.

Export Bookmarks
Users should be able to export an HTML file containing bookmarks from the bookmarks screen.

device-2023-04-11-174934.mp4

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc

patches/chrome-browser-importer-profile_writer.cc.patch

patches/chrome-common-BUILD.gn.patch

android/java/org/chromium/chrome/browser/bookmarks/BraveBookmarkManagerMediator.java

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkManagerMediatorClassAdapter.java

chromium_src/chrome/utility/importer/bookmark_html_reader.cc

AlexeyBarabash

++

samartnik

lgtm

SergeyZhukovsky

++

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc

mkarolin · 2023-05-05T15:38:03Z

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc

+  base::FilePath file_export_path =
+      base::FilePath::FromUTF16Unsafe(export_path);
+
+  BookmarksExportObserver* observer = new FileBookmarksExportObserver(obj);


What's deleting this?

If we don't need to use the observer, then it seems like we can just pass in a nullptr like upstream does here.

we need to use this observer to get the result for exporting the bookmarks to display success or failure dialog to the user.

Oh I see, it deletes itself in OnExportFinished. Got it. Thanks.

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.h

common/sources.gni

mkarolin · 2023-05-10T15:55:37Z

common/sources.gni

+if (is_android) {
+  brave_common_sources = [
+    "//chrome/common/importer/imported_bookmark_entry.cc",
+    "//chrome/common/importer/imported_bookmark_entry.h",
+  ]
+}


Suggestion to minimize patching:

brave_common_sources = [] if (is_android) { brave_common_sources += [ "//chrome/common/importer/imported_bookmark_entry.cc", "//chrome/common/importer/imported_bookmark_entry.h", ] }

and then there would be no need to have the if (is_android) in patches/chrome-common-BUILD.gn.patch

Sure, I'll update it

mkarolin

LGTM

deeppandya

nit : we should have newline for all new classes.

deeppandya · 2023-05-10T21:47:15Z

android/brave_java_sources.gni

@@ -48,6 +48,7 @@ brave_java_sources = [
  "../../brave/android/java/org/chromium/chrome/browser/app/BraveActivity.java",
  "../../brave/android/java/org/chromium/chrome/browser/app/appmenu/AppMenuIconRowFooter.java",
  "../../brave/android/java/org/chromium/chrome/browser/app/appmenu/BraveAppMenuPropertiesDelegateImpl.java",
+  "../../brave/android/java/org/chromium/chrome/browser/app/bookmarks/BraveBookmarkActivity.java",


@tapanmodh can we move BraveBookmarkActivity to bookmark directory ?

BraveBookmarkActivity is in same place as BookmarkActivity from chromium. I think we shouldn't move to bookmark directory.

deeppandya · 2023-05-10T21:50:30Z

android/java/AndroidManifest.xml

+    android:theme="@style/Theme.Chromium.Activity.Fullscreen"
+    android:exported="false"
+    android:windowSoftInputMode="stateAlwaysHidden|adjustResize"
+    android:configChanges="orientation|keyboardHidden|keyboard|screenSize|mcc|mnc|screenLayout|smallestScreenSize"/>


Just curious, Why do we need MCC and MNC here ? i think it's useful when we need to have configChanges for mobile country code change.

I just use the same as BookmarkActivity has in chromium AndroidManifest

deeppandya · 2023-05-10T21:54:55Z

android/java/org/chromium/chrome/browser/app/bookmarks/BraveBookmarkActivity.java

+        if (mWindowAndroid.handlePermissionResult(requestCode, permissions, grantResults)) return;
+        super.onRequestPermissionsResult(requestCode, permissions, grantResults);
+    }
+}


nit : newline at the end

I think newline is there in the code

deeppandya · 2023-05-10T21:57:16Z

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkActivityClassAdapter.java

+        deleteField(sBraveBookmarkActivityClassName, "mBookmarkManagerCoordinator");
+        makeProtectedField(sBookmarkActivityClassName, "mBookmarkManagerCoordinator");
+    }
+}


nit : newline at the end

I think newline is there in the code

deeppandya · 2023-05-10T21:57:20Z

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkBridgeClassAdapter.java

+        deleteField(sBraveBookmarkBridgeClassName, "mNativeBookmarkBridge");
+        makeProtectedField(sBookmarkBridgeClassName, "mNativeBookmarkBridge");
+    }
+}


nit : newline at the end

I think newline is there in the code

deeppandya · 2023-05-10T21:57:37Z

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkManagerMediatorClassAdapter.java

+        deleteField(sBraveBookmarkManagerMediatorClassName, "mContext");
+        makeProtectedField(sBookmarkManagerMediatorClassName, "mContext");
+    }
+}


nit : newline at the end

deeppandya · 2023-05-10T21:57:43Z

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkModelClassAdapter.java

+        deleteMethod(sBraveBookmarkModelClassName, "importBookmarks");
+        deleteMethod(sBraveBookmarkModelClassName, "exportBookmarks");
+    }
+}


nit : newline at the end

deeppandya · 2023-05-10T21:57:47Z

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkPageClassAdapter.java

+        deleteField(sBraveBookmarkPageClassName, "mManager");
+        makeProtectedField(sBookmarkPageClassName, "mManager");
+    }
+}


nit : newline at the end

deeppandya · 2023-05-10T21:57:53Z

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkToolbarClassAdapter.java

+        deleteField(sBraveBookmarkToolbarClassName, "mBookmarkModel");
+        makeProtectedField(sBookmarkToolbarClassName, "mBookmarkModel");
+    }
+}


nit : newline at the end

deeppandya · 2023-05-10T21:58:09Z

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkToolbarCoordinatorClassAdapter.java

+        deleteField(sBraveBookmarkToolbarCoordinatorClassName, "mToolbar");
+        makeProtectedField(sBookmarkToolbarCoordinatorClassName, "mToolbar");
+    }
+}


nit : newline at the end

deeppandya

LGTM

github-actions bot assigned tapanmodh Mar 29, 2023

tapanmodh added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x86 CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 labels Mar 29, 2023

tapanmodh force-pushed the import_export_bookmarks_android branch 2 times, most recently from cd64552 to f2a05e0 Compare April 5, 2023 11:41

tapanmodh changed the title ~~wip - import export bookmarks android~~ import export bookmarks android Apr 5, 2023

tapanmodh force-pushed the import_export_bookmarks_android branch 2 times, most recently from b1cc736 to 979befd Compare April 6, 2023 19:44

tapanmodh added this to the 1.52.x - Nightly milestone Apr 6, 2023

tapanmodh marked this pull request as ready for review April 6, 2023 19:54

tapanmodh requested review from a team as code owners April 6, 2023 19:54

tapanmodh requested review from AlexeyBarabash, deeppandya and SergeyZhukovsky April 6, 2023 19:55

github-actions bot reviewed Apr 6, 2023

View reviewed changes

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc Show resolved Hide resolved

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc Show resolved Hide resolved

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc Show resolved Hide resolved

github-actions bot added the needs-security-review label Apr 6, 2023

github-actions bot assigned fmarier and thypon Apr 6, 2023

thypon removed the needs-security-review label Apr 7, 2023

thypon removed their assignment Apr 7, 2023

AlexeyBarabash reviewed Apr 7, 2023

View reviewed changes

patches/chrome-browser-importer-profile_writer.cc.patch Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Apr 7, 2023

View reviewed changes

patches/chrome-common-BUILD.gn.patch Outdated Show resolved Hide resolved

tapanmodh force-pushed the import_export_bookmarks_android branch 2 times, most recently from 983e5f2 to b1fddef Compare May 2, 2023 20:43

tapanmodh requested review from SergeyZhukovsky, samartnik and AlexeyBarabash May 3, 2023 16:43

SergeyZhukovsky reviewed May 3, 2023

View reviewed changes

android/java/org/chromium/chrome/browser/bookmarks/BraveBookmarkManagerMediator.java Outdated Show resolved Hide resolved

AlexeyBarabash reviewed May 4, 2023

View reviewed changes

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkManagerMediatorClassAdapter.java Outdated Show resolved Hide resolved

AlexeyBarabash reviewed May 4, 2023

View reviewed changes

build/android/bytecode/java/org/brave/bytecode/BraveBookmarkManagerMediatorClassAdapter.java Outdated Show resolved Hide resolved

AlexeyBarabash reviewed May 4, 2023

View reviewed changes

chromium_src/chrome/utility/importer/bookmark_html_reader.cc Outdated Show resolved Hide resolved

AlexeyBarabash approved these changes May 4, 2023

View reviewed changes

samartnik approved these changes May 4, 2023

View reviewed changes

SergeyZhukovsky approved these changes May 4, 2023

View reviewed changes

mkarolin reviewed May 5, 2023

View reviewed changes

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc Outdated Show resolved Hide resolved

mkarolin reviewed May 5, 2023

View reviewed changes

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.cc Show resolved Hide resolved

mkarolin reviewed May 5, 2023

View reviewed changes

chromium_src/chrome/browser/bookmarks/android/bookmark_bridge.h Show resolved Hide resolved

mkarolin reviewed May 5, 2023

View reviewed changes

common/sources.gni Outdated Show resolved Hide resolved

tapanmodh requested a review from mkarolin May 10, 2023 14:04

mkarolin reviewed May 10, 2023

View reviewed changes

mkarolin approved these changes May 10, 2023

View reviewed changes

import export bookmarks android

d4cfc48

tapanmodh force-pushed the import_export_bookmarks_android branch from 0dd1ffe to d4cfc48 Compare May 10, 2023 20:27

deeppandya reviewed May 10, 2023

View reviewed changes

tapanmodh modified the milestones: 1.52.x - Beta, 1.53.x - Nightly May 12, 2023

tapanmodh requested a review from deeppandya May 12, 2023 13:53

deeppandya approved these changes May 12, 2023

View reviewed changes

tapanmodh merged commit 87c07a9 into master May 13, 2023

tapanmodh deleted the import_export_bookmarks_android branch May 13, 2023 17:53

SergeyZhukovsky mentioned this pull request May 17, 2023

Crash in BraveBookmarkActivity brave/brave-browser#30408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import export bookmarks android #17821

import export bookmarks android #17821

tapanmodh commented Mar 29, 2023 •

edited

Loading

AlexeyBarabash left a comment

samartnik left a comment

SergeyZhukovsky left a comment

mkarolin May 5, 2023

mkarolin May 10, 2023

tapanmodh May 10, 2023

mkarolin May 10, 2023

mkarolin May 10, 2023 •

edited

Loading

tapanmodh May 10, 2023

mkarolin left a comment

deeppandya left a comment

deeppandya May 10, 2023

tapanmodh May 11, 2023

deeppandya May 10, 2023

tapanmodh May 11, 2023

deeppandya May 10, 2023

tapanmodh May 11, 2023

deeppandya May 10, 2023

tapanmodh May 11, 2023

deeppandya May 10, 2023

tapanmodh May 11, 2023

deeppandya May 10, 2023

deeppandya May 10, 2023

deeppandya May 10, 2023

deeppandya May 10, 2023

deeppandya May 10, 2023

deeppandya left a comment

import export bookmarks android #17821

import export bookmarks android #17821

Conversation

tapanmodh commented Mar 29, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

AlexeyBarabash left a comment

Choose a reason for hiding this comment

samartnik left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

tapanmodh commented Mar 29, 2023 •

edited

Loading

mkarolin May 10, 2023 •

edited

Loading