Do not show untrustworthy strings in the basic auth dialog.

Do not show untrustworthy strings in the basic auth dialog.

Browser chrome should display only trustworthy or verified strings.

BUG=544244
Committed: https://crrev.com/d4fe8211476a0bba1a347204e430aa283c2e7d7f
Cr-Commit-Position: refs/heads/master@{#363658}

[palmer, 2015-11-20 00:22:36]

palmer@chromium.org changed reviewers:
+ felt@chromium.org, meacer@chromium.org, msw@chromium.org

[palmer, 2015-11-20 00:22:37]

PTAL. Thanks!

[meacer, 2015-11-20 00:31:31]

https://codereview.chromium.org/1466473003/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/1466473003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:3444: -        The proxy <ph
name="DOMAIN">$1<ex>google.com</ex></ph> requires a username and password. The
proxy says: <ph name="REALM">$2<ex>opaque string</ex></ph>.
The proxy strings were just added today by
https://codereview.chromium.org/1460873003

I think we should keep differentiating between proxy and http auth.

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
File chrome/browser/ui/views/login_view.cc (right):

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
chrome/browser/ui/views/login_view.cc:9: #include "ui/views/background.h"
Not needed I think

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
File chrome/browser/ui/views/login_view.h (right):

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
chrome/browser/ui/views/login_view.h:56: // Authority and realm messages.
Authority and explanation messages?

[palmer, 2015-11-20 01:29:53]

https://codereview.chromium.org/1466473003/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (left):

https://codereview.chromium.org/1466473003/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:3444: -        The proxy <ph
name="DOMAIN">$1<ex>google.com</ex></ph> requires a username and password. The
proxy says: <ph name="REALM">$2<ex>opaque string</ex></ph>.
On 2015/11/20 00:31:30, Mustafa Emre Acer wrote:
> The proxy strings were just added today by
> https://codereview.chromium.org/1460873003
> 
> I think we should keep differentiating between proxy and http auth.

Done.

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
File chrome/browser/ui/views/login_view.cc (right):

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
chrome/browser/ui/views/login_view.cc:9: #include "ui/views/background.h"
On 2015/11/20 00:31:30, Mustafa Emre Acer wrote:
> Not needed I think

Done.

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
File chrome/browser/ui/views/login_view.h (right):

https://codereview.chromium.org/1466473003/diff/1/chrome/browser/ui/views/log...
chrome/browser/ui/views/login_view.h:56: // Authority and realm messages.
On 2015/11/20 00:31:30, Mustafa Emre Acer wrote:
> Authority and explanation messages?

Done.

[meacer, 2015-11-20 01:36:14]

Lgtm with a question

https://codereview.chromium.org/1466473003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/login_view.cc (right):

https://codereview.chromium.org/1466473003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/login_view.cc:33: message_label_(new
views::Label(explanation)),
Looks like this leaks message_label_ if explanation is empty?

[palmer, 2015-11-20 02:23:31]

https://codereview.chromium.org/1466473003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/login_view.cc (right):

https://codereview.chromium.org/1466473003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/login_view.cc:33: message_label_(new
views::Label(explanation)),
On 2015/11/20 01:36:14, Mustafa Emre Acer wrote:
> Looks like this leaks message_label_ if explanation is empty?

Done.

[palmer, 2015-11-20 19:09:46]

palmer@chromium.org changed reviewers:
+ groby@chromium.org, yfriedman@chromium.org

[palmer, 2015-11-20 19:09:47]

PTAL, all. Thanks!

meacer: c/b/ui/login
msw: c/b/ui/views
groby: c/b/ui/cocoa
yfriedman: c/b/ui/android

[meacer, 2015-11-20 19:14:02]

c/b/ui/login lgtm

[msw, 2015-11-20 19:15:07]

lgtm with nits, but please post screenshots to the bug.

https://codereview.chromium.org/1466473003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1466473003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3444: +        The connection to this site is
not secure.
nit: is there a similar existing string that we could use instead?
(maybe there's nothing close enough, but that's kinda surprising)

https://codereview.chromium.org/1466473003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/chrome_http_auth_handler.cc (right):

https://codereview.chromium.org/1466473003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_http_auth_handler.cc:27: : observer_(NULL),
authority_(authority), explanation_(explanation) {}
nit: nullptr

https://codereview.chromium.org/1466473003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_http_auth_handler.cc:82: if
(explanation_.length() == 0)
nit: if (explanation_.empty())

[palmer, 2015-11-20 19:33:48]

