MD Settings: Certificate manager, delete confirmation dialog.

MD Settings: Certificate manager, delete confirmation dialog.

BUG=547073
Committed: https://crrev.com/01ae227eaf011cd802f7d8363d525cc1cc72afcc
Cr-Commit-Position: refs/heads/master@{#380545}

[dpapad, 2016-03-10 00:23:50]

Description was changed from

==========
MD Settings: Certificate manager, delete confirmation dialogs.

BUG=547073
==========

to

==========
MD Settings: Certificate manager, delete confirmation dialog.

BUG=547073
==========

[dpapad, 2016-03-10 01:14:53]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-03-10 01:14:59]

Patchset #3 (id:40001) has been deleted

[dpapad, 2016-03-10 01:15:04]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-03-10 01:15:11]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-03-10 01:24:11]

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

[dpapad, 2016-03-10 01:24:11]

Screenshots at http://imgur.com/a/gxlIQ. Depending on the certificate type that
is being deleted the dialog displays a different title and description.

[Dan Beam, 2016-03-10 05:03:06]

On 2016/03/10 01:24:11, dpapad wrote:
> Screenshots at http://imgur.com/a/gxlIQ. Depending on the certificate type
that
> is being deleted the dialog displays a different title and description.

one of them doesn't have a description.  what's up with that?

either way: lgtm

https://codereview.chromium.org/1784553004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js
(right):

https://codereview.chromium.org/1784553004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:49:
default: // settings.CertificateType.OTHER
why are you using default instead of case settings.CertificateType.OTHER: to
enumerate all types?

https://codereview.chromium.org/1784553004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:74:
return localizedMessageId == null ? '' :
nit: maybe just return from each case with an

  assertNotReached();

at the bottom so the compiler knows a string is always returned?

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

https://codereview.chromium.org/1784553004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/certificate_manager_page_test.js:20:
'deleteCertificate',
nit: alphabetize, maybe?

[dpapad, 2016-03-10 18:31:07]

Yes, the confirmation dialog for the case of OTHER certificate type has no
description, see old Options similar dialog http://imgur.com/ydkPKEn. I am just
following old Options since we don't actually have mocks for any of those
dialogs.

https://codereview.chromium.org/1784553004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js
(right):

https://codereview.chromium.org/1784553004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:49:
default: // settings.CertificateType.OTHER
On 2016/03/10 at 05:03:05, Dan Beam wrote:
> why are you using default instead of case settings.CertificateType.OTHER: to
enumerate all types?

Changed. I was using "default" for the only remaining type, which must be OTHER
at this point (indicated by the comment), but assertNotReached() is probably
better.

https://codereview.chromium.org/1784553004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:74:
return localizedMessageId == null ? '' :
On 2016/03/10 at 05:03:05, Dan Beam wrote:
> nit: maybe just return from each case with an
> 
>   assertNotReached();
> 
> at the bottom so the compiler knows a string is always returned?

Changed similarly to previous comment. Regardless of this change, a string is
always returned (execution always gets to line 74), so I don't see any problems
for the compiler here.

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

https://codereview.chromium.org/1784553004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/certificate_manager_page_test.js:20:
'deleteCertificate',
On 2016/03/10 at 05:03:06, Dan Beam wrote:
> nit: alphabetize, maybe?

Done.

[dpapad, 2016-03-10 18:43:41]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-03-10 18:43:42]

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/1784553004/#ps100001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-03-10 18:44:23]

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

[Dan Beam, 2016-03-10 19:01:26]

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js
(right):

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:52:
default:
if you're enumerating every case, remove the default:, it may give you better
compiler coverage

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:63:
var localizedMessageId = null;
nit: why null over undefined?

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:79:
return localizedMessageId == null ? '' :
nit: use === when multiple falsy values are involved

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

