Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[base 1.11.x beta] Sync: Display a deprecation warning for users of sync v1 #5806

Merged
merged 1 commit into from
Jun 30, 2020
Merged

[base 1.11.x beta] Sync: Display a deprecation warning for users of sync v1 #5806

merged 1 commit into from
Jun 30, 2020

Conversation

petemill
Copy link
Member

@petemill petemill commented Jun 10, 2020 •
edited
Loading

Resolves brave/brave-browser#10231

Test Plan

A

  • Enable brave://flags flag for brave sync
  • Restart
  • Create a sync chain at brave://sync
  • Restart
  • Observe infobar is shown:
    image

B

  • Perform Plan A
  • Click link
  • Observe infobar is removed from source tab
  • Restart
  • Observe Infobar is not shown

C

  • Perform Plan A
  • Click (x)
  • Restart
  • Observe Infobar is not shown

Submitter Checklist:

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.

@petemill petemill self-assigned this Jun 10, 2020
@petemill petemill force-pushed the sync-v1-deprecation-notice branch 2 times, most recently from a58bec3 to 5f47119 Compare June 11, 2020 19:43
@petemill petemill changed the title Sync: Display a deprecation warning for users of sync v1 [base 1.11.x beta] Sync: Display a deprecation warning for users of sync v1 Jun 11, 2020
@petemill petemill force-pushed the sync-v1-deprecation-notice branch 2 times, most recently from 93803c5 to be445dc Compare June 11, 2020 19:48
@petemill petemill changed the base branch from master to 1.11.x June 11, 2020 19:48
@petemill petemill marked this pull request as ready for review June 11, 2020 19:54
@petemill petemill requested a review from bridiver as a code owner June 11, 2020 19:54
@petemill petemill added this to the 1.11.x - Beta milestone Jun 11, 2020
@petemill petemill force-pushed the sync-v1-deprecation-notice branch from be445dc to 6c9e9be Compare June 11, 2020 20:08
@petemill petemill force-pushed the sync-v1-deprecation-notice branch 2 times, most recently from c958980 to 22e3f4b Compare June 16, 2020 04:19
@petemill petemill force-pushed the sync-v1-deprecation-notice branch from 22e3f4b to e85594c Compare June 24, 2020 19:17
@petemill petemill requested a review from darkdh June 24, 2020 19:18
darkdh
darkdh approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@petemill petemill requested a review from a team June 24, 2020 21:34
simonhong
simonhong reviewed Jun 25, 2020
View reviewed changes
browser/infobars/sync_v1_deprecation_infobar_delegate.cc
return;
}

// Only show the bar once, ever
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user uses multiple profile, maybe that user could see this infobar multiple times.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with that, the other profiles must have sync enabled too. We can't assume that the other profiles are not other people perhaps.

simonhong
simonhong approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits.

browser/infobars/sync_v1_deprecation_infobar_delegate.cc
@@ -0,0 +1,92 @@
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 2020

browser/infobars/sync_v1_deprecation_infobar_delegate.h
@@ -0,0 +1,38 @@
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 2020

@kjozwiak
Copy link
Member

Looks like test-install failed on macOS as per https://ci.brave.com/job/pr-brave-browser-sync-v1-deprecation-notice/4/execution/node/647/log/. However, this a known intermittent failure and doesn't block uplift.

@petemill petemill merged commit a955604 into 1.11.x Jun 30, 2020
@petemill petemill deleted the sync-v1-deprecation-notice branch June 30, 2020 16:55
@petemill petemill mentioned this pull request Jun 30, 2020
Closed
@LaurenWags LaurenWags mentioned this pull request Jul 8, 2020
Closed
@AlexeyBarabash AlexeyBarabash mentioned this pull request Jul 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh approved these changes

@simonhong simonhong simonhong approved these changes

@AlexeyBarabash AlexeyBarabash Awaiting requested review from AlexeyBarabash

@bbondy bbondy Awaiting requested review from bbondy

@bsclifton bsclifton Awaiting requested review from bsclifton

@bridiver bridiver Awaiting requested review from bridiver bridiver is a code owner

Assignees

@petemill petemill

Labels
None yet
Projects
None yet
Milestone
1.11.x - Release #1
Development

Successfully merging this pull request may close these issues.
4 participants
@petemill @kjozwiak @simonhong @darkdh