[Sync] Add an option in setting page to Enter Custom Sync Url #25484

jagadeshjai · 2024-09-07T06:12:01Z

Added an option in settings page to enter custom sync url which is specific to each profile.
Also handle the GPO for the same.
Rename brave_components_sync_driver_[sources/deps] -> brave_components_sync_service_[sources/deps]

Resolves brave/brave-browser#12314

Resolves brave/brave-browser#41280

UI:

brave_custom_sync_input.webm

After configuring policy

Two different Profiles

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 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:

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

Test Plan:

jagadeshjai · 2024-09-09T17:45:12Z

Hey @aguscruiz ! Could you confirm the design for this UI?

Kindly refer SC in Overview.

jagadeshjai · 2024-09-15T18:36:14Z

chromium_src/chrome/browser/sync/sync_service_factory.cc

 #include "brave/components/sync/service/brave_sync_service_impl.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/sync/device_info_sync_service_factory.h"
+#include "components/sync/base/command_line_switches.h"
+
+// Below includes are just to prevent redefining of


These included seems nasty,
@goodov Do we have any known alternatives to avoid this, other than direct patching?

yeah, this is too messy.

I think we should stop using syncer::kSyncServiceURL for setting the URL via the command line, but instead do this:

override syncer::internal::kSyncServerUrl and syncer::internal::kSyncDevServerUrl to be BUILDFLAG(BRAVE_SYNC_ENDPOINT).

modify BraveGetSyncServiceURL to also use the pref value if syncer::kSyncServiceURL flag is not passed (right now it's used only if the pref is managed).

remove syncer::kSyncServiceURL setting in //brave/app/brave_main_delegate.cc.

support AdjustSyncServiceUrlForAndroid in BraveGetSyncServiceURL.

cc @AlexeyBarabash ^ does this sound correct? aren't there any hidden shenanigans on iOS/Android if we go this way?

Sorry, @jagadeshjai @goodov I have conmpletly missed this PR and the question.

all above sounds correct and I see the code is now much cleaner.

No hidden shenanigans on Android.

iOS should also be ok. Just in case lets ask @Brandon-T, does iOS have any code to setup custom sync server url?

I just have tested the PR on Android and Desktop - works fine.

chromium_src/components/sync/base/DEPS

chromium_src/chrome/browser/sync/sync_service_factory.cc

browser/resources/settings/brave_sync_page/sync_url_input.ts

jagadeshjai · 2024-09-27T18:10:24Z

chromium_src/components/sync/service/sync_service_impl.cc

      }
    }
  }
+#else
+  const char kBraveSyncServiceStagingURL[] =


@AlexeyBarabash Could you please review these changes?

@jagadeshjai , done

I left two comments about the places worth to be improved.

Thanks @AlexeyBarabash 😄

chromium_src/components/sync/service/sync_service_impl.cc

AlexeyBarabash

LGTM for sync logic, but for sure it needs design review.

The URL edit field at the beginning may cause many questions from users.

AlexeyBarabash · 2024-09-30T11:01:45Z

@jagadeshjai
Just found that sometimes the URL edibox is displayed at device screen list:

jagadeshjai · 2024-10-01T17:28:34Z

@jagadeshjai Just found that sometimes the URL edibox is displayed at device screen list

Yes, Thanks for pointing out, will push the fix now!

goodov · 2024-10-08T09:10:31Z

@jagadeshjai

======= Failed test run #1 ==========
  FAILURE
  ------- Stdout: -------
  [ RUN      ] BraveMainDelegateUnitTest.DefaultCommandLineOverrides
  ../../brave/app/brave_main_delegate_unittest.cc:28: Failure
  Expected equality of these values:
    (("https://sync-v2.brave.com/v2"))
      Which is: "https://sync-v2.brave.com/v2
    command_line.GetSwitchValueASCII(syncer::kSyncServiceURL).c_str()
      Which is: ""
  Stack trace:
  #0 0x60bf182273b8 BraveMainDelegateUnitTest_DefaultCommandLineOverrides_Test::TestBody() [../../brave/app/brave_main_delegate_unittest.cc:38:3]
  [  FAILED  ] BraveMainDelegateUnitTest.DefaultCommandLineOverrides (1 ms)
  [3093/6896] BraveMainDelegateUnitTest.DefaultCommandLineOverrides (1 ms)
