Support iOS Syncing & Bookmarks #6722

Brandon-T · 2020-09-29T15:44:24Z

fix brave/brave-browser#12971

Adds support for iOS Sync chain with Brave Desktop.
Adds support for iOS Chromium bookmarks so we can sync them with Desktop.
Adds better compatibility with iOS and Chromium core in general.
All interfaces that are native to iOS must be explicitly exported with OBJC_EXPORT

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

ios/browser/api/bookmarks/brave_bookmarks_api.h

kylehickinson

Super basic first pass, let me know if any of the nits I had conflict with Chromium guidelines

ios/browser/api/bookmarks/brave_bookmarks_api.h

ios/app/brave_core_main.mm

ios/browser/api/bookmarks/brave_bookmarks_observer.mm

ios/browser/api/sync/brave_sync_api.h

ios/browser/api/sync/brave_sync_worker.cc

ios/browser/brave_web_main_parts.h

vendor/brave-ios/Ledger/BATBraveLedger.mm

kylehickinson · 2020-09-29T16:48:31Z

vendor/brave-ios/objc-gen/objc-gen/Interface.swift

@@ -38,6 +38,7 @@ struct Interface: Hashable, Comparable {
  /// The generated output of a this @interface
  var generatedPublicInterface: String {
    return """
+    OBJC_EXPORT


This file isn't even used anymore :) So this change can be removed. In fact, if you want you can completely delete objc-gen since its done by mojom now

I think that should probably be a separate PR

ios/browser/api/bookmarks/brave_bookmarks_api.h

ios/browser/api/bookmarks/brave_bookmarks_api.mm

ios/browser/api/bookmarks/brave_bookmarks_observer.h

bridiver · 2020-10-09T17:33:01Z

vendor/brave-ios/BUILD.gn

@@ -19,6 +20,7 @@ config("internal_config") {
    "-Wno-objc-property-synthesis",
    "-Wno-sign-compare",
  ]
+  ldflags = [ "-Wl,-no_compact_unwind" ]


this is needed for rust, correct? If so, we should add a rust config and do configs += so we can better track what these are used for

patches/base-sequence_checker.h.patch

chromium_src/components/sync_device_info/device_info.cc

ios/app/brave_main_delegate.mm

ios/browser/api/sync/brave_sync_worker.cc

ios/browser/api/bookmarks/exporter/bookmark_html_writer.cc

diracdeltas · 2020-10-29T23:22:30Z

ios/browser/api/bookmarks/exporter/bookmark_html_writer.cc

+    int64_t internal_value;
+    base::StringToInt64(time_string, &internal_value);
+    return Write(base::NumberToString(
+        base::Time::FromInternalValue(internal_value).ToTimeT()));


FromInternalValue is not recommended anymore (https://source.chromium.org/chromium/chromium/src/+/master:base/time/time.h;l=769?q=FromInternalValue&ss=chromium) and may be deprecated at some point, so we should use something else for deserialization. maybe https://source.chromium.org/chromium/chromium/src/+/master:base/time/time.h;l=617?q=FromDeltaSinceWindowsEpoch&ss=chromium ?

@diracdeltas this is code copied from upstream, I recommend we keep it as-is to avoid compatibility issues

diracdeltas · 2020-10-29T23:37:46Z

ios/browser/api/bookmarks/exporter/bookmarks_encoder.cc

+BookmarkEncoder::~BookmarkEncoder() = default;
+
+void BookmarkEncoder::UpdateChecksum(const std::string& str) {
+  base::MD5Update(&md5_context_, str);


md5 is not secure against an attacker who can modify the input to it. it should be ok if it's only used here to check if some data has changed, not for a security-related check.

@diracdeltas as with the other code I commented on, this is copied from upstream. In this case we should actually be using the original BookmarkCodec class as I mentioned above

bridiver · 2020-12-01T00:45:38Z

ios/browser/api/bookmarks/exporter/brave_bookmarks_exporter.mm

+            GetApplicationContext()->GetChromeBrowserStateManager();
+        DCHECK(browserStateManager);
+
+        ChromeBrowserState* chromeBrowserState =


this should be passed in to the constructor

…parent folders into child folders.

ios/app/brave_main_delegate.mm

bridiver · 2020-12-01T18:04:44Z

ios/browser/api/bookmarks/brave_bookmarks_api.h

@@ -73,6 +73,8 @@ OBJC_EXPORT
 @property(nonatomic, readonly) NSUInteger childCount;

 - (nullable IOSBookmarkNode*)childAtIndex:(NSUInteger)index;
+- (NSArray<BookmarkFolder*>*)nestedChildFoldersFiltered:(BOOL(^)(BookmarkFolder*))included


I'm not following what this is supposed to do. Filtered for what? Is there a similar method for android? I definitely think this method needs some comments or a name change because it's not intuitive what is going on here or why this is needed to prevent users from moving parent folders into child folders

Removed duplicate code.

…nstead of Unknown.

kylehickinson · 2020-12-01T18:37:40Z

ios/browser/api/bookmarks/brave_bookmarks_api.h

+- (void)moveToParent:(nonnull IOSBookmarkNode*)parent;
+- (void)moveToParent:(nonnull IOSBookmarkNode*)parent index:(NSUInteger)index;
+- (NSInteger)indexOfChild:(nonnull IOSBookmarkNode*)child;
+- (bool)hasAncestor:(nonnull IOSBookmarkNode*)parent;


Redundant nonnull additions

kylehickinson · 2020-12-01T18:39:19Z

build/commands/lib/config.js

-    args.enable_stripping = args.enable_dsyms
-    args.use_xcode_clang = this.isOfficialBuild()
+    args.enable_stripping = !this.isDebug()
+    args.use_xcode_clang = false


This is an intentional change, right? Does the 1.15/1.16 forks have this change? Just want to make sure we don't end up having issues submitting to Apple

The change is intentional. This is what Chromium does for Xcode 12.0+. I explicitly changed this to false for use_xcode_clang. I have not tested submitting to Apple's servers but it did pass verification locally.

bridiver · 2020-12-01T20:29:11Z

win had an unrelated intermittent failure

Brandon-T requested review from bridiver, kylehickinson and tmancey September 29, 2020 15:44

Brandon-T force-pushed the ios-sync-brandon-bj branch from 00eb778 to c1d1fbb Compare September 29, 2020 16:10

kylehickinson reviewed Sep 29, 2020

View reviewed changes

ios/browser/api/bookmarks/brave_bookmarks_api.h Outdated Show resolved Hide resolved

kylehickinson requested changes Sep 29, 2020

View reviewed changes

bsclifton requested a review from jumde September 29, 2020 17:17

kylehickinson requested changes Oct 1, 2020

View reviewed changes

Brandon-T force-pushed the ios-sync-brandon-bj branch 2 times, most recently from e733c17 to 839ff25 Compare October 2, 2020 16:42

kylehickinson reviewed Oct 2, 2020

View reviewed changes

ios/browser/api/bookmarks/brave_bookmarks_observer.h Outdated Show resolved Hide resolved

Brandon-T force-pushed the ios-sync-brandon-bj branch from 839ff25 to e5b94d0 Compare October 4, 2020 23:07

bridiver reviewed Oct 9, 2020

View reviewed changes

patches/base-sequence_checker.h.patch Outdated Show resolved Hide resolved

bridiver reviewed Oct 9, 2020

View reviewed changes

chromium_src/components/sync_device_info/device_info.cc Outdated Show resolved Hide resolved

Brandon-T requested a review from a team as a code owner October 13, 2020 16:09

Brandon-T force-pushed the ios-sync-brandon-bj branch from fc100ef to 5de403d Compare October 13, 2020 20:52

bridiver reviewed Oct 14, 2020

View reviewed changes

ios/app/brave_main_delegate.mm Outdated Show resolved Hide resolved

Brandon-T mentioned this pull request Oct 15, 2020

iOS - Sync 1.15.x - Uplift #6866

Closed

33 tasks

bsclifton force-pushed the ios-sync-brandon-bj branch 3 times, most recently from c39a612 to be4340f Compare October 21, 2020 17:38

bsclifton mentioned this pull request Oct 21, 2020

Add Sync v2 support for bookmarks brave/brave-ios#2946

Closed

bsclifton force-pushed the ios-sync-brandon-bj branch from be4340f to 2123454 Compare October 22, 2020 17:46

Brandon-T force-pushed the ios-sync-brandon-bj branch from 25eddb4 to b88668d Compare October 27, 2020 20:13

darkdh reviewed Oct 29, 2020

View reviewed changes

ios/browser/api/sync/brave_sync_worker.cc Outdated Show resolved Hide resolved

diracdeltas reviewed Oct 29, 2020

View reviewed changes

ios/browser/api/bookmarks/exporter/bookmark_html_writer.cc Show resolved Hide resolved

diracdeltas reviewed Oct 29, 2020

View reviewed changes

ios/browser/api/bookmarks/exporter/bookmark_html_writer.cc Show resolved Hide resolved

diracdeltas reviewed Oct 29, 2020

View reviewed changes

bridiver added 2 commits November 30, 2020 15:34

cleanup

701995e

remove incorrect header

bbb22dc

bridiver reviewed Dec 1, 2020

View reviewed changes

bridiver and others added 3 commits November 30, 2020 17:48

clang format cleanup

5f447ac

Fixed Xcode 12 compiling and sync service URL to be retrieved from GN.

8464de7

Added folder filtering for the iOS side to prevent users from moving …

4f47f85

…parent folders into child folders.

bridiver reviewed Dec 1, 2020

View reviewed changes

ios/app/brave_main_delegate.mm Outdated Show resolved Hide resolved

bridiver reviewed Dec 1, 2020

View reviewed changes

Rename functions and add documentation explaining it.

46a8534

Removed duplicate code.

Brandon-T force-pushed the ios-sync-brandon-bj branch from 69353a2 to 46a8534 Compare December 1, 2020 18:17

bridiver and others added 2 commits December 1, 2020 11:26

make sure brave_sync_endpoint is not empty for official builds

91aeecd

Fixed iOS mobile user agent for Sync UI so it shows "iOS" correctly i…

c885282

…nstead of Unknown.

kylehickinson reviewed Dec 1, 2020

View reviewed changes

kylehickinson approved these changes Dec 1, 2020

View reviewed changes

bridiver approved these changes Dec 1, 2020

View reviewed changes

bridiver merged commit f3ce528 into master Dec 1, 2020

bridiver deleted the ios-sync-brandon-bj branch December 1, 2020 20:29

bridiver added this to the 1.20.x - Nightly milestone Dec 1, 2020

bridiver added the feature/sync label Dec 1, 2020

mkarolin mentioned this pull request Dec 7, 2020

chromium_src/components/content_settings/core/browser/cookie_settings_utils.cc is overriding a wrong path. brave/brave-browser#13078

Closed

soner-yuksel added CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 and removed CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels May 19, 2021

mkarolin mentioned this pull request Jun 24, 2022

Upgrade from Chromium 103 to Chromium 104. #13498

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support iOS Syncing & Bookmarks #6722

Support iOS Syncing & Bookmarks #6722

Brandon-T commented Sep 29, 2020 •

edited by bridiver

Loading

kylehickinson left a comment

kylehickinson Sep 29, 2020

bridiver Oct 9, 2020

bridiver Oct 9, 2020

diracdeltas Oct 29, 2020

bridiver Nov 11, 2020

diracdeltas Oct 29, 2020

bridiver Nov 11, 2020

bridiver Dec 1, 2020

bridiver Dec 1, 2020

kylehickinson Dec 1, 2020

kylehickinson Dec 1, 2020

Brandon-T Dec 1, 2020

bridiver commented Dec 1, 2020

Support iOS Syncing & Bookmarks #6722

Support iOS Syncing & Bookmarks #6722

Conversation

Brandon-T commented Sep 29, 2020 • edited by bridiver Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

kylehickinson 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

bridiver commented Dec 1, 2020

Brandon-T commented Sep 29, 2020 •

edited by bridiver

Loading