Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12477009:
With browser instant extended enabled, signing in should redirect to chrome://apps instead of to NT… (Closed)
|
Created:
7 years, 9 months ago by kmadhusu Modified:
7 years, 9 months ago CC:
chromium-reviews, Raghu Simha, haitaol1, akalin, tim (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWith browser instant extended enabled, signing in should redirect to chrome://apps instead of to NTP.
BUG=178615
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190046
Patch Set 1 : '' #Patch Set 2 : Added a new SyncPromoUI::Source for Apps page signin. #
Total comments: 10
Patch Set 3 : Addressed comments #Patch Set 4 : Addressed comments (Reverted to patchset 1) #
Messages
Total messages: 12 (0 generated)
mad@: PTAL. Thanks.
Sorry, I'm not very familiar with this code, maybe rogerta@ or tim@ could take a look. For my part, I'm not convinced we should assume that if we are running with instant extended enabled, we simply replace the NTP with the Apps page as a continue URL... This seems fragile... What if we add other pages with the sing in UI? Maybe we need to specify the continue URL as we call into the one click sign in?
On 2013/03/21 20:08:10, MAD wrote: > Sorry, I'm not very familiar with this code, maybe rogerta@ or tim@ could take a > look. > > For my part, I'm not convinced we should assume that if we are running with > instant extended enabled, we simply replace the NTP with the Apps page as a > continue URL... This seems fragile... What if we add other pages with the sing > in UI? Maybe we need to specify the continue URL as we call into the one click > sign in? Thanks for your comments. I will take a look on how to pass the source url or continue url into one click sign in. In the mean time, I will wait for other reviewers to comment on the existing code.
https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (left): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:1005: source_ = SyncPromoUI::SOURCE_UNKNOWN; This method is meant to reset the state of OneClickSigninHelper. This should not be removed. Why do you need to remove it? https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:964: VLOG(1) << "OneClickSigninHelper::RedirectToAppsPage"; Please rename to RedirectToNtpOrAppsPage https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:978: (source_ == SyncPromoUI::SOURCE_MENU && Why does the menu redirect to the apps page?
rogerta@: Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (left): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:1005: source_ = SyncPromoUI::SOURCE_UNKNOWN; On 2013/03/22 14:23:23, Roger Tawa wrote: > This method is meant to reset the state of OneClickSigninHelper. This should > not be removed. Why do you need to remove it? OneClickSigninHelper::SigninSuccess() is called when signin request is complete. We redirect to NTP or Apps page from OneClickSigninHelper::SigninSuccess(). If we reset |source_| to SyncPromoUI::SOURCE_UNKNOWN here, we will lose the source details while calling RedirectToAppsPage() from OneClickSigninHelper::SigninSuccess() function. That's why I removed from here and reset |source_| at lines 1335 and 1135. The other way to achieve the same is to store the SyncPromoUI::Source detail in StateTracker. But it doesn't seem right. Please let me know if there any better to find the source details in OneClickSigninHelper::SigninSuccess(). https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:964: VLOG(1) << "OneClickSigninHelper::RedirectToAppsPage"; On 2013/03/22 14:23:23, Roger Tawa wrote: > Please rename to RedirectToNtpOrAppsPage Done. https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:978: (source_ == SyncPromoUI::SOURCE_MENU && On 2013/03/22 14:23:23, Roger Tawa wrote: > Why does the menu redirect to the apps page? If there is an error during "signing to chrome" from wrench menu, we want to display the error message in the apps page signin bubble. With instant extended enabled, we can only add signin bubble in the apps page.
Thanks Kausalya. A couple of comments below. https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (left): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:1005: source_ = SyncPromoUI::SOURCE_UNKNOWN; On 2013/03/22 15:03:09, kmadhusu wrote: > On 2013/03/22 14:23:23, Roger Tawa wrote: > > This method is meant to reset the state of OneClickSigninHelper. This should > > not be removed. Why do you need to remove it? > > OneClickSigninHelper::SigninSuccess() is called when signin request is complete. > We redirect to NTP or Apps page from OneClickSigninHelper::SigninSuccess(). If > we reset |source_| to SyncPromoUI::SOURCE_UNKNOWN here, we will lose the source > details while calling RedirectToAppsPage() from > OneClickSigninHelper::SigninSuccess() function. That's why I removed from here > and reset |source_| at lines 1335 and 1135. > > The other way to achieve the same is to store the SyncPromoUI::Source detail in > StateTracker. But it doesn't seem right. Please let me know if there any better > to find the source details in OneClickSigninHelper::SigninSuccess(). Once you fix RedirectToNtpOrAppsPage() to no longer use |source_|, this problem goes away. https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:978: (source_ == SyncPromoUI::SOURCE_MENU && On 2013/03/22 15:03:09, kmadhusu wrote: > On 2013/03/22 14:23:23, Roger Tawa wrote: > > Why does the menu redirect to the apps page? > > If there is an error during "signing to chrome" from wrench menu, we want to > display the error message in the apps page signin bubble. With instant extended > enabled, we can only add signin bubble in the apps page. if that is the case with instant extended, then I'm not sure the condition is correct. We need to show the bubble for success and failures. If the NTP can no longer show the bubble when instant extended is enabled, then we should always go to apps page. I think the original code is patchset 1 is correct. Or maybe the following might be better: GURL(chrome::search::IsInstantExtendedAPIEnabled() && show_bubble ? chrome::kChromeUIAppsURL : chrome::kChromeUINewTabURL)
rogerta@: Addressed comments. PTAL. Thanks. https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (left): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:1005: source_ = SyncPromoUI::SOURCE_UNKNOWN; On 2013/03/22 20:44:01, Roger Tawa wrote: > On 2013/03/22 15:03:09, kmadhusu wrote: > > On 2013/03/22 14:23:23, Roger Tawa wrote: > > > This method is meant to reset the state of OneClickSigninHelper. This > should > > > not be removed. Why do you need to remove it? > > > > OneClickSigninHelper::SigninSuccess() is called when signin request is > complete. > > We redirect to NTP or Apps page from OneClickSigninHelper::SigninSuccess(). If > > we reset |source_| to SyncPromoUI::SOURCE_UNKNOWN here, we will lose the > source > > details while calling RedirectToAppsPage() from > > OneClickSigninHelper::SigninSuccess() function. That's why I removed from here > > and reset |source_| at lines 1335 and 1135. > > > > The other way to achieve the same is to store the SyncPromoUI::Source detail > in > > StateTracker. But it doesn't seem right. Please let me know if there any > better > > to find the source details in OneClickSigninHelper::SigninSuccess(). > > Once you fix RedirectToNtpOrAppsPage() to no longer use |source_|, this problem > goes away. Done. https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... File chrome/browser/ui/sync/one_click_signin_helper.cc (right): https://codereview.chromium.org/12477009/diff/16001/chrome/browser/ui/sync/on... chrome/browser/ui/sync/one_click_signin_helper.cc:978: (source_ == SyncPromoUI::SOURCE_MENU && On 2013/03/22 20:44:01, Roger Tawa wrote: > On 2013/03/22 15:03:09, kmadhusu wrote: > > On 2013/03/22 14:23:23, Roger Tawa wrote: > > > Why does the menu redirect to the apps page? > > > > If there is an error during "signing to chrome" from wrench menu, we want to > > display the error message in the apps page signin bubble. With instant > extended > > enabled, we can only add signin bubble in the apps page. > > if that is the case with instant extended, then I'm not sure the condition is > correct. We need to show the bubble for success and failures. If the NTP can > no longer show the bubble when instant extended is enabled, then we should > always go to apps page. > > I think the original code is patchset 1 is correct. Done. >Or maybe the following > might be better: > > GURL(chrome::search::IsInstantExtendedAPIEnabled() && show_bubble ? > chrome::kChromeUIAppsURL : chrome::kChromeUINewTabURL) We should not use the "show_bubble" value to figure out the redirect page. From my understanding of this code, we first load the redirect page without the signin bubble and then once the signin request is complete, we reload the redirect page with the success/failure result in the signin bubble. With instant extended enabled, if we use "show_bubble" in the above said condition, we will show the new tab page first and then we will show the apps page. I don't think that will be a good user experience. WDYT?
lgtm Makes sense. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/12477009/23001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/12477009/23001
Message was sent while issue was closed.
Change committed as 190046 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