======= Failed test run #2 ==========
  FAILURE
  ------- Stdout: -------
  [ RUN      ] BraveMainDelegateUnitTest.DefaultCommandLineOverrides
  ../../brave/app/brave_main_delegate_unittest.cc:28: Failure
  Expected equality of these values:
    (("https://sync-v2.brave.com/v2"))
      Which is: "https://sync-v2.brave.com/v2
    command_line.GetSwitchValueASCII(syncer::kSyncServiceURL).c_str()
      Which is: ""
  Stack trace:
  #0 0x6444466e13b8 BraveMainDelegateUnitTest_DefaultCommandLineOverrides_Test::TestBody() [../../brave/app/brave_main_delegate_unittest.cc:38:3]
  [  FAILED  ] BraveMainDelegateUnitTest.DefaultCommandLineOverrides (0 ms)
  [6897/6901] BraveMainDelegateUnitTest.DefaultCommandLineOverrides (0 ms)

relaunch button when toogle/url change.

…ndling custom sync url. Also show Relauch button and Managed icon when required.

…ervice via command line before bulding its service in browser context.

…DevServerUrl using chromium_src overrides instead of passing it via command_line

…in Android to |BraveGetSyncServiceURL|.

…s_sync_service_[sources/deps]. And Remove some unused includes.

…and line

jagadeshjai · 2024-10-19T05:46:11Z

@jagadeshjai

======= Failed test run #1 ==========

Fixed 👍

jagadeshjai changed the title ~~[Sync] Add an option in setting page to Enable/Enter Custom Sync Url.~~ [Sync] [WIP] Add an option in setting page to Enable/Enter Custom Sync Url. Sep 8, 2024

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 8d7d7ff to f26c2e6 Compare September 8, 2024 19:18

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 2 times, most recently from e2da192 to 4fc9c3e Compare September 9, 2024 18:09

NicholasFlamy mentioned this pull request Sep 10, 2024

Group policy/settings: configuration for --sync-url= argument brave/brave-browser#20431

Closed

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 5 times, most recently from 7ddc422 to 8cc6efd Compare September 15, 2024 16:14

jagadeshjai changed the title ~~[Sync] [WIP] Add an option in setting page to Enable/Enter Custom Sync Url.~~ [Sync] [WIP] Add an option in setting page to Enter Custom Sync Url Sep 15, 2024

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 2 times, most recently from e0844cf to 6034292 Compare September 15, 2024 18:29

jagadeshjai commented Sep 15, 2024

View reviewed changes

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 6034292 to c42c7ba Compare September 15, 2024 18:38

jagadeshjai marked this pull request as ready for review September 15, 2024 18:38

jagadeshjai requested review from a team as code owners September 15, 2024 18:38

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 5 times, most recently from 2c9a8ce to 0d046ba Compare September 22, 2024 04:30

goodov reviewed Sep 23, 2024

View reviewed changes

chromium_src/components/sync/base/DEPS Outdated Show resolved Hide resolved

chromium_src/chrome/browser/sync/sync_service_factory.cc Outdated Show resolved Hide resolved

browser/resources/settings/brave_sync_page/sync_url_input.ts Show resolved Hide resolved

brave-builds mentioned this pull request Sep 23, 2024

CI run for contributor PR #25484 #25691

Draft

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 0d046ba to bf28c14 Compare September 25, 2024 03:04

jagadeshjai requested a review from a team as a code owner September 25, 2024 03:04

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from bf28c14 to 5d72880 Compare September 27, 2024 16:30

jagadeshjai requested a review from goodov September 27, 2024 16:51

jagadeshjai commented Sep 27, 2024

View reviewed changes

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 06fd867 to a0e1ef5 Compare September 27, 2024 18:25

AlexeyBarabash reviewed Sep 27, 2024

View reviewed changes

chromium_src/components/sync/service/sync_service_impl.cc Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Sep 27, 2024

View reviewed changes

