MD Settings: Certificate manager, implement importing CA certificate.

MD Settings: Certificate manager, implement importing CA certificate.

BUG=600813
Committed: https://crrev.com/428e32f0d8e3abf983847314c1a82f1fe0ae935a
Cr-Commit-Position: refs/heads/master@{#385605}

[dpapad, 2016-04-05 23:34:27]

Description was changed from

==========
MD Settings: Certificate manager, import CA certificate.

BUG=600813
==========

to

==========
MD Settings: Certificate manager, implement importing CA certificate.

BUG=600813
==========

[dpapad, 2016-04-05 23:39:06]

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

[dschuyler, 2016-04-05 23:58:11]

LGTM with a question.

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js
(right):

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.id !== null) {
Should the definition of CertificateSubnode
in certificates_browser_proxy.js note the
string as null-able?

[dpapad, 2016-04-06 00:59:00]

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js
(right):

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.id !== null) {
On 2016/04/05 at 23:58:11, dschuyler wrote:
> Should the definition of CertificateSubnode
> in certificates_browser_proxy.js note the
> string as null-able?

Yes, to be accurate id should be marked an nullable, but also all other members
of the CertificateSubnode struct (policy, readonly, untrusted, urlLocked), which
I chose not to do, because I think it would be confusing, especially because
when this data structure comes from C++ none of those members can be null, it
only happens within JS temporarily.

I used an alternative approach to address this issue though, see latest patch
diff. I hope it makes the types more explicit without adding the confusion I
mentioned above. Let me know if that's OK.

[dschuyler, 2016-04-06 01:17:07]

Thanks! I like that better than the null check.  LGTM

[dpapad, 2016-04-06 01:18:40]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-04-06 01:19:20]

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

[dpapad, 2016-04-06 01:19:55]

On 2016/04/06 at 01:17:07, dschuyler wrote:
> Thanks! I like that better than the null check.  LGTM

Thanks for the suggestion.

[commit-bot: I haz the power, 2016-04-06 07:20:32]

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

[commit-bot: I haz the power, 2016-04-06 07:20:33]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dpapad, 2016-04-06 17:15:58]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-04-06 17:16:11]

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

[Dan Beam, 2016-04-06 18:31:08]

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

[Dan Beam, 2016-04-06 18:31:09]

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js
(right):

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.id !== null) {
why not just (!this.model.id)?

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

https://codereview.chromium.org/1863733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.hasOwnProperty('id')) {
p.s. this will break if id is part of the __proto__ chain instead of directly on
|model|

function Model() {}
Model.prototype.id = 'ID';

function FancyModel() {}
FancyModel.prototype = Model.prototype;

new FancyModel().hasOwnProperty('id')

[dpapad, 2016-04-06 18:33:24]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2016-04-06 18:56:52]

@dbeam: I removed the CL from the CQ since you added some comments. Waiting for
your LG before putting it back in the CQ.

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js
(right):

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.id !== null) {
On 2016/04/06 at 18:31:09, Dan Beam wrote:
> why not just (!this.model.id)?

Because 'id' can be zero, and it should still considered a valid ID.

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

https://codereview.chromium.org/1863733003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.hasOwnProperty('id')) {
On 2016/04/06 at 18:31:09, Dan Beam wrote:
> p.s. this will break if id is part of the __proto__ chain instead of directly
on |model|
> 
> function Model() {}
> Model.prototype.id = 'ID';
> 
> function FancyModel() {}
> FancyModel.prototype = Model.prototype;
> 
> new FancyModel().hasOwnProperty('id')

model is a plain JS object, so that is currently not an issue. If we decide in
the future to convert the model into a proper class, then there is no need to
look up a property like 'id' to differentiate between to slightly different
models, we could simply do "instanceof CertificateSubnode" and "instanceof
NewCertificateSubnode". Currently both of these are simply typedefs.

[Dan Beam, 2016-04-06 19:00:55]

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js
(right):

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.id !== null) {
On 2016/04/06 18:56:52, dpapad wrote:
> On 2016/04/06 at 18:31:09, Dan Beam wrote:
> > why not just (!this.model.id)?
> 
> Because 'id' can be zero, and it should still considered a valid ID.

I thought CertificateSubnode#id is a string?

[dpapad, 2016-04-06 19:08:10]

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js
(right):

https://codereview.chromium.org/1863733003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.js:41:
if (this.model.id !== null) {
On 2016/04/06 at 19:00:54, Dan Beam wrote:
> On 2016/04/06 18:56:52, dpapad wrote:
> > On 2016/04/06 at 18:31:09, Dan Beam wrote:
> > > why not just (!this.model.id)?
> > 
> > Because 'id' can be zero, and it should still considered a valid ID.
> 
> I thought CertificateSubnode#id is a string?

Ah right, in this case zero is not a possibility, so done, see latest patch.

[Dan Beam, 2016-04-06 19:12:08]

cool! lgtm

[dpapad, 2016-04-06 19:25:59]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-04-06 19:26:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/1863733003/#ps120001
(title: "Remove hasOwnProperty.")

[commit-bot: I haz the power, 2016-04-06 19:26:40]

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

[commit-bot: I haz the power, 2016-04-06 21:48:27]

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

[commit-bot: I haz the power, 2016-04-06 21:48:28]

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-04-07 00:00:39]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-04-07 00:01:44]

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

[commit-bot: I haz the power, 2016-04-07 00:38:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-07 00:40:29]

Description was changed from

==========
MD Settings: Certificate manager, implement importing CA certificate.

BUG=600813
==========

to

==========
MD Settings: Certificate manager, implement importing CA certificate.

BUG=600813

Committed: https://crrev.com/428e32f0d8e3abf983847314c1a82f1fe0ae935a
Cr-Commit-Position: refs/heads/master@{#385605}
==========

[commit-bot: I haz the power, 2016-04-07 00:40:30]

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