Add export function to WebUI certificate viewer.

Add export function to WebUI certificate viewer.

This adds the export certificate function to the WebUI certificate viewer. Additionally this moves the implementation of extracting certificate details to chrome/browser/ui/webui/certificate_viewer.cc eliminating the need to pass a hex encoded certificate pointer through the WebUI javascript.

BUG=None
TEST=Manually tested that you can export a certificate.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96955

[flackr, 2011-08-09 15:51:27]

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer.cc (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer.cc:134: 
The following code is moved from
chrome/browser/ui/webui/certificate_viewer_ui.cc with minor changes to how the
certificate is selected (first few lines).

[Rick Byers, 2011-08-09 17:29:18]

Looks good to me, just a couple small comments.

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
chrome/browser/resources/certificate_viewer.js:16: $('export').onclick =
function() {
Doesn't seem to be any value in using a lambda here, just do:
$('export').onclick = exportCertificate;

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
chrome/browser/resources/certificate_viewer.js:26:
chrome.send('RequestCertificateInfo', ['']);
can you just send an empty array for the args instead of an array with an empty
string?

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
chrome/browser/resources/certificate_viewer.js:50: .concat([$('export')]);
I'd suggest just using Array.push instead of Array.concat (no need to make
another copy).

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer.cc (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer.cc:124: if (!(args->GetString(0,
&val)))
You should be able to use GetDouble here rather than going through a string (I
think I saw somewhere that it was the preferred style now - even though most
WebUI still passes args as strings).  I spent a few minutes trying to look into
how this works (and whether GetInteger might automatically do the truncation
too) but it's not obvious to me where this is coming from (V8ValueConverter
perhaps).

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer.h (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer.h:74: // fields are returned as a
tree structure with values for certain nodes.
mention the particular callback that is invoked here

[flackr, 2011-08-09 18:25:15]

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
chrome/browser/resources/certificate_viewer.js:16: $('export').onclick =
function() {
On 2011/08/09 17:29:18, Rick Byers wrote:
> Doesn't seem to be any value in using a lambda here, just do:
> $('export').onclick = exportCertificate;

Done.

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
chrome/browser/resources/certificate_viewer.js:26:
chrome.send('RequestCertificateInfo', ['']);
On 2011/08/09 17:29:18, Rick Byers wrote:
> can you just send an empty array for the args instead of an array with an
empty
> string?

Done.

http://codereview.chromium.org/7528027/diff/1/chrome/browser/resources/certif...
chrome/browser/resources/certificate_viewer.js:50: .concat([$('export')]);
On 2011/08/09 17:29:18, Rick Byers wrote:
> I'd suggest just using Array.push instead of Array.concat (no need to make
> another copy).

Done.

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer.cc (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer.cc:124: if (!(args->GetString(0,
&val)))
On 2011/08/09 17:29:18, Rick Byers wrote:
> You should be able to use GetDouble here rather than going through a string (I
> think I saw somewhere that it was the preferred style now - even though most
> WebUI still passes args as strings).  I spent a few minutes trying to look
into
> how this works (and whether GetInteger might automatically do the truncation
> too) but it's not obvious to me where this is coming from (V8ValueConverter
> perhaps).

Done. Both ways seemed a little ugly. I tried using GetInteger but the Javscript
always passes it as a double and it won't automatically truncate.

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer.h (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer.h:74: // fields are returned as a
tree structure with values for certain nodes.
On 2011/08/09 17:29:18, Rick Byers wrote:
> mention the particular callback that is invoked here

Done.

[Rick Byers, 2011-08-09 20:03:41]

LGTM

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
File chrome/browser/ui/webui/certificate_viewer.cc (right):

http://codereview.chromium.org/7528027/diff/1/chrome/browser/ui/webui/certifi...
chrome/browser/ui/webui/certificate_viewer.cc:124: if (!(args->GetString(0,
&val)))
On 2011/08/09 18:25:15, flackr wrote:
> On 2011/08/09 17:29:18, Rick Byers wrote:
> > You should be able to use GetDouble here rather than going through a string
(I
> > think I saw somewhere that it was the preferred style now - even though most
> > WebUI still passes args as strings).  I spent a few minutes trying to look
> into
> > how this works (and whether GetInteger might automatically do the truncation
> > too) but it's not obvious to me where this is coming from (V8ValueConverter
> > perhaps).
> 
> Done. Both ways seemed a little ugly. I tried using GetInteger but the
Javscript
> always passes it as a double and it won't automatically truncate.

Yeah, Javascript has only doubles, so at least this makes some sense..

http://codereview.chromium.org/7528027/diff/5001/chrome/browser/resources/cer...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/5001/chrome/browser/resources/cer...
chrome/browser/resources/certificate_viewer.js:46: // Convert the NodeList to an
array so that we can use concat.
Update this comment now (no longer using concat).  Maybe "copy all the tab
labels into an array"

[flackr, 2011-08-09 20:58:00]

http://codereview.chromium.org/7528027/diff/5001/chrome/browser/resources/cer...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/5001/chrome/browser/resources/cer...
chrome/browser/resources/certificate_viewer.js:46: // Convert the NodeList to an
array so that we can use concat.
On 2011/08/09 20:03:41, Rick Byers wrote:
> Update this comment now (no longer using concat).  Maybe "copy all the tab
> labels into an array"

Done.

[James Hawkins, 2011-08-09 21:00:50]

LGTM with nits.

http://codereview.chromium.org/7528027/diff/10001/chrome/browser/resources/ce...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/10001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:24:
chrome.send('RequestCertificateInfo', []);
No need to pass an empty array param.

http://codereview.chromium.org/7528027/diff/10001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:126: if (item &&
item.detail.payload.index !== undefined) {
No braces for single line logic blocks.

[flackr, 2011-08-09 21:14:23]

http://codereview.chromium.org/7528027/diff/10001/chrome/browser/resources/ce...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/10001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:24:
chrome.send('RequestCertificateInfo', []);
On 2011/08/09 21:00:50, James Hawkins wrote:
> No need to pass an empty array param.

Done.

http://codereview.chromium.org/7528027/diff/10001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:126: if (item &&
item.detail.payload.index !== undefined) {
On 2011/08/09 21:00:50, James Hawkins wrote:
> No braces for single line logic blocks.

Done.

[flackr, 2011-08-10 15:18:46]

In chromeos builds, using the certificate viewer dialog as the parent for the
export dialog fails at locating the browser for that window, so I'm passing the
parent dialog through to the export handler now.

[Rick Byers, 2011-08-10 15:41:24]

Definitely seems unfortunate that you have to do this, but I don't know enough
about this... You should definitely investigate if/how HTML dialogs are
different from popup windows in this respect and consider making them behave
more like popup windows to resolve such issues.

Anyway I'm OK with just leaving a TODO for now and doing that in a separate CL.

[James Hawkins, 2011-08-12 19:58:26]

LGTM with nits.

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:24:
chrome.send('RequestCertificateInfo');
s/Request/request/

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:49: for (var i = 0; i <
nodes.length; i++) {
Remove braces.

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:160:
chrome.send('ExportCertificate', [item.detail.payload.index]);
s/Export/export/

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/ui/webui/cer...
File chrome/browser/ui/webui/certificate_viewer.h (right):

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/ui/webui/cer...
chrome/browser/ui/webui/certificate_viewer.h:65: explicit
CertificateViewerDialogHandler(gfx::NativeWindow parent,
Remove explicit.

[flackr, 2011-08-15 13:47:37]

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:24:
chrome.send('RequestCertificateInfo');
On 2011/08/12 19:58:26, James Hawkins wrote:
> s/Request/request/

Done.

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:49: for (var i = 0; i <
nodes.length; i++) {
On 2011/08/12 19:58:26, James Hawkins wrote:
> Remove braces.

Done.

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.js:160:
chrome.send('ExportCertificate', [item.detail.payload.index]);
On 2011/08/12 19:58:26, James Hawkins wrote:
> s/Export/export/

Done.

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/ui/webui/cer...
File chrome/browser/ui/webui/certificate_viewer.h (right):

http://codereview.chromium.org/7528027/diff/10014/chrome/browser/ui/webui/cer...
chrome/browser/ui/webui/certificate_viewer.h:65: explicit
CertificateViewerDialogHandler(gfx::NativeWindow parent,
On 2011/08/12 19:58:26, James Hawkins wrote:
> Remove explicit.

Done.

[commit-bot: I haz the power, 2011-08-16 16:18:22]

Change committed as 96955