[protector] Homepage UI polishing.

[protector] Homepage UI polishing.

*) Bubble always shown, even when user has no homepage button.
*) When old and homepage are the same, a generic caption is used for the Discard button.


BUG=114298
TEST=HomepageChangeTest.*, manual


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

[Ivan Korotkov, 2012-04-28 16:26:22]

Small UI cleanup, please take a look

[whywhat, 2012-04-29 16:24:15]

lgtm

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change.cc:166: // Display a generic string if
old setting looks the same as the backup (for
did you mean new setting by old setting?
won't it still be [ Keep settings ] [ Change to Google ] in the case of false
positive and still confusing to users?

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change_unittest.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change_unittest.cc:79: // NTP -> NTP.
The comments before the last three sections are identical, but code is
different. What's the truth?
Also, consider splitting test cases into several to make it easier to understand
which case was broken (it still could be done by line number now).

[Ivan Korotkov, 2012-05-02 11:17:26]

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change.cc:166: // Display a generic string if
old setting looks the same as the backup (for
On 2012/04/29 16:24:15, whywhat wrote:
> did you mean new setting by old setting?
> won't it still be [ Keep settings ] [ Change to Google ] in the case of false
> positive and still confusing to users?

Yep, typo.
It's not a false positive, just the case when not the homepage URL but something
other (the 'show button' checkbox) has changed and we decided to show the bubble
anyway in that case.

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change_unittest.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change_unittest.cc:79: // NTP -> NTP.
On 2012/04/29 16:24:15, whywhat wrote:
> The comments before the last three sections are identical, but code is
> different. What's the truth?
> Also, consider splitting test cases into several to make it easier to
understand
> which case was broken (it still could be done by line number now).

You're right, they'd better be separated.
NTP can be represented either as empty homepage URL or any URL with
"homepage_is_ntp = true", hence different code sections with the same comment.

[commit-bot: I haz the power, 2012-05-02 11:17:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/10261002/6001

[whywhat, 2012-05-02 11:23:48]

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change.cc:166: // Display a generic string if
old setting looks the same as the backup (for
On 2012/05/02 11:17:26, Ivan Korotkov wrote:
> On 2012/04/29 16:24:15, whywhat wrote:
> > did you mean new setting by old setting?
> > won't it still be [ Keep settings ] [ Change to Google ] in the case of
false
> > positive and still confusing to users?
> 
> Yep, typo.
> It's not a false positive, just the case when not the homepage URL but
something
> other (the 'show button' checkbox) has changed and we decided to show the
bubble
> anyway in that case.

Anyway, sounds confusing to me. Imagine that I had Google as my homepage, then
something changed my settings and I'm asked if I want to keep settings (which
was Google) or change it to Google. I'm asking myself what was it if not Google.
Not to mention, that I think "Keep setting" could be read as "Keep [new]
setting" so I'd click on Change to Google.

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change_unittest.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change_unittest.cc:79: // NTP -> NTP.
On 2012/05/02 11:17:26, Ivan Korotkov wrote:
> On 2012/04/29 16:24:15, whywhat wrote:
> > The comments before the last three sections are identical, but code is
> > different. What's the truth?
> > Also, consider splitting test cases into several to make it easier to
> understand
> > which case was broken (it still could be done by line number now).
> 
> You're right, they'd better be separated.
> NTP can be represented either as empty homepage URL or any URL with
> "homepage_is_ntp = true", hence different code sections with the same comment.

My point is that the comments should explain this difference between the
sections.

[Ivan Korotkov, 2012-05-02 11:29:40]

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change.cc:166: // Display a generic string if
old setting looks the same as the backup (for
On 2012/05/02 11:23:48, whywhat wrote:
> On 2012/05/02 11:17:26, Ivan Korotkov wrote:
> > On 2012/04/29 16:24:15, whywhat wrote:
> > > did you mean new setting by old setting?
> > > won't it still be [ Keep settings ] [ Change to Google ] in the case of
> false
> > > positive and still confusing to users?
> > 
> > Yep, typo.
> > It's not a false positive, just the case when not the homepage URL but
> something
> > other (the 'show button' checkbox) has changed and we decided to show the
> bubble
> > anyway in that case.
> 
> Anyway, sounds confusing to me. Imagine that I had Google as my homepage, then
> something changed my settings and I'm asked if I want to keep settings (which
> was Google) or change it to Google. I'm asking myself what was it if not
Google.
> Not to mention, that I think "Keep setting" could be read as "Keep [new]
> setting" so I'd click on Change to Google.

The string is "Keep my settings" to make it clearer. I agree  with you it's not
informative but, as, you know, we want to simplify these bubbles as much as
possible...
It's a rare event anyway that something external would change the 'show homepage
button' but not the homepage itself :)

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
File chrome/browser/protector/homepage_change_unittest.cc (right):

http://codereview.chromium.org/10261002/diff/1/chrome/browser/protector/homep...
chrome/browser/protector/homepage_change_unittest.cc:79: // NTP -> NTP.
On 2012/05/02 11:23:48, whywhat wrote:
> On 2012/05/02 11:17:26, Ivan Korotkov wrote:
> > On 2012/04/29 16:24:15, whywhat wrote:
> > > The comments before the last three sections are identical, but code is
> > > different. What's the truth?
> > > Also, consider splitting test cases into several to make it easier to
> > understand
> > > which case was broken (it still could be done by line number now).
> > 
> > You're right, they'd better be separated.
> > NTP can be represented either as empty homepage URL or any URL with
> > "homepage_is_ntp = true", hence different code sections with the same
comment.
> 
> My point is that the comments should explain this difference between the
> sections.

Done.

[commit-bot: I haz the power, 2012-05-02 11:30:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/10261002/11003

[commit-bot: I haz the power, 2012-05-02 13:11:46]

Change committed as 134898