sync: Show the expired credential errors in the Bookmark Manager.

sync: Show the expired credential errors in the Bookmark Manager.

BUG=26551
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33152

[tfarina (gmail-do not use), 2009-11-21 14:16:49]

Hi idana, could you review this to me?

[idana, 2009-11-23 20:53:43]

Sure. I just sent the patch to the try servers. I'll reply when everything goes
green.

[idana, 2009-11-23 23:58:43]

Thiago,

Your patch doesn't really fix the bug in question. If you read the bug
description, it indicates that if the user is required to re-enter the password,
the Bookmark Manager will currently display "Synced to foo@gmail.com" and what
it should display is "Sync Error" (just like what we display on the bookmarks
bar). With you patch, if we are in the error state the Bookmarks Manager will
display "Sync my bookmarks..." which is actually worse than what we do right
now.

[tfarina (gmail-do not use), 2009-11-24 00:02:50]

On 2009/11/23 23:58:43, idana wrote:
> Thiago,
> 
> Your patch doesn't really fix the bug in question. If you read the bug
> description, it indicates that if the user is required to re-enter the
password,
> the Bookmark Manager will currently display "Synced to foo@gmail.com" and what
> it should display is "Sync Error" (just like what we display on the bookmarks
> bar). With you patch, if we are in the error state the Bookmarks Manager will
> display "Sync my bookmarks..." which is actually worse than what we do right
> now.

OK, that explain the issue, I will take a look right now.

[tfarina (gmail-do not use), 2009-11-24 00:17:54]

On 2009/11/24 00:02:50, tfarina wrote:
> On 2009/11/23 23:58:43, idana wrote:
> > Thiago,
> > 
> > Your patch doesn't really fix the bug in question. If you read the bug
> > description, it indicates that if the user is required to re-enter the
> password,
> > the Bookmark Manager will currently display "Synced to foo@gmail.com" and
what
> > it should display is "Sync Error" (just like what we display on the
bookmarks
> > bar). With you patch, if we are in the error state the Bookmarks Manager
will
> > display "Sync my bookmarks..." which is actually worse than what we do right
> > now.
> 

I made the changes required and I'm going to test here, so as soon it passes, I
will upload the fix.

[tfarina (gmail-do not use), 2009-11-24 00:38:14]

idana, how can I reproduce exactly the issue?

It says "--sync-url to a server which expire the login ", which url are you
using? I pick a random url, but it seems not working.

[idana, 2009-11-24 01:26:24]

Unfortunately, it won't be possible for you to test your changes easily
because we don't (yet) have an open source server implementation you can run
locally.

Given the above, I think it might be better to punt this bug for now (and
remove it from Mstone-4) because it is a pretty low priority issue. I
apologize for the inconvenience.

On Mon, Nov 23, 2009 at 4:38 PM, <thiago.farina@gmail.com> wrote:

> idana, how can I reproduce exactly the issue?
>
> It says "--sync-url to a server which expire the login ", which url are you
> using? I pick a random url, but it seems not working.
>
>
>
>
> http://codereview.chromium.org/418044
>



-- 
-Idan

[tfarina (gmail-do not use), 2009-11-24 01:29:00]

On 2009/11/24 01:26:24, idana wrote:
> Unfortunately, it won't be possible for you to test your changes easily
> because we don't (yet) have an open source server implementation you can run
> locally.
> 
> Given the above, I think it might be better to punt this bug for now (and
> remove it from Mstone-4) because it is a pretty low priority issue. I
> apologize for the inconvenience.

I would like to upload my fix. Can you test it if I upload?

[idana, 2009-11-24 01:29:52]

Yes, I can test it for you.

On Mon, Nov 23, 2009 at 5:28 PM, <thiago.farina@gmail.com> wrote:

> On 2009/11/24 01:26:24, idana wrote:
>
>> Unfortunately, it won't be possible for you to test your changes easily
>> because we don't (yet) have an open source server implementation you can
>> run
>> locally.
>>
>
>  Given the above, I think it might be better to punt this bug for now (and
>> remove it from Mstone-4) because it is a pretty low priority issue. I
>> apologize for the inconvenience.
>>
>
> I would like to upload my fix. Can you test it if I upload?
>
>
> http://codereview.chromium.org/418044
>



-- 
-Idan

[tfarina (gmail-do not use), 2009-11-24 01:35:29]

On 2009/11/24 01:29:52, idana wrote:
> Yes, I can test it for you.

Thanks, I uploaded the changes. Please take a look.

[tfarina (gmail-do not use), 2009-11-24 18:51:23]

Didn't work?

[idana, 2009-11-24 21:53:39]

