Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10539128:
Set timeout in sync setup (Closed)
|
Created:
8 years, 6 months ago by peria Modified:
8 years, 4 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionOn timeout of setting up sync, show a dialog to tell users the task longer time than expected and to enable them to close spinner window.
BUG=128692
TEST=manually
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148740
Patch Set 1 : Remove a needless empty line #
Total comments: 11
Patch Set 2 : Fixed commented points #
Total comments: 8
Patch Set 3 : Fixed some descriptions. #
Total comments: 14
Patch Set 4 : Fix some commented points #
Total comments: 2
Patch Set 5 : Removed a needless line. #Patch Set 6 : Removed a needless line #Patch Set 7 : Move back setting up of backend-timer into DisplaySpinner() #
Total comments: 9
Patch Set 8 : Fixed commented descriptions #Patch Set 9 : Do not show GAIA login on timeout #
Total comments: 6
Patch Set 10 : Check if login is forced #Patch Set 11 : Removed the branch with force_login #
Total comments: 2
Patch Set 12 : Fix a comment #Patch Set 13 : Fix a unittest #
Total comments: 2
Patch Set 14 : Fix a unittest putting MessageLoop #
Messages
Total messages: 49 (0 generated)
Hi, Would you review this change?
IIUC this condition may happen not just because network failure. This may be caused by invalid auth token. So it would be nice what action the user can take for the error. Probably the dialog should look like: [Failed to start up the sync backend] "Please make sure your network connection and if the problem persists, please try sign out and sign in again to refresh your credentials." https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/app/generate... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/app/generate... chrome/app/generated_resources.grd:11665: + Network time out nit: Indent 2 chars. https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/app/generate... chrome/app/generated_resources.grd:11668: + Cannot connect to the server for a while. Please check your network and try to set up sync again few minutes later. nit: Indent 2 chars. https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/reso... File chrome/browser/resources/sync_setup_overlay.html (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/reso... chrome/browser/resources/sync_setup_overlay.html:365: <h1 i18n-content="syncSetupTimeOutTitle">foo</h1> Please use camel case consistently. TimeOut or Timeout? (I prefer the latter) (cf. kTimeoutSec in .cc file) https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/sync_setup_handler.cc:536: // TODO(kochi): Handle error conditions (timeout, other failures). Timeout is handled, but still need to handle other errors... (SigninFailed() case) Please rewrite as TODO(kochi): Handle error conditions other than timeout. http://crbug.com/128692 https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/sync_setup_handler.cc:551: configuring_sync_ = false; Timeout is handled, but still need to handle other errors... (SigninFailed() case) Please rewrite as TODO(kochi): Handle error conditions other than timeout. http://crbug.com/128692 https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/sync_setup_handler.h (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/sync_setup_handler.h:193: scoped_ptr<base::OneShotTimer<SyncSetupHandler> > login_timer_; Please add comment for this variable. Probably |backend_start_timer_| as a variable name would be better. This is not waiting for login.
Why are we building a timeout? Why not just put a cancel button in the spinner, and let the user decide how long he wants to wait?
Fixed commented points. https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/app/generate... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/app/generate... chrome/app/generated_resources.grd:11665: + Network time out On 2012/06/13 09:08:29, Takayoshi Kochi wrote: > nit: Indent 2 chars. Done. https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/app/generate... chrome/app/generated_resources.grd:11668: + Cannot connect to the server for a while. Please check your network and try to set up sync again few minutes later. On 2012/06/13 09:08:29, Takayoshi Kochi wrote: > nit: Indent 2 chars. Done. https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/reso... File chrome/browser/resources/sync_setup_overlay.html (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/reso... chrome/browser/resources/sync_setup_overlay.html:365: <h1 i18n-content="syncSetupTimeOutTitle">foo</h1> On 2012/06/13 09:08:29, Takayoshi Kochi wrote: > Please use camel case consistently. > TimeOut or Timeout? (I prefer the latter) > (cf. kTimeoutSec in .cc file) Done. https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/sync_setup_handler.cc:551: configuring_sync_ = false; On 2012/06/13 09:08:29, Takayoshi Kochi wrote: > Timeout is handled, but still need to handle other errors... > (SigninFailed() case) > > Please rewrite as > TODO(kochi): Handle error conditions other than timeout. http://crbug.com/128692 Done. https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/sync_setup_handler.h (right): https://chromiumcodereview.appspot.com/10539128/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/sync_setup_handler.h:193: scoped_ptr<base::OneShotTimer<SyncSetupHandler> > login_timer_; On 2012/06/13 09:08:29, Takayoshi Kochi wrote: > Please add comment for this variable. Probably |backend_start_timer_| > as a variable name would be better. > This is not waiting for login. Done.
On 2012/06/13 22:52:49, Andrew T Wilson wrote: > Why are we building a timeout? Why not just put a cancel button in the spinner, > and let the user decide how long he wants to wait? The spinner window already has a cancel button, and it works as you expect. Our new window will show what he/she should do to set up sync successfully. The case that the credentials are broken, described in Kochi's comment, is an example.
On 2012/06/13 22:52:49, Andrew T Wilson wrote: > Why are we building a timeout? Why not just put a cancel button in the spinner, > and let the user decide how long he wants to wait? The user can get an idea what next step can be taken for the error from the dialog. If it takes more than 30 seconds, at least something is wrong.
We need unit tests for this change as well. http://codereview.chromium.org/10539128/diff/1007/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10539128/diff/1007/chrome/app/generated_resour... chrome/app/generated_resources.grd:11664: + <message name="IDS_SYNC_SETUP_TIME_OUT_TITLE"> Need to add 'desc' attributes describing what the strings are for. http://codereview.chromium.org/10539128/diff/1007/chrome/app/generated_resour... chrome/app/generated_resources.grd:11668: + Please make sure your network connection and if the problem persists, please try sign out and sign in again to refresh your credentials. This should probably say: "Please make sure your network connection _is working_..." http://codereview.chromium.org/10539128/diff/1007/chrome/browser/resources/sy... File chrome/browser/resources/sync_setup_overlay.html (right): http://codereview.chromium.org/10539128/diff/1007/chrome/browser/resources/sy... chrome/browser/resources/sync_setup_overlay.html:366: <div i18n-content="syncSetupTimeoutContent" class="content-area">hoge</div> What is "hoge"? http://codereview.chromium.org/10539128/diff/1007/chrome/browser/ui/webui/syn... File chrome/browser/ui/webui/sync_setup_handler.cc (right): http://codereview.chromium.org/10539128/diff/1007/chrome/browser/ui/webui/syn... chrome/browser/ui/webui/sync_setup_handler.cc:557: This doesn't seem to cancel sync initialization. What happens if sync initialization completes while this timeout screen is visible?
https://chromiumcodereview.appspot.com/10539128/diff/1007/chrome/app/generate... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10539128/diff/1007/chrome/app/generate... chrome/app/generated_resources.grd:11664: + <message name="IDS_SYNC_SETUP_TIME_OUT_TITLE"> On 2012/06/15 23:29:40, Andrew T Wilson wrote: > Need to add 'desc' attributes describing what the strings are for. Done. https://chromiumcodereview.appspot.com/10539128/diff/1007/chrome/app/generate... chrome/app/generated_resources.grd:11668: + Please make sure your network connection and if the problem persists, please try sign out and sign in again to refresh your credentials. On 2012/06/15 23:29:40, Andrew T Wilson wrote: > This should probably say: > > "Please make sure your network connection _is working_..." Done. https://chromiumcodereview.appspot.com/10539128/diff/1007/chrome/browser/reso... File chrome/browser/resources/sync_setup_overlay.html (right): https://chromiumcodereview.appspot.com/10539128/diff/1007/chrome/browser/reso... chrome/browser/resources/sync_setup_overlay.html:366: <div i18n-content="syncSetupTimeoutContent" class="content-area">hoge</div> On 2012/06/15 23:29:40, Andrew T Wilson wrote: > What is "hoge"? Oops, this "hoge" and above "foo" are used for checking appearances. Removed. https://chromiumcodereview.appspot.com/10539128/diff/1007/chrome/browser/ui/w... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/1007/chrome/browser/ui/w... chrome/browser/ui/webui/sync_setup_handler.cc:557: On 2012/06/15 23:29:40, Andrew T Wilson wrote: > This doesn't seem to cancel sync initialization. What happens if sync > initialization completes while this timeout screen is visible? No, it does not cancel initialization. In such case, the usual confirm window shows up replacing the visible timeout window.
> On 2012/06/15 23:29:40, Andrew T Wilson wrote: > > This doesn't seem to cancel sync initialization. What happens if sync > > initialization completes while this timeout screen is visible? > > No, it does not cancel initialization. > In such case, the usual confirm window shows up replacing the visible timeout > window. I'm not sure that's the right behavior - typically timeouts cancel whatever kind of configure step was already in progress. See my comment. Overall, this CL looks pretty close, but it still needs unittests.
https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/app/generat... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/app/generat... chrome/app/generated_resources.grd:11668: + Please make sure your network connection is working and if the problem persists, please try sign out and sign in again to refresh your credentials. nit: "try sign out" -> "sign out" https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:543: backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>()); Seems like we should never get called with a non-empty backend_start_timer_ - should we add a DCHECK(backend_start_timer_.get()) before calling reset()? https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:554: configuring_sync_ = false; I'm slightly nervous about leaving sync setup running in the background while the timeout dialog is here. Perhaps we should call CloseSyncSetup() first, before displaying the timeout page? https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:685: if (backend_start_timer_.get()) No need to do a .get() first - just call reset(). https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:711: if (backend_start_timer_.get()) Just call reset(), don't bother checking get() first. Also, should we clear the timer in CloseOverlay() also, in case the user cancels out of the spinner?
https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/app/generat... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/app/generat... chrome/app/generated_resources.grd:11668: + Please make sure your network connection is working and if the problem persists, please try sign out and sign in again to refresh your credentials. On 2012/06/20 00:12:02, Andrew T Wilson wrote: > nit: "try sign out" -> "sign out" Done. https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:543: backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>()); On 2012/06/20 00:12:02, Andrew T Wilson wrote: > Seems like we should never get called with a non-empty backend_start_timer_ - > should we add a DCHECK(backend_start_timer_.get()) before calling reset()? I think it is not necessary to put DCHECK() here, because reset() checks if the scoped pointer figures a non-empty OneShotTimer instance. And if we put DCHECK() here, it fails in the first call of this method because backend_start_timer_ is not instantiated yet. https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:554: configuring_sync_ = false; On 2012/06/20 00:12:02, Andrew T Wilson wrote: > I'm slightly nervous about leaving sync setup running in the background while > the timeout dialog is here. Perhaps we should call CloseSyncSetup() first, > before displaying the timeout page? SGTM. Done. https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:685: if (backend_start_timer_.get()) On 2012/06/20 00:12:02, Andrew T Wilson wrote: > No need to do a .get() first - just call reset(). Done. https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:711: if (backend_start_timer_.get()) On 2012/06/20 00:12:02, Andrew T Wilson wrote: > Just call reset(), don't bother checking get() first. > > Also, should we clear the timer in CloseOverlay() also, in case the user cancels > out of the spinner? Done.
https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:543: backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>()); Sorry, I meant to say "DCHECK(!backend_start_timer_.get()) to make sure it's never already set when we call this routine". But I don't feel that strongly about it, so it's fine as-is. https://chromiumcodereview.appspot.com/10539128/diff/18001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/18001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:554: configuring_sync_ = false; I think CloseSyncSetup() clears the configuring_sync_ flag so you don't have to do this here.
https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:543: backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>()); On 2012/06/20 05:23:41, Andrew T Wilson wrote: > Sorry, I meant to say "DCHECK(!backend_start_timer_.get()) to make sure it's > never already set when we call this routine". > > But I don't feel that strongly about it, so it's fine as-is. I got it. Thank you for your explanation. But it can also fail, because this method can be called many times (described repro-steps below). So I decided not to put it. 1. Click "Setup Sync" 2. DisplaySpinner() is called 3. backend_start_timer_ is set 4. Click "Cancel" 5. Click "Setup Sync" again 6. DisplaySpinner() is called 7. DCHECK fails https://chromiumcodereview.appspot.com/10539128/diff/18001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/18001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:554: configuring_sync_ = false; On 2012/06/20 05:23:42, Andrew T Wilson wrote: > I think CloseSyncSetup() clears the configuring_sync_ flag so you don't have to > do this here. Done.
https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:543: backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>()); On 2012/06/20 09:20:17, peria wrote: > On 2012/06/20 05:23:41, Andrew T Wilson wrote: > > Sorry, I meant to say "DCHECK(!backend_start_timer_.get()) to make sure it's > > never already set when we call this routine". > > > > But I don't feel that strongly about it, so it's fine as-is. > > I got it. Thank you for your explanation. > But it can also fail, because this method can be called many times (described > repro-steps below). So I decided not to put it. > > 1. Click "Setup Sync" > 2. DisplaySpinner() is called > 3. backend_start_timer_ is set > 4. Click "Cancel" > 5. Click "Setup Sync" again > 6. DisplaySpinner() is called > 7. DCHECK fails Isn't that a bug, then? Shouldn't the timeout timer be reset when you cancel? That's kind of why I suggested adding that DCHECK.
https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:543: backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>()); On 2012/06/20 16:40:27, Andrew T Wilson wrote: > On 2012/06/20 09:20:17, peria wrote: > > On 2012/06/20 05:23:41, Andrew T Wilson wrote: > > > Sorry, I meant to say "DCHECK(!backend_start_timer_.get()) to make sure it's > > > never already set when we call this routine". > > > > > > But I don't feel that strongly about it, so it's fine as-is. > > > > I got it. Thank you for your explanation. > > But it can also fail, because this method can be called many times (described > > repro-steps below). So I decided not to put it. > > > > 1. Click "Setup Sync" > > 2. DisplaySpinner() is called > > 3. backend_start_timer_ is set > > 4. Click "Cancel" > > 5. Click "Setup Sync" again > > 6. DisplaySpinner() is called > > 7. DCHECK fails > > Isn't that a bug, then? Shouldn't the timeout timer be reset when you cancel? > That's kind of why I suggested adding that DCHECK. Yes, it is, even though it works without erros. Again, I traced the source in HEAD, and found that this method was called twice in one click of "Set up Sync" button. There are two entry points from JavaScript layer, HandleAttachHandler() and HandleShowSetupUI(). So I replace this code into HandleShowSetupUI() with DCHECK. Would you please take another look?
Could you try rebasing to ToT HEAD? I submitted some changes to this file and may conflict. On 2012/06/21 06:45:32, peria wrote: > https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... > File chrome/browser/ui/webui/sync_setup_handler.cc (right): > > https://chromiumcodereview.appspot.com/10539128/diff/13001/chrome/browser/ui/... > chrome/browser/ui/webui/sync_setup_handler.cc:543: > backend_start_timer_.reset(new base::OneShotTimer<SyncSetupHandler>()); > On 2012/06/20 16:40:27, Andrew T Wilson wrote: > > On 2012/06/20 09:20:17, peria wrote: > > > On 2012/06/20 05:23:41, Andrew T Wilson wrote: > > > > Sorry, I meant to say "DCHECK(!backend_start_timer_.get()) to make sure > it's > > > > never already set when we call this routine". > > > > > > > > But I don't feel that strongly about it, so it's fine as-is. > > > > > > I got it. Thank you for your explanation. > > > But it can also fail, because this method can be called many times > (described > > > repro-steps below). So I decided not to put it. > > > > > > 1. Click "Setup Sync" > > > 2. DisplaySpinner() is called > > > 3. backend_start_timer_ is set > > > 4. Click "Cancel" > > > 5. Click "Setup Sync" again > > > 6. DisplaySpinner() is called > > > 7. DCHECK fails > > > > Isn't that a bug, then? Shouldn't the timeout timer be reset when you cancel? > > That's kind of why I suggested adding that DCHECK. > > Yes, it is, even though it works without erros. > > Again, I traced the source in HEAD, and found that this method was called twice > in one click of "Set up Sync" button. > There are two entry points from JavaScript layer, HandleAttachHandler() and > HandleShowSetupUI(). So I replace this code into HandleShowSetupUI() with > DCHECK. > > Would you please take another look? I was also wondering this recursive behavior, and I thought it should go away, but I am not completely sure. Anyway, I don't think this move is not correct, as HandleShowSetupUI() can be used by non-spinner dialog case.
Put back the setting up of the timer in DisplaySpinner(). Hanckathon changes resolved the bug that this method was called twice in one click of "Set up Sync" button. Now you can click "Set up Sync" button and its cancel button repeatedly.
LGTM http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... File chrome/browser/ui/webui/sync_setup_handler.h (right): http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... chrome/browser/ui/webui/sync_setup_handler.h:148: // Returns true if we're the active login object. Apparently the style guide discourages pronouns. You'll need to change this to "if this object is the active login object"
lgtm Once you corrected the comment, I'll submit to the CQ. On 2012/06/26 18:11:25, Andrew T Wilson wrote: > LGTM > > http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... > File chrome/browser/ui/webui/sync_setup_handler.h (right): > > http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... > chrome/browser/ui/webui/sync_setup_handler.h:148: // Returns true if we're the > active login object. > Apparently the style guide discourages pronouns. You'll need to change this to > "if this object is the active login object"
On 2012/06/26 22:54:10, Takayoshi Kochi wrote: > lgtm > > Once you corrected the comment, I'll submit to the CQ. Oops, you still need an OWNERS lgtm. > On 2012/06/26 18:11:25, Andrew T Wilson wrote: > > LGTM > > > > > http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... > > File chrome/browser/ui/webui/sync_setup_handler.h (right): > > > > > http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... > > chrome/browser/ui/webui/sync_setup_handler.h:148: // Returns true if we're the > > active login object. > > Apparently the style guide discourages pronouns. You'll need to change this to > > "if this object is the active login object"
Adding jhawkins@ for OWNERS review.
http://chromiumcodereview.appspot.com/10539128/diff/38001/chrome/browser/reso... File chrome/browser/resources/sync_setup_overlay.html (right): http://chromiumcodereview.appspot.com/10539128/diff/38001/chrome/browser/reso... chrome/browser/resources/sync_setup_overlay.html:366: <div i18n-content="syncSetupTimeoutContent" class="content-area"></div> s/div/span/ http://chromiumcodereview.appspot.com/10539128/diff/38001/chrome/browser/reso... chrome/browser/resources/sync_setup_overlay.html:368: <input id="timeout-ok" type="button" i18n-values="value:ok"> Use <button></button> http://chromiumcodereview.appspot.com/10539128/diff/38001/chrome/browser/ui/w... File chrome/browser/ui/webui/sync_setup_handler.h (right): http://chromiumcodereview.appspot.com/10539128/diff/38001/chrome/browser/ui/w... chrome/browser/ui/webui/sync_setup_handler.h:148: // Returns true if we're the active login object. On 2012/06/26 18:11:25, Andrew T Wilson wrote: > Apparently the style guide discourages pronouns. You'll need to change this to > "if this object is the active login object" Yay! Rationale: pronouns are ambiguous.
+cpu for chrome/app/generated_resources.grd OWNERS review.
lgtm http://codereview.chromium.org/10539128/diff/38001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10539128/diff/38001/chrome/app/generated_resou... chrome/app/generated_resources.grd:11766: + <message name="IDS_SYNC_SETUP_TIME_OUT_CONTENT" desc="Text explaining what to do if setting up sync goes timeout."> "what to do if sync times out."
Thank you for all your reviews, but please cancel current LGTMs and suspend reviewing. My check was not enough after merging with HEAD code, and the current change does not resolve. I will notify when I solve it again. http://codereview.chromium.org/10539128/diff/38001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10539128/diff/38001/chrome/app/generated_resou... chrome/app/generated_resources.grd:11766: + <message name="IDS_SYNC_SETUP_TIME_OUT_CONTENT" desc="Text explaining what to do if setting up sync goes timeout."> On 2012/06/28 18:44:24, cpu wrote: > "what to do if sync times out." Done. http://codereview.chromium.org/10539128/diff/38001/chrome/browser/resources/s... File chrome/browser/resources/sync_setup_overlay.html (right): http://codereview.chromium.org/10539128/diff/38001/chrome/browser/resources/s... chrome/browser/resources/sync_setup_overlay.html:366: <div i18n-content="syncSetupTimeoutContent" class="content-area"></div> On 2012/06/26 23:00:14, James Hawkins wrote: > s/div/span/ Done. http://codereview.chromium.org/10539128/diff/38001/chrome/browser/resources/s... chrome/browser/resources/sync_setup_overlay.html:368: <input id="timeout-ok" type="button" i18n-values="value:ok"> On 2012/06/26 23:00:14, James Hawkins wrote: > Use <button></button> Done. http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... File chrome/browser/ui/webui/sync_setup_handler.h (right): http://codereview.chromium.org/10539128/diff/38001/chrome/browser/ui/webui/sy... chrome/browser/ui/webui/sync_setup_handler.h:148: // Returns true if we're the active login object. On 2012/06/26 18:11:25, Andrew T Wilson wrote: > Apparently the style guide discourages pronouns. You'll need to change this to > "if this object is the active login object" Done.
I was sorry to wait for a while. Please start reviewing again. The problem I found was that a GAIA login dialog appeared at the same time if the timeout dialog showed up.
lgtm with one style nit. I don't like where |visible_timeout_| is cleared, but probably we can clean up these recursion guard code once we can remove HandleAttachHandler() calls from JS code. https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:884: } You don't need {} here.
https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:882: if (visible_timeout_) { I don't get it - why is HandleAttachHandler getting called when the timeout dialog is displayed? I thought this was supposed to only get called when the window is first shown? It seems like a bug if this is called twice, and adding visible_timeout_ logic to avoid the bug seems like the wrong approach - we should not call this multiple times.
I tested manually, and it works well on Cr, CrOS, and unconnected CrOS. https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:882: if (visible_timeout_) { On 2012/07/02 15:31:58, Andrew T Wilson wrote: > I don't get it - why is HandleAttachHandler getting called when the timeout > dialog is displayed? I thought this was supposed to only get called when the > window is first shown? It seems like a bug if this is called twice, and adding > visible_timeout_ logic to avoid the bug seems like the wrong approach - we > should not call this multiple times. Yes, this method is called a dialog is shown up in sync_setup_overlay.js. I am sorry, my comment in the code was wrong. https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:884: } On 2012/07/02 09:34:48, Takayoshi Kochi wrote: > You don't need {} here. removed the branch.
https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:882: if (visible_timeout_) { Your new code still breaks if you get a timeout when forceLogin is passed. Try testing using this URL: chrome://chrome/settings/syncSetup#forceLogin I suspect that if you hit a timeout, it'll display the login screen again along with the timeout. Also, with your code as-is, can you still hit chrome://chrome/settings/syncSetup, and have the sync setup dialog display?
BTW, it looks like the forceLogin code is obsolete - we don't need to support it any more. So, if visiting chrome://chrome/settings/syncSetup works (it displays the sync setup dialog automatically) then I think this is OK to land.
https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/56001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:882: if (visible_timeout_) { On 2012/07/03 16:35:05, Andrew T Wilson wrote: > Your new code still breaks if you get a timeout when forceLogin is passed. > > Try testing using this URL: > > chrome://chrome/settings/syncSetup#forceLogin > > I suspect that if you hit a timeout, it'll display the login screen again along > with the timeout. > > Also, with your code as-is, can you still hit > chrome://chrome/settings/syncSetup, and have the sync setup dialog display? I checked with current code (Patch 10), - click "Set up Sync" button and jump to chrome://chrome/settings/syncSetup shows dialogs we expect - hit chrome://chrome/settings/syncSetup shows no dialogs. - hit chrome://chrome/settings/syncSetup#forceLogin shows Gaia login dialog. Assuming forceLogin obsolete as you said in another message, I will fix second case above.
LGTM
I'm very sorry for being so late. I've updated the patch, which removes the branch of |force_login|. I manually tested with the situations we discussed, and it worked as expected.
On 2012/07/20 04:27:12, peria wrote: > I'm very sorry for being so late. > I've updated the patch, which removes the branch of |force_login|. > > I manually tested with the situations we discussed, and it worked as expected. lgtm Thanks for working on this patiently:) Now this changee looks much cleaner and safe.
Oops, I missed to publish review comments - one nit for the comment. https://chromiumcodereview.appspot.com/10539128/diff/69001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/69001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:617: // Temporarily disable signin tracker. The code looks good, but this is not temporary. Disabling SigninTracker is to not listening the signin success/fail events. Probably this comment should say like "Do not listen to signin events."
https://chromiumcodereview.appspot.com/10539128/diff/69001/chrome/browser/ui/... File chrome/browser/ui/webui/sync_setup_handler.cc (right): https://chromiumcodereview.appspot.com/10539128/diff/69001/chrome/browser/ui/... chrome/browser/ui/webui/sync_setup_handler.cc:617: // Temporarily disable signin tracker. On 2012/07/20 05:18:34, Takayoshi Kochi wrote: > The code looks good, but this is not temporary. > Disabling SigninTracker is to not listening the signin success/fail events. > Probably this comment should say like "Do not listen to signin events." Done.
Andrew PTAL?
On 2012/07/23 04:29:10, peria wrote: > Andrew > PTAL? Still LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/peria@chromium.org/10539128/70006
Try job failure for 10539128-70006 (retry) on linux_rel for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/peria@chromium.org/10539128/70006
Try job failure for 10539128-70006 (retry) (retry) on linux_rel for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
http://codereview.chromium.org/10539128/diff/80001/chrome/browser/ui/webui/sy... File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right): http://codereview.chromium.org/10539128/diff/80001/chrome/browser/ui/webui/sy... chrome/browser/ui/webui/sync_setup_handler_unittest.cc:425: MessageLoop message_loop_; Could you elaborate why this fixes the test? Please add why this has to be exist, otherwise this just looks *not used*.
My previous change broke tests with DCHECK fails. It was because Timer class required MessageLoop internally, and it was not set in unit tests in default. Last patch sets a MessageLoop instance in our unittest file to fix it.
Put a comment to explain why MessageLoop is added. http://codereview.chromium.org/10539128/diff/80001/chrome/browser/ui/webui/sy... File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right): http://codereview.chromium.org/10539128/diff/80001/chrome/browser/ui/webui/sy... chrome/browser/ui/webui/sync_setup_handler_unittest.cc:425: MessageLoop message_loop_; On 2012/07/27 05:13:53, Takayoshi Kochi wrote: > Could you elaborate why this fixes the test? > Please add why this has to be exist, otherwise this just looks *not used*. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/peria@chromium.org/10539128/80003
Change committed as 148740 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