https://codereview.chromium.org/1466473003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1466473003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3444: +        The connection to this site is
not secure.
On 2015/11/20 19:15:07, msw wrote:
> nit: is there a similar existing string that we could use instead?
> (maybe there's nothing close enough, but that's kinda surprising)

Done.

https://codereview.chromium.org/1466473003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/chrome_http_auth_handler.cc (right):

https://codereview.chromium.org/1466473003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_http_auth_handler.cc:27: : observer_(NULL),
authority_(authority), explanation_(explanation) {}
On 2015/11/20 19:15:07, msw wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1466473003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/chrome_http_auth_handler.cc:82: if
(explanation_.length() == 0)
On 2015/11/20 19:15:07, msw wrote:
> nit: if (explanation_.empty())

Done.

[felt, 2015-11-20 20:19:21]

lgtm

[groby-ooo-7-16, 2015-11-20 22:12:56]

I'm not sure how this would work on Cocoa, since the field has not been added to
the xib (I.e. the description for the dialog layout)

[Yaron, 2015-11-23 16:56:14]

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/chrome_http_auth_handler.cc (right):

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/chrome_http_auth_handler.cc:85: env, authority_ +
base::ASCIIToUTF16(" ") + explanation_);
You may want to put this into another TextView. Otherwise on a landscape tablet,
this will just be a continuation of the previous string - is that what you want?

[palmer, 2015-11-24 00:15:42]

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/chrome_http_auth_handler.cc (right):

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/chrome_http_auth_handler.cc:85: env, authority_ +
base::ASCIIToUTF16(" ") + explanation_);
On 2015/11/23 16:56:14, Yaron wrote:
> You may want to put this into another TextView. Otherwise on a landscape
tablet,
> this will just be a continuation of the previous string - is that what you
want?

Well, there's not much vertical room to spare. You can see screenshots on the
bug: https://code.google.com/p/chromium/issues/detail?id=544244#c19

[groby-ooo-7-16, 2015-11-24 01:41:56]

Sorry for the xib nitpickery - I never trust newer IB versions. 

Based on the screenshots, you have tested this on OSX? (If so, LGTM % xib fixes)

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
File chrome/app/nibs/HttpAuthLoginSheet.xib (right):

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:12: <outlet property="authorityField_"
destination="abu-ex-fsZ" id="52"/>
You might want to use a numeric ID - I don't know if random text IDs work for
older IB versions.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:21: <customObject id="-3"
userLabel="Application" customClass="NSObject"/>
Probably want to remove that, not relevant to your change.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:40: <animations/>
Kill all the empty <animations/> tags - I suspect that's a property of a later
interface builder. (Let's just not take chances)

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:74: <rect key="frame" x="168" y="20"
width="231" height="52"/>
Is that size change here deliberate?

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:111: <rect key="frame" x="20" y="74"
width="89" height="68"/>
Deliberately moved?

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:142: <rect key="frame" x="18" y="151"
width="365" height="17"/>
Deliberately moved?

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:152: <rect key="frame" x="19" y="176"
width="365" height="17"/>
That's rather wide, and you seem to have a texfield right after it?

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:155: <textFieldCell key="cell"
sendsActionOnEndEditing="YES" title="authority" id="8wI-KZ-dbK">
Numerical ID, probably.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:177: <point key="canvasLocation" x="681"
y="449"/>
I think you can kill this.

[Yaron, 2015-11-24 16:01:28]

yfriedman@chromium.org changed reviewers:
+ newt@chromium.org

[Yaron, 2015-11-24 16:01:28]

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/chrome_http_auth_handler.cc (right):

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/chrome_http_auth_handler.cc:85: env, authority_ +
base::ASCIIToUTF16(" ") + explanation_);
On 2015/11/24 00:15:42, palmer wrote:
> On 2015/11/23 16:56:14, Yaron wrote:
> > You may want to put this into another TextView. Otherwise on a landscape
> tablet,
> > this will just be a continuation of the previous string - is that what you
> want?
> 
> Well, there's not much vertical room to spare. You can see screenshots on the
> bug: https://code.google.com/p/chromium/issues/detail?id=544244#c19

hmm. I suppose that's reasonable.

+newt in case he has concerns or ideas. I know that in general it's not good to
assume a space between two sentences localizes well but perhaps we don't have
the room here.

[newt (away), 2015-11-24 16:57:28]

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/chrome_http_auth_handler.cc (right):