Sorry, Thiago, I haven't tried it out yet. I'll do it in the coming couple
of hours and will let you know.

On Tue, Nov 24, 2009 at 10:51 AM, <thiago.farina@gmail.com> wrote:

> Didn't work?
>
>
>
>
> http://codereview.chromium.org/418044
>



-- 
-Idan

[idana, 2009-11-24 23:13:41]

In addition to my comment about he icon and tooltip (see below) there is another
problem with you change which is that clicking on the sync button from the
Bookmark Manager when credentials are required brings up the Options dialog
instead of bringing up the login dialog, which is what happens when clicking the
re-auth button on the bookmarks bar.

http://codereview.chromium.org/418044/diff/8001/7002
File chrome/browser/views/bookmark_manager_view.cc (right):

http://codereview.chromium.org/418044/diff/8001/7002#newcode836
chrome/browser/views/bookmark_manager_view.cc:836:
*ResourceBundle::GetSharedInstance().GetBitmapNamed(IDR_WARNING));
You set the icon and the tooltip when there is an error, but you don't reset
them when transitioning back to the 'no error' state and the end result is that
the icon and the tooltip stay on the button even after the credentials are
successfully re-entered and sync is working again.

[tfarina (gmail-do not use), 2009-11-25 00:03:43]

On 2009/11/24 23:13:41, idana wrote:
> In addition to my comment about he icon and tooltip (see below) there is
another
> problem with you change which is that clicking on the sync button from the
> Bookmark Manager when credentials are required brings up the Options dialog
> instead of bringing up the login dialog, which is what happens when clicking
the
> re-auth button on the bookmarks bar.

To do this I have to options, either create a boolean member variable like
|sync_status_error_| or use the approach of the bookmarks bar code that is to
set the tag of button. Do you have any preferences? I guess the latter is fine. 
 
> http://codereview.chromium.org/418044/diff/8001/7002
> File chrome/browser/views/bookmark_manager_view.cc (right):
> 
> http://codereview.chromium.org/418044/diff/8001/7002#newcode836
> chrome/browser/views/bookmark_manager_view.cc:836:
> *ResourceBundle::GetSharedInstance().GetBitmapNamed(IDR_WARNING));
> You set the icon and the tooltip when there is an error, but you don't reset
> them when transitioning back to the 'no error' state and the end result is
that
> the icon and the tooltip stay on the button even after the credentials are
> successfully re-entered and sync is working again.

Sure, I will reset them.

[tfarina (gmail-do not use), 2009-11-25 19:23:55]

Hi Idan, I rebased it to ToT and I made the changes, please have another look.

[idana, 2009-11-25 20:12:38]

Hi Thiago,

I tested your patch and it seems to work pretty well. Please fix the nit comment
I have and re-upload the patch. Once you do that, I'll send your patch to the
try bots and we'll see how it goes.

http://codereview.chromium.org/418044/diff/4008/5003
File chrome/browser/views/bookmark_manager_view.h (right):

http://codereview.chromium.org/418044/diff/4008/5003#newcode232
chrome/browser/views/bookmark_manager_view.h:232: bool sync_status_error_;
Please change the name of this member to 'sync_relogin_required_'. Also, change
to comment to something like: "True if the cached credentials have expired and
we need to prompt the user to re-enter their password".

[tfarina (gmail-do not use), 2009-11-25 20:33:07]

Hi Idan,

> Hi Thiago,
> 
> I tested your patch and it seems to work pretty well.
Nice to hear that it works. :)

> Please fix the nit comment
> I have and re-upload the patch. Once you do that, I'll send your patch to the
> try bots and we'll see how it goes.

OK, I uploaded it with the changes required. Please take a look.

http://codereview.chromium.org/418044/diff/4008/5003
File chrome/browser/views/bookmark_manager_view.h (right):

http://codereview.chromium.org/418044/diff/4008/5003#newcode232
chrome/browser/views/bookmark_manager_view.h:232: bool sync_status_error_;
On 2009/11/25 20:12:38, idana wrote:
> Please change the name of this member to 'sync_relogin_required_'. Also,
change
> to comment to something like: "True if the cached credentials have expired and
> we need to prompt the user to re-enter their password".

Renamed, and changed the comment with your suggestion.

[idana, 2009-11-25 21:58:16]

LGTM

I just sent the patch to the try servers.

[idana, 2009-11-26 00:26:29]

Committed. Thanks for fixing this!

[tfarina (gmail-do not use), 2009-11-28 22:31:23]

On 2009/11/26 00:26:29, idana wrote:
> Committed. Thanks for fixing this!

Np. Thanks Idan.