Note: Non-HTTPS URLs are not supported and will be ignored in favor of the browser's default sync URL

Outcome

https://github.com/user-attachments/assets/24593292-2c15-4854-b3fe-a0cecdebac67

Resolves https://github.com/brave/brave-browser/issues/43181

Wohoo! Would love to see this merged!

Hey @AlexeyBarabash ! Could you please take a look at this PR?

@fmarier Have updated the description now, PTAL.

Launched a CI run https://github.com/brave/brave-core/pull/28552

OK folks - quick update; accepted the text change from @fmarier, fixed a lint issue, and rebased 😄

New CI run is taking place - we can watch over https://github.com/brave/brave-core/pull/28552 (prior run before text change and before rebase passed on all platforms)

CI looks great (all green). Let me ping some code owners 😄

@fmarier , do we need to submit the request for security review for this PR?

@fmarier , do we need to submit the request for security review for this PR?

If you can file one and assign it to me, I'll take care of it since I've already reviewed the PR.

@fmarier done! Created https://github.com/brave/reviews/issues/1918 and added some of the history there 😄

@jagadeshjai I also closed the Android PR. I don't mean any offense, you did great work! 😄👍 But I think this new apoproach you created is universal - it can solve it for all platforms. Thanks again! 🙏

Snap, I was hoping this would roll out in 1.79. Any chance we can see this in 1.80, then?

Gave this a rebase - thanks for revisiting @jagadeshjai 😄

CI running with https://github.com/brave/brave-core/pull/28552

@jagadeshjai looks like the browser test SyncUrlTests/BraveMainDelegateSyncUrlBrowserTest.SyncUrlHandling/None is failing; I think that needs an edit

@jagadeshjai hate to be an ass here but it would be tremendously appreciated if you could have it merged by the next release 🙏🏻

@jagadeshjai looks like the browser test SyncUrlTests/BraveMainDelegateSyncUrlBrowserTest.SyncUrlHandling/None is failing; I think that needs an edit

@bsclifton Sorry I've missed it, I'll check this by today.

Thanks for rebasing - CI running with https://github.com/brave/brave-core/pull/28552

I'm not sure why - but CI is failing. Specifically, one test is failing on Windows and Linux. Otherwise, everything is passing. I have re-ran CI and also rebased/pushed/re-ran CI.

The test failing is HistoryNoticeUtilsTest.WebHistoryStates and it fails like so:

[ RUN      ] HistoryNoticeUtilsTest.WebHistoryStates
../../components/browsing_data/core/history_notice_utils_unittest.cc:55: Failure
Expected equality of these values:
  expected_test_case_result
    Which is: true
  result
    Which is: false
Stack trace:
#0 0x59f4644b9a44 browsing_data::HistoryNoticeUtilsTest::ExpectShouldPopupDialogAboutOtherFormsOfBrowsingHistoryWithResult() [../../components/browsing_data/core/history_notice_utils_unittest.cc:55:5]
#1 0x59f4644b9b8f browsing_data::HistoryNoticeUtilsTest_WebHistoryStates_Test::TestBody() [../../components/browsing_data/core/history_notice_utils_unittest.cc:99:3]

cc: @mkarolin @cdesouza-chromium in case you had an idea about this upstream test 🤔

I'm not sure why - but CI is failing. Specifically, one test is failing on Windows and Linux. Otherwise, everything is passing. I have re-ran CI and also rebased/pushed/re-ran CI.

The test failing is HistoryNoticeUtilsTest.WebHistoryStates and it fails like so:

[ RUN      ] HistoryNoticeUtilsTest.WebHistoryStates
../../components/browsing_data/core/history_notice_utils_unittest.cc:55: Failure
Expected equality of these values:
  expected_test_case_result
    Which is: true
  result
    Which is: false
Stack trace:
#0 0x59f4644b9a44 browsing_data::HistoryNoticeUtilsTest::ExpectShouldPopupDialogAboutOtherFormsOfBrowsingHistoryWithResult() [../../components/browsing_data/core/history_notice_utils_unittest.cc:55:5]
#1 0x59f4644b9b8f browsing_data::HistoryNoticeUtilsTest_WebHistoryStates_Test::TestBody() [../../components/browsing_data/core/history_notice_utils_unittest.cc:99:3]

cc: @mkarolin @cdesouza-chromium in case you had an idea about this upstream test 🤔

The test now fails because this PR changes the internal::kSyncServerUrl value to BRAVE_SYNC_ENDPOINT and components/history/core/test/fake_web_history_service.cc has the value for kSyncServerHost hardcoded to clients4.google.com. The mismatch makes FakeWebHistoryService::FakeRequest::GetResponseBody not return the expected data. I don't immediately see an easy way to use an override and I don't think we should patch for this, so likely it's best to add the test to the brave/test/filters/components_unittests.filter with an explanation.

Thanks for digging in, @mkarolin! 😄

cc: @jagadeshjai on above - if you can add that exception then I think we're good to go!

Thanks for digging in, @mkarolin! 😄

cc: @jagadeshjai on above - if you can add that exception then I think we're good to go!

Thanks @mkarolin :heart: , Have pushed the changes, @bsclifton , could you kindly trigger the CI again?

OK great @jagadeshjai - got that workflow kicked off 😄

It should start CI here in the next 10 mins or so. Stay tuned - you can check out https://github.com/brave/brave-core/pull/28552

OK great - that passed; I think we're good to go! 😄

Released in v1.82.44

Woohoo! Thanks to everyone involved!

Does that release (version) also apply to the mobile versions?

@brevilo it should apply to Android for sure - I don't remember if we're exposing brave://flags on iOS

I am testing this and for some reason, the trailing /v2 from my sync server's URL gets stripped both in brave://sync-internals/ and brave://flags view. The syncing fails as well.

EDIT: I tried with the same Nightly but by disabling the flag and using the --sync-url command line and the /v2 URL is picked up as expected, per brave://sync-internals.

FYI @jagadeshjai

@jagadeshjai not sure if you saw my comment above, but this functionality seems partially broken at this point — at least on my end, so I'd appreciate if someone could confirm it on theirs.

@jagadeshjai not sure if you saw my comment above, but this functionality seems partially broken at this point — at least on my end, so I'd appreciate if someone could confirm it on theirs.

Indentical on my end. /v2 gets stripped from the url regardless of the tricks i try

Same here. Unless I put the sync server behind a different subdomain (so that the URL path is empty), I cannot get the brave flag to work because the path of the URL is removed on restart. BTW I have TLS enabled on my deployed brave sync endpoint and it's not accessible through HTTP either (so it should be considered secure).

Same here.

With the sync server running behind a traefik reverse-proxy it was easy to append a prefix to the path and then it worked fine:

      - "traefik.http.middlewares.brave-add-prefix.addprefix.prefix=/v2"
      - "traefik.http.routers.brave-sync.middlewares=brave-add-prefix@docker"

Nonetheless, I agree this could be easier to deploy if the path would not be stripped or the sync-server implementation would not need the "/v2"-endpoint.

@jagadeshjai can you please acknowledge this issue on your end? I am afraid this will get released in that broken state.

If this requires a separate ticket to be opened, I am happy to do so.