https://codereview.chromium.org/1784553004/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:14: var
TestCertificatesBrowserProxy = function() {
also, it probably makes sense to figure out if we can extract some similarities
between this and other tests with regard to Test*BrowserProxy

[dpapad, 2016-03-10 19:08:50]

The CQ bit was unchecked by dpapad@chromium.org

[Dan Beam, 2016-03-10 19:53:53]

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js
(right):

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:52:
default:
On 2016/03/10 19:01:26, Dan Beam wrote:
> if you're enumerating every case, remove the default:, it may give you better
> compiler coverage

here's some examples of doing this to make both compile-time and run-time
checking possible:
https://gist.github.com/anonymous/f4b1abfbcffb075c5c2b
https://gist.github.com/anonymous/e4f94b232bd0c26649cf

[dpapad, 2016-03-10 20:21:12]

Regarding Test**BrowserProxy, I think we can extract a superclass that holds
some common functionality (will try that in a separate CL).

PTAL.

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js
(right):

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:52:
default:
On 2016/03/10 at 19:53:53, Dan Beam wrote:
> On 2016/03/10 19:01:26, Dan Beam wrote:
> > if you're enumerating every case, remove the default:, it may give you
better
> > compiler coverage
> 
> here's some examples of doing this to make both compile-time and run-time
checking possible:
> https://gist.github.com/anonymous/f4b1abfbcffb075c5c2b
> https://gist.github.com/anonymous/e4f94b232bd0c26649cf

Changed per suggestion.

As a side-comment, suggestion 1 relies on "assertNotReached()" being handled
specially by Chromium's version of the compiler, which means future readers need
this implicit knowledge to understand how this is passing the "missing return"
compiler check.

Similar example with vanilla compiler fails, see
https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClo...

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:63:
var localizedMessageId = null;
On 2016/03/10 at 19:01:26, Dan Beam wrote:
> nit: why null over undefined?

Removed.

For the record, I think "null" serves as a much better sentinel value
(https://en.wikipedia.org/wiki/Sentinel_value) compared to "undefined". The
latter can be the result of various programmer errors, whereas when a variable
has a null value, it means that it was explicitly assigned to null at some
previous execution point.

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:79:
return localizedMessageId == null ? '' :
On 2016/03/10 at 19:01:26, Dan Beam wrote:
> nit: use === when multiple falsy values are involved

Removed.

[Dan Beam, 2016-03-10 21:44:13]

still lgtm

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js
(right):

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:52:
default:
On 2016/03/10 20:21:12, dpapad wrote:
> On 2016/03/10 at 19:53:53, Dan Beam wrote:
> > On 2016/03/10 19:01:26, Dan Beam wrote:
> > > if you're enumerating every case, remove the default:, it may give you
> better
> > > compiler coverage
> > 
> > here's some examples of doing this to make both compile-time and run-time
> checking possible:
> > https://gist.github.com/anonymous/f4b1abfbcffb075c5c2b
> > https://gist.github.com/anonymous/e4f94b232bd0c26649cf
> 
> Changed per suggestion.
> 
> As a side-comment, suggestion 1 relies on "assertNotReached()" being handled
> specially by Chromium's version of the compiler, which means future readers
need
> this implicit knowledge to understand how this is passing the "missing return"
> compiler check.

feel free to fix this however you see fit

> 
> Similar example with vanilla compiler fails, see
>
https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClo...

i recommend using the same compiler that will check your code

https://codereview.chromium.org/1784553004/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_delete_confirmation_dialog.js:63:
var localizedMessageId = null;
On 2016/03/10 20:21:12, dpapad wrote:
> On 2016/03/10 at 19:01:26, Dan Beam wrote:
> > nit: why null over undefined?
> 
> Removed.
> 
> For the record, I think "null" serves as a much better sentinel value
> (https://en.wikipedia.org/wiki/Sentinel_value) compared to "undefined". The
> latter can be the result of various programmer errors, whereas when a variable
> has a null value, it means that it was explicitly assigned to null at some
> previous execution point.

either one is fine, null just requires an assignment

generally: i don't really see why they both exist, but that ship has sailed.

I do know that undefined is the language's default state

  var blah;  // typeof blah == 'undefined'
  typeof window.randomkey // == 'undefined'

either are fine, i was just wondering why

[dpapad, 2016-03-10 21:48:37]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-10 21:49:24]

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

[commit-bot: I haz the power, 2016-03-10 22:26:23]

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

[commit-bot: I haz the power, 2016-03-10 22:26:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dpapad, 2016-03-10 22:42:29]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-10 22:43:06]

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

[commit-bot: I haz the power, 2016-03-10 23:16:07]

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

[commit-bot: I haz the power, 2016-03-10 23:16:08]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2016-03-10 23:41:46]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-10 23:43:05]

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

[commit-bot: I haz the power, 2016-03-11 00:22:24]

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

[commit-bot: I haz the power, 2016-03-11 00:22:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dpapad, 2016-03-11 03:05:16]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-11 03:05:30]

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

[commit-bot: I haz the power, 2016-03-11 05:42:17]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-03-11 05:44:29]

Description was changed from

==========
MD Settings: Certificate manager, delete confirmation dialog.

BUG=547073
==========

to

==========
MD Settings: Certificate manager, delete confirmation dialog.

BUG=547073

Committed: https://crrev.com/01ae227eaf011cd802f7d8363d525cc1cc72afcc
Cr-Commit-Position: refs/heads/master@{#380545}
==========

[commit-bot: I haz the power, 2016-03-11 05:44:30]

Patchset 3 (id:??) landed as
https://crrev.com/01ae227eaf011cd802f7d8363d525cc1cc72afcc
Cr-Commit-Position: refs/heads/master@{#380545}