[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:

Make sure Sync is turned OFF and Custom sync URL option is shown in brave://settings/braveSync
Enter the Custom Sync URL and it should ask for relaunch
Turn on the sync chain.
Now Custom Sync URL should be disabled with the given value.
Navigate to brave://sync-internals and check whether the given URL is assigned in Server URL (Under Version info - left side)

Note: If sync already turned ON and Custom sync URL is empty then the Custom sync URL option should be hidden.

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

goodov

please also rebase and regenerate patches.

patches/components-sync-base-BUILD.gn.patch

relaunch button when toogle/url change.

…ndling custom sync url. Also show Relauch button and Managed icon when required. Conflicts: browser/ui/webui/brave_settings_ui.cc

…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

…ock when sync already started and the custom sync url is empty. Also disable the 'Custom sync URL' block when sync started.

jagadeshjai · 2024-11-27T09:22:30Z

@goodov Can you run the CI now?

goodov · 2024-11-27T14:09:15Z

@bsclifton CI run is green #25691

This can be merged.

bsclifton · 2024-11-27T21:44:40Z

Huge thanks for sticking with this PR @jagadeshjai! 🎉

Really great to have this change finally 😄👍

brave-builds · 2024-11-28T00:04:56Z

Released in v1.75.58

jagadeshjai · 2024-11-28T17:26:17Z

Thanks All ❤️ 😄 !

…n_custom_sync_url" This reverts commit d547c86 and 9757d93, reversing changes made to ab6eba7.

* Revert "Merge pull request #25484 from jagadeshjai/feature__add_option_custom_sync_url" This reverts commit d547c86 and 9757d93, reversing changes made to ab6eba7. * revert #27054 * fixed presubmit errors * Removed code supposed to support `Use Sync staging server` QA preference Related PR - #26804 --------- Co-authored-by: AlexeyBarabash <[email protected]> Co-authored-by: AlexeyBarabash <[email protected]>

bsclifton · 2025-01-08T19:08:32Z

@jagadeshjai quick update- we discovered a few issues unfortunately while folks were reviewing (post-merge). Because those issues had security implications, this work was reverted with #27136

Something we missed and I think this is on me - is making sure we had a security review before merge. I'll try to help with the next steps so this doesn't get lost. We'll want to make a new issue to track the request (and not re-open brave/brave-browser#12314).

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

Merged

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

goodov reviewed Nov 27, 2024

View reviewed changes

patches/components-sync-base-BUILD.gn.patch Outdated Show resolved Hide resolved

jagadeshjai and others added 11 commits November 27, 2024 13:49

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

8775f05

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

ed3323d

relaunch button when toogle/url change.

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

7731484

…ndling custom sync url. Also show Relauch button and Managed icon when required. Conflicts: browser/ui/webui/brave_settings_ui.cc

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

48497e8

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

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

30c781e

…DevServerUrl using chromium_src overrides instead of passing it via command_line

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

76494c7

…in Android to |BraveGetSyncServiceURL|.

Rename brave_components_sync_driver_[sources/deps] -> brave_component…

03fe158

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

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

b241d17

…and line

FIXUP: small edits for CI and string changes

ff3cb90

[Sync] Add tooltip for policy indicator and hide 'Custom sync URL' bl…

b002090

…ock when sync already started and the custom sync url is empty. Also disable the 'Custom sync URL' block when sync started.

[Sync] Update patches

c4b0885

jagadeshjai force-pushed the feature__add_option_custom_sync_url branch from 6226566 to c4b0885 Compare November 27, 2024 08:35

goodov approved these changes Nov 27, 2024

View reviewed changes

bsclifton merged commit d547c86 into brave:master Nov 27, 2024
16 of 19 checks passed

bsclifton assigned jagadeshjai Nov 27, 2024

bsclifton added this to the 1.75.x - Nightly milestone Nov 27, 2024

jagadeshjai deleted the feature__add_option_custom_sync_url branch November 28, 2024 17:26

goodov mentioned this pull request Dec 18, 2024

Default sync server URL to staging URL if not set in .env brave/brave-browser#42945

Closed

bridiver added a commit that referenced this pull request Jan 6, 2025

Revert "Merge pull request #25484 from jagadeshjai/feature__add_optio…

da1e94b

…n_custom_sync_url" This reverts commit d547c86 and 9757d93, reversing changes made to ab6eba7.

This was referenced Jan 6, 2025

Revert custom sync url #27136

Merged

Revert custom sync url brave/brave-browser#43128

Closed

bsclifton mentioned this pull request Jan 8, 2025

Make Sync Server Url configurable in the UI brave/brave-browser#43181

Open

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

goodov left a comment

jagadeshjai commented Nov 27, 2024

goodov commented Nov 27, 2024 •

edited

Loading

bsclifton commented Nov 27, 2024

brave-builds commented Nov 28, 2024

jagadeshjai commented Nov 28, 2024

bsclifton commented Jan 8, 2025 •

edited

Loading

[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

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

goodov left a comment

Choose a reason for hiding this comment

jagadeshjai commented Nov 27, 2024

goodov commented Nov 27, 2024 • edited Loading

bsclifton commented Nov 27, 2024

brave-builds commented Nov 28, 2024

jagadeshjai commented Nov 28, 2024

bsclifton commented Jan 8, 2025 • edited Loading

jagadeshjai commented Sep 7, 2024 •

edited

Loading

goodov Sep 16, 2024 •

edited

Loading

goodov commented Nov 27, 2024 •

edited

Loading

bsclifton commented Jan 8, 2025 •

edited

Loading