[Password Manager] Add link to remotely manage passowrds on desktop

[Password Manager] Add link to remotely manage passowrds on desktop

Similar UI has already been added to mobile platforms. Functionality is hidden behind a flag until the passwords website launches.

BUG=426508
R=estade@chromium.org, isherman@chromium.org, mkwst@chromium.org, nyquist@chromium.org

Committed: https://crrev.com/222991206441e3760bc972fbfba562e832e5e256
Cr-Commit-Position: refs/heads/master@{#302486}

[Garrett Casto, 2014-10-28 21:48:34]

Patchset #1 (id:1) has been deleted

[Garrett Casto, 2014-10-28 21:50:43]

gcasto@chromium.org changed reviewers:
+ estade@chromium.org, mkwst@chromium.org, nyquist@chromium.org

[Garrett Casto, 2014-10-28 21:50:43]

estade@ - WebUI changes
mkwst@ - Password manager changes
nyquist@ - Android changes

[nyquist, 2014-10-28 22:19:52]

chrome/browser/android lgtm

[Evan Stade, 2014-10-28 22:50:48]

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.css (right):

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.css:31: text-decoration: none;
pretty sure this rule should not be here. Links to external websites in
chrome://settings get underlined.

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.css:34: #manage-passwords-span
{
don't think you need this either. The span should just be inline.

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.js:219:
$('manage-passwords-span').style.display = 'block';
$('manage-passwords-span').hidden = !visible;

[Garrett Casto, 2014-10-28 23:18:55]

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.css (right):

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.css:31: text-decoration: none;
On 2014/10/28 22:50:48, Evan Stade wrote:
> pretty sure this rule should not be here. Links to external websites in
> chrome://settings get underlined.

Okay. Mocks had this without the underline and I saw some instances of links
that didn't underline, but it does look like they are all internal links of
various sorts. Removed.

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.css:34: #manage-passwords-span
{
On 2014/10/28 22:50:48, Evan Stade wrote:
> don't think you need this either. The span should just be inline.

To be clear, are you saying that this isn't necessary because it doesn't do
anything useful or because we don't want it to look like this? This span is in
flex box with end packing so without it the text is right justified. I think it
both looks better and is more consistent to have it left justified. The Autofill
management page uses this exact same styling for a similar link.

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.js:219:
$('manage-passwords-span').style.display = 'block';
On 2014/10/28 22:50:48, Evan Stade wrote:
> $('manage-passwords-span').hidden = !visible;

Done.

[Evan Stade, 2014-10-28 23:26:01]

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.css (right):

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.css:34: #manage-passwords-span
{
On 2014/10/28 23:18:55, Garrett Casto wrote:
> On 2014/10/28 22:50:48, Evan Stade wrote:
> > don't think you need this either. The span should just be inline.
> 
> To be clear, are you saying that this isn't necessary because it doesn't do
> anything useful or because we don't want it to look like this? This span is in
> flex box with end packing so without it the text is right justified. I think
it
> both looks better and is more consistent to have it left justified. The
Autofill
> management page uses this exact same styling for a similar link.

both places should add a spacer div right after the link. That spacer div has
-webkit-box-flex: 1. Then this rule can go away. That's preferable because

a) 18px is a magic number
b) position: absolute is hacky

[Mike West, 2014-10-29 07:19:51]

password manager changes LGTM.

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

https://codereview.chromium.org/682133003/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13151: +      Manage passwords remotely on
you |Google Account
s/you/your/

[Garrett Casto, 2014-10-29 20:25:46]

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.css (right):

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.css:34: #manage-passwords-span
{
On 2014/10/28 23:26:01, Evan Stade wrote:
> On 2014/10/28 23:18:55, Garrett Casto wrote:
> > On 2014/10/28 22:50:48, Evan Stade wrote:
> > > don't think you need this either. The span should just be inline.
> > 
> > To be clear, are you saying that this isn't necessary because it doesn't do
> > anything useful or because we don't want it to look like this? This span is
in
> > flex box with end packing so without it the text is right justified. I think
> it
> > both looks better and is more consistent to have it left justified. The
> Autofill
> > management page uses this exact same styling for a similar link.
> 
> both places should add a spacer div right after the link. That spacer div has
> -webkit-box-flex: 1. Then this rule can go away. That's preferable because
> 
> a) 18px is a magic number
> b) position: absolute is hacky

Sounds good. Unfortunately this fails in PRESUBMIT because you apparently aren't
supposed to use the prefixed version anymore. However just changing this to use
the standardized alternative ("flex-grow") doesn't work unless you also change
action-area to use "display:flex" instead of "display: -webkit-box". This change
seems a little scary given the amount of pages that use "action-area". Would you
prefer that I try to fix all of them (presumably in another CL), add
standardized styles to overlay.css as well as the current prefixed versions, or
ignore the PRESUBMIT?

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

https://codereview.chromium.org/682133003/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13151: +      Manage passwords remotely on
you |Google Account
On 2014/10/29 07:19:51, Mike West (OOO until 3rd) wrote:
> s/you/your/

Yikes. Fixed.

[Evan Stade, 2014-10-29 21:28:28]

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.css (right):

https://codereview.chromium.org/682133003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.css:34: #manage-passwords-span
{
On 2014/10/29 20:25:46, Garrett Casto wrote:
> On 2014/10/28 23:26:01, Evan Stade wrote:
> > On 2014/10/28 23:18:55, Garrett Casto wrote:
> > > On 2014/10/28 22:50:48, Evan Stade wrote:
> > > > don't think you need this either. The span should just be inline.
> > > 
> > > To be clear, are you saying that this isn't necessary because it doesn't
do
> > > anything useful or because we don't want it to look like this? This span
is
> in
> > > flex box with end packing so without it the text is right justified. I
think
> > it
> > > both looks better and is more consistent to have it left justified. The
> > Autofill
> > > management page uses this exact same styling for a similar link.
> > 
> > both places should add a spacer div right after the link. That spacer div
has
> > -webkit-box-flex: 1. Then this rule can go away. That's preferable because
> > 
> > a) 18px is a magic number
> > b) position: absolute is hacky
> 
> Sounds good. Unfortunately this fails in PRESUBMIT because you apparently
aren't
> supposed to use the prefixed version anymore. However just changing this to
use
> the standardized alternative ("flex-grow") doesn't work unless you also change
> action-area to use "display:flex" instead of "display: -webkit-box". This
change
> seems a little scary given the amount of pages that use "action-area". Would
you
> prefer that I try to fix all of them (presumably in another CL), add
> standardized styles to overlay.css as well as the current prefixed versions,
or
> ignore the PRESUBMIT?