chromium_src/components/sync/service/sync_service_impl.cc Show resolved Hide resolved

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from a0e1ef5 to 22a48d8 Compare September 28, 2024 06:17

jagadeshjai changed the title ~~[Sync] [WIP] Add an option in setting page to Enter Custom Sync Url~~ [Sync] Add an option in setting page to Enter Custom Sync Url Sep 28, 2024

jagadeshjai requested a review from AlexeyBarabash September 28, 2024 06:37

AlexeyBarabash mentioned this pull request Sep 30, 2024

Rename brave_components_sync_driver_* gn vars brave/brave-browser#41280

Open

AlexeyBarabash requested a review from aguscruiz September 30, 2024 09:03

AlexeyBarabash approved these changes Sep 30, 2024

View reviewed changes

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch 3 times, most recently from 88c82ea to ae64392 Compare October 8, 2024 01:36

bsclifton mentioned this pull request Oct 14, 2024

Change sync server to self-hosted sync server on mobile devices brave/brave-browser#41611

Closed

jagadeshjai added 8 commits October 17, 2024 13:41

[Sync] Add an option in setting page to Enable/Enter Custom Sync Url.

ae1827d

[Sync] Add a spearate input element for custom sync url and show

eab1859

relaunch button when toogle/url change.

[Sync] Remove Enable/Disable switch and just use input element for ha…

fd61ce9

…ndling custom sync url. Also show Relauch button and Managed icon when required.

[Sync] Pass the custom sync url from the profile preference to sync s…

22757d6

…ervice via command line before bulding its service in browser context.

[Sync] Set the |BRAVE_SYNC_ENDPOINT| directly in kSyncServerUrl/kSync…

8d890bd

…DevServerUrl using chromium_src overrides instead of passing it via command_line

[Android][Sync] Move the logic for changing the sync url for staging …

0d4c80c

…in Android to |BraveGetSyncServiceURL|.

Rename brave_components_sync_driver_[sources/deps] -> brave_component…

84b09d3

…s_sync_service_[sources/deps]. And Remove some unused includes.

[Sync] Remove test case for checking BRAVE_SYNC_ENDPOINT set via comm…

a8b34bc

…and line

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from ae64392 to a8b34bc Compare October 19, 2024 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

jagadeshjai commented Sep 7, 2024 •

edited

Loading

jagadeshjai commented Sep 9, 2024

jagadeshjai Sep 15, 2024

goodov Sep 16, 2024 •

edited

Loading

goodov Sep 16, 2024

AlexeyBarabash Sep 27, 2024

jagadeshjai Sep 27, 2024 •

edited

Loading

AlexeyBarabash Sep 27, 2024

jagadeshjai Sep 28, 2024

AlexeyBarabash left a comment

AlexeyBarabash commented Sep 30, 2024 •

edited

Loading

jagadeshjai commented Oct 1, 2024

goodov commented Oct 8, 2024

jagadeshjai commented Oct 19, 2024

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

Are you sure you want to change the base?

[Sync] Add an option in setting page to Enter Custom Sync Url #25484

Conversation

jagadeshjai commented Sep 7, 2024 • edited Loading

After configuring policy

Two different Profiles

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

jagadeshjai commented Sep 9, 2024

jagadeshjai Sep 15, 2024

Choose a reason for hiding this comment

goodov Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

goodov Sep 16, 2024

Choose a reason for hiding this comment

AlexeyBarabash Sep 27, 2024

Choose a reason for hiding this comment

jagadeshjai Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

AlexeyBarabash Sep 27, 2024

Choose a reason for hiding this comment

jagadeshjai Sep 28, 2024

Choose a reason for hiding this comment

AlexeyBarabash left a comment

Choose a reason for hiding this comment

AlexeyBarabash commented Sep 30, 2024 • edited Loading

jagadeshjai commented Oct 1, 2024

goodov commented Oct 8, 2024

jagadeshjai commented Oct 19, 2024

jagadeshjai commented Sep 7, 2024 •

edited

Loading

goodov Sep 16, 2024 •

edited

Loading

jagadeshjai Sep 27, 2024 •

edited

Loading

AlexeyBarabash commented Sep 30, 2024 •

edited

Loading