https://codereview.chromium.org/1466473003/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/chrome_http_auth_handler.cc:85: env, authority_ +
base::ASCIIToUTF16(" ") + explanation_);
On 2015/11/24 16:01:28, Yaron wrote:
> On 2015/11/24 00:15:42, palmer wrote:
> > On 2015/11/23 16:56:14, Yaron wrote:
> > > You may want to put this into another TextView. Otherwise on a landscape
> > tablet,
> > > this will just be a continuation of the previous string - is that what you
> > want?
> > 
> > Well, there's not much vertical room to spare. You can see screenshots on
the
> > bug: https://code.google.com/p/chromium/issues/detail?id=544244#c19
> 
> hmm. I suppose that's reasonable.
> 
> +newt in case he has concerns or ideas. I know that in general it's not good
to
> assume a space between two sentences localizes well but perhaps we don't have
> the room here.

A space between sentences doesn't localize especially well (e.g. Chinese doesn't
use spaces between sentences).

I've wondered what the best option is in cases like this as well, so I just sent
an email to the localization folks (and cc'd you) to hear their thoughts.

[palmer, 2015-12-01 21:15:21]

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
File chrome/app/nibs/HttpAuthLoginSheet.xib (right):

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:12: <outlet property="authorityField_"
destination="abu-ex-fsZ" id="52"/>
On 2015/11/24 01:41:55, groby wrote:
> You might want to use a numeric ID - I don't know if random text IDs work for
> older IB versions.

Done.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:21: <customObject id="-3"
userLabel="Application" customClass="NSObject"/>
On 2015/11/24 01:41:55, groby wrote:
> Probably want to remove that, not relevant to your change.

Done.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:40: <animations/>
On 2015/11/24 01:41:55, groby wrote:
> Kill all the empty <animations/> tags - I suspect that's a property of a later
> interface builder. (Let's just not take chances)

Done.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:111: <rect key="frame" x="20" y="74"
width="89" height="68"/>
On 2015/11/24 01:41:56, groby wrote:
> Deliberately moved?

Yes because of the text field, I believe.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:142: <rect key="frame" x="18" y="151"
width="365" height="17"/>
On 2015/11/24 01:41:55, groby wrote:
> Deliberately moved?

Yes, same reason, I believe.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:152: <rect key="frame" x="19" y="176"
width="365" height="17"/>
On 2015/11/24 01:41:55, groby wrote:
> That's rather wide, and you seem to have a texfield right after it?

I don't know; that's just what Interface Builder output.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:155: <textFieldCell key="cell"
sendsActionOnEndEditing="YES" title="authority" id="8wI-KZ-dbK">
On 2015/11/24 01:41:56, groby wrote:
> Numerical ID, probably.

Done.

https://codereview.chromium.org/1466473003/diff/80001/chrome/app/nibs/HttpAut...
chrome/app/nibs/HttpAuthLoginSheet.xib:177: <point key="canvasLocation" x="681"
y="449"/>
On 2015/11/24 01:41:56, groby wrote:
> I think you can kill this.

Done.

[groby-ooo-7-16, 2015-12-07 21:18:36]

xib lgtm (ilcm[1])

[1] ilcm == i like cryptic messages.

[palmer, 2015-12-07 21:34:29]

yfriedman, newt: We don't seem to be getting an answer from the i18n people. OK
to land this now and fix it to meet their advice later?

[newt (away), 2015-12-07 21:49:12]

On 2015/12/07 21:34:29, palmer wrote:
> yfriedman, newt: We don't seem to be getting an answer from the i18n people.
OK
> to land this now and fix it to meet their advice later?

I'm OK with landing this now and following up to make the localization better if
possible.

[Yaron, 2015-12-07 21:49:57]

lgtm

[palmer, 2015-12-07 23:50:55]

The CQ bit was checked by palmer@chromium.org

[palmer, 2015-12-07 23:50:55]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
meacer@chromium.org, felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1466473003/#ps120001
(title: "Fix XIB.")

[commit-bot: I haz the power, 2015-12-07 23:54:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1466473003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1466473003/120001

[commit-bot: I haz the power, 2015-12-08 01:07:20]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-08 01:08:14]

Description was changed from

==========
Do not show untrustworthy strings in the basic auth dialog.

Browser chrome should display only trustworthy or verified strings.

BUG=544244
==========

to

==========
Do not show untrustworthy strings in the basic auth dialog.

Browser chrome should display only trustworthy or verified strings.

BUG=544244

Committed: https://crrev.com/d4fe8211476a0bba1a347204e430aa283c2e7d7f
Cr-Commit-Position: refs/heads/master@{#363658}
==========

[commit-bot: I haz the power, 2015-12-08 01:08:15]

Patchset 7 (id:??) landed as
https://crrev.com/d4fe8211476a0bba1a347204e430aa283c2e7d7f
Cr-Commit-Position: refs/heads/master@{#363658}