Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885943002:
Implementing sign-in promo for "Other Devices" on the iPad (Closed)
|
Created:
3 years, 7 months ago by jlebel Modified:
3 years, 7 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementing sign-in promo for "Other Devices" on the iPad
https://drive.google.com/open?id=0ByXziH_JVCGJVm9odnNCMlRoMkE
https://drive.google.com/open?id=0ByXziH_JVCGJcUJFNEdpTnBoa0E
BUG=661794
Review-Url: https://codereview.chromium.org/2885943002
Cr-Commit-Position: refs/heads/master@{#474240}
Committed: https://chromium.googlesource.com/chromium/src/+/3e87b200830a0eb992d8e370eaf2c4a314750ecf
Patch Set 1 #Patch Set 2 : Merge #
Total comments: 10
Patch Set 3 : . #
Depends on Patchset: Messages
Total messages: 22 (12 generated)
Description was changed from ========== Implementing sign-in promo for "Other Devices" on the iPad BUG=661794 ========== to ========== Implementing sign-in promo for "Other Devices" on the iPad https://drive.google.com/open?id=0ByXziH_JVCGJVm9odnNCMlRoMkE https://drive.google.com/open?id=0ByXziH_JVCGJcUJFNEdpTnBoa0E BUG=661794 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Hello Mihai, Can you review this patch about sign-in promo in "Other Devices" tab on iPad? Thanks,
jlebel@chromium.org changed reviewers: + msarda@chromium.org
Hello Mihai, Can you review this patch about sign-in promo in "Other Devices" tab on iPad? Thanks,
Please keep in mind that I am not an owner of this code. https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm (right): https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:180: // Adding sign-in promo view. It looks like the sign-in promo view is created even when the user is signed in. I think that we should avoid creating the sign-in promo view when the overlay type is different than OVERLAY_PANEL_USER_SIGNED_OUT https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:184: l10n_util::GetNSString(IDS_IOS_SIGNIN_PROMO_SETTINGS); Why are we using the settings promo string in the recent tabs? https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:207: [_signinPromoView setNeedsLayout]; Why should the sign-in promo view be layed out if it is hidden? https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:233: [self updateText]; When the promo is enabled, it looks like we do not call updateText or updateButtonTarget when the overlay type is SIGNED_OUT. However we still call it for the other cases. Is that expected? https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:254: case TabSwitcherPanelOverlayType::OVERLAY_PANEL_USER_SIGNED_OUT: Maybe: DCHECK(!experimental_flags::IsSigninPromoEnabled());
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm (right): https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:180: // Adding sign-in promo view. On 2017/05/23 12:55:54, msarda wrote: > It looks like the sign-in promo view is created even when the user is signed in. > I think that we should avoid creating the sign-in promo view when the overlay > type is different than OVERLAY_PANEL_USER_SIGNED_OUT Done. https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:184: l10n_util::GetNSString(IDS_IOS_SIGNIN_PROMO_SETTINGS); On 2017/05/23 12:55:55, msarda wrote: > Why are we using the settings promo string in the recent tabs? Done. https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:207: [_signinPromoView setNeedsLayout]; On 2017/05/23 12:55:55, msarda wrote: > Why should the sign-in promo view be layed out if it is hidden? I've done it just to make sure the layout is correct when it will shown. But there is no reason anyway for this line since we just created the view, and the layout has to be made anyway. https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:233: [self updateText]; On 2017/05/23 12:55:55, msarda wrote: > When the promo is enabled, it looks like we do not call updateText or > updateButtonTarget when the overlay type is SIGNED_OUT. However we still call it > for the other cases. Is that expected? In this overlay view, I can't use the current UI element. I choose to add the SigninPromoView on top and hide one or the other. So if the sign-in promo view is shown there is no reason to update the hidden elements. https://codereview.chromium.org/2885943002/diff/60001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_overlay_view.mm:254: case TabSwitcherPanelOverlayType::OVERLAY_PANEL_USER_SIGNED_OUT: On 2017/05/23 12:55:55, msarda wrote: > Maybe: DCHECK(!experimental_flags::IsSigninPromoEnabled()); Done.
jlebel@chromium.org changed reviewers: + sdefresne@chromium.org
Hello Sylvain, Can you review this patch related to the sign-in promo for "Other devices" tab? Thanks,
lgtm
lgtm
The CQ bit was checked by jlebel@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cluis2034@gmail.com changed reviewers: + cluis2034@gmail.com
lgtm Hi
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495614952633090,
"parent_rev": "61d2da8726d19192add48211792c0ec9a09fe58d", "commit_rev":
"3e87b200830a0eb992d8e370eaf2c4a314750ecf"}
Message was sent while issue was closed.
Description was changed from ========== Implementing sign-in promo for "Other Devices" on the iPad https://drive.google.com/open?id=0ByXziH_JVCGJVm9odnNCMlRoMkE https://drive.google.com/open?id=0ByXziH_JVCGJcUJFNEdpTnBoa0E BUG=661794 ========== to ========== Implementing sign-in promo for "Other Devices" on the iPad https://drive.google.com/open?id=0ByXziH_JVCGJVm9odnNCMlRoMkE https://drive.google.com/open?id=0ByXziH_JVCGJcUJFNEdpTnBoa0E BUG=661794 Review-Url: https://codereview.chromium.org/2885943002 Cr-Commit-Position: refs/heads/master@{#474240} Committed: https://chromium.googlesource.com/chromium/src/+/3e87b200830a0eb992d8e370eaf2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://chromium.googlesource.com/chromium/src/+/3e87b200830a0eb992d8e370eaf2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
