MD Settings: Certificate manager, move "Import" button to the tab level.

MD Settings: Certificate manager, move "Import" button to the tab level.

BUG=595768
Committed: https://crrev.com/d0b9741782d2e63238afb2c740d35898fd4e7710
Cr-Commit-Position: refs/heads/master@{#384963}

[dpapad, 2016-04-01 17:43:28]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[dpapad, 2016-04-01 17:43:29]

Before/after screenshots at https://imgur.com/a/4tLcm.

[Dan Beam, 2016-04-01 18:38:02]

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_list.html
(right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.html:5:
<link rel="import" href="chrome://md-settings/settings_shared_css.html">
add this somewhere for assertNotReached

<link rel="import" href="chrome://resources/html/assert.html">

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.html:15:
<div class="settings-box">
class="settings-box first" drops the border

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_list.js
(right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.js:35:
return get('YourCertificates');
I would recommend not splitting up these identifiers like this.  If I were you,
I'd do:

  behaviors: [I18nBehavior],

and later I think all of these will fit on 1 line:

  return this.i18n('certificateManagerYourCertificatesDescription');

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.js:92:
}
nit: no curlies

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html
(right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html:26:
text-transform: uppercase;
hmmm, why are we differing from Polymer in this regard?  did bettes thing
paper-tabs have ripples as well?

any time i see ALL UPPER text I assume it's got a ripple

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html:54:
<paper-tabs noink selected="{{selected}}">
why no ink?

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/certificate_manager_page_test.js (right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:724: 
nit: \n\n -> \n

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:752:
function(event) {
should this be

  .then(function(event) {

on the same line or more continuation indent?

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:758: });
i'm not trying to enforce my ident on you, but

return browserProxy.whenCalled(proxyMethodName).then(function() {
  return waitForActionEvent;
}.then(function(event) {
  ...
});

seems less convoluted to me

[dpapad, 2016-04-01 21:13:17]

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_list.html
(right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.html:5:
<link rel="import" href="chrome://md-settings/settings_shared_css.html">
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> add this somewhere for assertNotReached
> 
> <link rel="import" href="chrome://resources/html/assert.html">

Done.

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.html:15:
<div class="settings-box">
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> class="settings-box first" drops the border

Done.

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_list.js
(right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.js:35:
return get('YourCertificates');
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> I would recommend not splitting up these identifiers like this.  If I were
you, I'd do:
> 
>   behaviors: [I18nBehavior],
> 
> and later I think all of these will fit on 1 line:
> 
>   return this.i18n('certificateManagerYourCertificatesDescription');

Done. Also modified 18n() definition to call getString instead of getStringF
depending on number of arguments.

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_list.js:92:
}
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html
(right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html:26:
text-transform: uppercase;
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> hmmm, why are we differing from Polymer in this regard?  did bettes thing
paper-tabs have ripples as well?
> 
> any time i see ALL UPPER text I assume it's got a ripple

We don't use paper-tabs anywhere else in settings, so there is no consistency
problem here. With regards to Polymer, I don't know why @bettes chose upper case
here, would have to ask him.

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html:54:
<paper-tabs noink selected="{{selected}}">
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> why no ink?

The mocks don't specify ink/noink on this one. I chose to remove it, because the
ripple effect has a yellow-ish color that just looked weird IMO). We can fine
tune this in a follow up CL, if it requires feedback from @bettes.

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/certificate_manager_page_test.js (right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:724: 
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> nit: \n\n -> \n

Done.

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:752:
function(event) {
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> should this be
> 
>   .then(function(event) {
> 
> on the same line or more continuation indent?

Done.

Personally I am equally OK with either way. My brain has been trained to parse
Promise-based syntax enough to not struggle with the \n positioning. I chose
this syntax because of a comment by @tommycli in a previous review for a similar
case, see
https://codereview.chromium.org/1815733004/diff/80001/chrome/browser/resource....

Ultimately it would be nice if we come to a conclusion about such cases, by
either of the following:
1) Allow flexible formatting, and stop striving to be 100% consistent on \n
positioning across the codebase (with regards to promise callbacks).
2) Find a single formatting style that keeps everyone happy and stick with it.

No1 sounds more viable and realistic IMO. Current situation of formatting \n
based on who is reviewing, leads to the same result as option 1 above (having
both occur in the code), but requires more time (from reviewee and reviewer)
than option 1.

https://codereview.chromium.org/1842403004/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:758: });
On 2016/04/01 at 18:38:02, Dan Beam wrote:
> i'm not trying to enforce my ident on you, but
> 
> return browserProxy.whenCalled(proxyMethodName).then(function() {
>   return waitForActionEvent;
> }.then(function(event) {
>   ...
> });
> 
> seems less convoluted to me

Done.

[Dan Beam, 2016-04-02 02:08:17]

lgtm

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html
(right):

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html:26:
text-transform: uppercase;
On 2016/04/01 21:13:17, dpapad wrote:
> On 2016/04/01 at 18:38:02, Dan Beam wrote:
> > hmmm, why are we differing from Polymer in this regard?  did bettes thing
> paper-tabs have ripples as well?
> > 
> > any time i see ALL UPPER text I assume it's got a ripple
> 
> We don't use paper-tabs anywhere else in settings, so there is no consistency
> problem here. With regards to Polymer, I don't know why @bettes chose upper
case
> here, would have to ask him.

it's because all the demos are IN ALL UPPER (but not through style, the text is
LITERALLY UPPERCASED, UGH)

https://codereview.chromium.org/1842403004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.html:54:
<paper-tabs noink selected="{{selected}}">
On 2016/04/01 21:13:17, dpapad wrote:
> On 2016/04/01 at 18:38:02, Dan Beam wrote:
> > why no ink?
> 
> The mocks don't specify ink/noink on this one. I chose to remove it, because
the
> ripple effect has a yellow-ish color that just looked weird IMO). We can fine
> tune this in a follow up CL, if it requires feedback from @bettes.

because this is unspecified i don't care, but i'd advise against bundling UI
changes without justification or consensus from Alan

https://codereview.chromium.org/1842403004/diff/140001/ui/webui/resources/js/...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1842403004/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:24: return arguments.length > 1 ?
nit:

return arguments.length == 1 ? loadTimeData.getString(id) :
    loadTimeData.getString.apply(loadTimeData, arguments);

or something along these [2] lines ;)

https://codereview.chromium.org/1842403004/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:26:
loadTimeData.getString.apply(loadTimeData, arguments);
this is equivalent to loadTimeData.getString(id)

[dpapad, 2016-04-04 17:25:51]

https://codereview.chromium.org/1842403004/diff/140001/ui/webui/resources/js/...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1842403004/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:24: return arguments.length > 1 ?
On 2016/04/02 at 02:08:16, Dan Beam wrote:
> nit:
> 
> return arguments.length == 1 ? loadTimeData.getString(id) :
>     loadTimeData.getString.apply(loadTimeData, arguments);
> 
> or something along these [2] lines ;)

Done.

https://codereview.chromium.org/1842403004/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:26:
loadTimeData.getString.apply(loadTimeData, arguments);
On 2016/04/02 at 02:08:16, Dan Beam wrote:
> this is equivalent to loadTimeData.getString(id)

Done.

[dpapad, 2016-04-04 17:27:05]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-04-04 17:27:05]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1842403004/#ps160001
(title: "Addressing comment.")

[commit-bot: I haz the power, 2016-04-04 17:27:30]

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

[commit-bot: I haz the power, 2016-04-04 18:41:14]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-04 18:43:48]

Description was changed from

==========
MD Settings: Certificate manager, move "Import" button to the tab level.

BUG=595768
==========

to

==========
MD Settings: Certificate manager, move "Import" button to the tab level.

BUG=595768

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

[commit-bot: I haz the power, 2016-04-04 18:43:49]

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

[michaelpg, 2016-04-05 04:46:01]

michaelpg@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-04-05 04:46:02]

this introduced closure errors

https://codereview.chromium.org/1842403004/diff/160001/ui/webui/resources/js/...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1842403004/diff/160001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:20: * @param {...string=} var_args
Placeholders required by the string ($0-9).
(ERROR) Error in: i18n_behavior.js
##
/usr/local/google/home/michaelpg/dev/c/work3/src/ui/webui/resources/js/i18n_behavior.js:20:
ERROR - Bad type annotation. expected closing }
##    * @param {...string=} var_args Placeholders required by the string ($0-9).
##                       ^
## 
##
/usr/local/google/home/michaelpg/dev/c/work3/src/ui/webui/resources/js/i18n_behavior.js:20:
ERROR - Bad type annotation. expecting a variable name in a @param tag
##    * @param {...string=} var_args Placeholders required by the string ($0-9).
##                       ^

[dpapad, 2016-04-05 16:50:45]

https://codereview.chromium.org/1842403004/diff/160001/ui/webui/resources/js/...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1842403004/diff/160001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:20: * @param {...string=} var_args
Placeholders required by the string ($0-9).
On 2016/04/05 at 04:46:02, michaelpg wrote:
> (ERROR) Error in: i18n_behavior.js
> ##
/usr/local/google/home/michaelpg/dev/c/work3/src/ui/webui/resources/js/i18n_behavior.js:20:
ERROR - Bad type annotation. expected closing }
> ##    * @param {...string=} var_args Placeholders required by the string
($0-9).
> ##                       ^
> ## 
> ##
/usr/local/google/home/michaelpg/dev/c/work3/src/ui/webui/resources/js/i18n_behavior.js:20:
ERROR - Bad type annotation. expecting a variable name in a @param tag
> ##    * @param {...string=} var_args Placeholders required by the string
($0-9).
> ##                       ^

This is addressed by https://codereview.chromium.org/1853323003.