I think it's ok to ignore this PRESUBMIT check.

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

https://codereview.chromium.org/682133003/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13151: +      Manage passwords remotely on
you |Google Account
ah, sorry for missing this before. I don't think you can assume that for every
language there will only need to be a single separator. For example, some
languages would be something akin to:

Manage passwords remotely on |Google Account| yours.

[Garrett Casto, 2014-10-29 22:05:35]

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

https://codereview.chromium.org/682133003/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13151: +      Manage passwords remotely on
you |Google Account
On 2014/10/29 21:28:28, Evan Stade wrote:
> ah, sorry for missing this before. I don't think you can assume that for every
> language there will only need to be a single separator. For example, some
> languages would be something akin to:
> 
> Manage passwords remotely on |Google Account| yours.

Makes sense. Fixed.

[Evan Stade, 2014-10-29 22:17:36]

https://codereview.chromium.org/682133003/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.html (right):

https://codereview.chromium.org/682133003/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.html:38: <div
id="spacer-div"></div>
you're not really supposed to have two divs with the same ID in the same page.
This should be a class instead of id.

[Garrett Casto, 2014-10-29 23:26:09]

gcasto@chromium.org changed reviewers:
+ isherman@chromium.org

[Garrett Casto, 2014-10-29 23:26:09]

+isherman for histogram changes.

https://codereview.chromium.org/682133003/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/password_manager.html (right):

https://codereview.chromium.org/682133003/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/password_manager.html:38: <div
id="spacer-div"></div>
On 2014/10/29 22:17:35, Evan Stade wrote:
> you're not really supposed to have two divs with the same ID in the same page.
> This should be a class instead of id.

Done.

[Ilya Sherman, 2014-10-29 23:45:18]

https://codereview.chromium.org/682133003/diff/120001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/682133003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46699: -  <int value="-1241747717"
label="enable-android-password-link"/>
Please leave old ones in place.

[Evan Stade, 2014-10-29 23:51:57]

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/autofill_options.css (right):

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/autofill_options.css:49: #autofill-options
.spacer-div {
why two rules for the same thing?

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/password_manager.css (right):

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/password_manager.css:30: #password-manger
.spacer-div {
heh... password-manger

[Garrett Casto, 2014-10-30 00:14:44]

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/autofill_options.css (right):

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/autofill_options.css:49: #autofill-options
.spacer-div {
On 2014/10/29 23:51:57, Evan Stade wrote:
> why two rules for the same thing?

Wasn't sure what the right place to put a shared rule would be. Ended up using
overlay.css. Hopefully that's reasonable.

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/password_manager.css (right):

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/password_manager.css:30: #password-manger
.spacer-div {
On 2014/10/29 23:51:57, Evan Stade wrote:
> heh... password-manger

N/A

https://codereview.chromium.org/682133003/diff/120001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/682133003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46699: -  <int value="-1241747717"
label="enable-android-password-link"/>
On 2014/10/29 23:45:17, Ilya Sherman wrote:
> Please leave old ones in place.

Done. So when do you remove old flags? After the change has propagated to
stable? Never?

[Ilya Sherman, 2014-10-30 00:46:58]

histograms.xml lgtm

https://codereview.chromium.org/682133003/diff/120001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/682133003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46699: -  <int value="-1241747717"
label="enable-android-password-link"/>
On 2014/10/30 00:14:44, Garrett Casto wrote:
> On 2014/10/29 23:45:17, Ilya Sherman wrote:
> > Please leave old ones in place.
> 
> Done. So when do you remove old flags? After the change has propagated to
> stable? Never?

Never.  histograms.xml is essentially an insert-only file, because it's used to
decode data from old Chrome instances as well as from current ones.

[Evan Stade, 2014-10-31 18:23:31]

lgtm

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/autofill_options.css (right):

https://codereview.chromium.org/682133003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/autofill_options.css:49: #autofill-options
.spacer-div {
On 2014/10/30 00:14:44, Garrett Casto wrote:
> On 2014/10/29 23:51:57, Evan Stade wrote:
> > why two rules for the same thing?
> 
> Wasn't sure what the right place to put a shared rule would be. Ended up using
> overlay.css. Hopefully that's reasonable.

seems legit

[Garrett Casto, 2014-11-03 21:15:05]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-11-03 21:17:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/682133003/140001

[commit-bot: I haz the power, 2014-11-03 21:25:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-03 21:25:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Garrett Casto, 2014-11-03 21:38:43]

Committed patchset #7 (id:140001) manually as
222991206441e3760bc972fbfba562e832e5e256.

[commit-bot: I haz the power, 2014-11-03 21:38:43]

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