The user's consent to crash dumps reporting can now be set via the UI (Windows only). The checkbox …

The user's consent to crash dumps reporting can now be set via the UI (Windows only). The checkbox is presented on the Start/Change PIN dialog. The user's selection is then written to usagestats under ClientStateMedium key.

BUG=130678
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=143504

[alexeypa (please no reviews), 2012-06-14 21:49:58]

PTAL.

[Jamie, 2012-06-14 23:43:56]

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/breakpad...
File remoting/host/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:29: bool GetUsageStatsConsent(bool*
set_by_policy_opt, bool* allowed) {
Is the _opt suffix intended to signify that the parameter is optional? If so,
then maybe it should be a prefix as is used in Javascript. Also, I think it's
more usual to make the "primary" return value the first parameter.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:59: *set_by_policy_opt = false;
You're setting |set_by_policy_opt| to false in both cases. Is that correct?

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller.h:137: const
GetUsageStatsConsentCallback& callback) = 0;
There's no provision here for returning "unsupported", which we could use to
remove the check-box on platforms that don't support usage stats.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:332:
control_.QueryInterface(IID_IDaemonControl2, control2_.ReceiveVoid());
Does this result in an elevated IDaemonControl2 instance? That seems bad if
we're saying that changing this setting should not require admin.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:632: callback.Run(false, false);
There should be a mechanism for reporting "not implemented" and we should use it
here.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:654: return;
It would be better to report failure to the caller, and propagate it to the
web-app, even if we just log an error for now.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.cc:775:
SetException("GetUsageStatsConsent: bad number of arguments");
Nit: camelCase for the message (ie, the name as visible to Javascript, not the
function name).

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.cc:781:
SetException("GetUsageStatsConsent: invalid callback parameter");
And here.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.h:167: uint32_t arg_count,
Nit: Indentation.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.h:173: uint32_t arg_count,
Nit: Indentation.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_c...
File remoting/webapp/host_controller.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_c...
remoting/webapp/host_controller.js:70: * Callback to be called when done.
Nit: Indent the description four spaces, and start it on the previous line if
possible.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_c...
remoting/webapp/host_controller.js:78: * @param {boolean} Whether the user
allows crash dump reporting.
You're missing an |allowed| in this JSDoc comment. Please run JSCompiler over
this CL before committing (ask me for instructions).

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:143: * @param {boolean} set_by_policy True
if crash dump reporting is allowed by
s/allowed/controlled/ (it might be disabled by policy)

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:144: *     the policy.
s/the//

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:151:
this.hostController_.getConsent(onGetConsent);
This belongs in showForStart. I suspect it won't make any real difference in
practise, so we can cheat here if the asynchronous nature of getConsent is going
to make life difficult, but dialog initialization should take place in the show
method, not the constructor.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:159: };
I think this would be better handled by deferring setting the policy until the
host is started, rather than trying to do it as soon as the checkbox changes. In
other words, add the logic to the onsubmit handler.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:168:
remoting.HostSetupDialog.prototype.showForStart = function() {
showForStart should show the check-box if crash reporting is enabled.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:188:
remoting.HostSetupDialog.prototype.showForPin = function() {
showForPin should hide the check-box. I don't think it makes sense to offer to
change this setting via the change-PIN dialog.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.css
File remoting/webapp/main.css (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.c...
remoting/webapp/main.css:468: #ask-pin-form label {
I think it would be better to make this rule more specific, rather than
overriding it below. Something like "#ask-pin-form table label" ought to do the
trick.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.c...
remoting/webapp/main.css:483: vertical-align: middle;
Is this what causes the checkbox to be vertically-centered wrt the label? I
think it would look better top-aligned, but UX might have other ideas.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.h...
remoting/webapp/main.html:224: <table>
Is a second table needed for the check-box? We shouldn't use tables for
non-tabular data (I wasn't happy about using one to lay out the PIN entry and
confirmation, but couldn't see any alternative).

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.h...
remoting/webapp/main.html:231: <span
i18n-content="HOST_SETUP_CRASH_REPORTING_MESSAGE"></span>
Nit: Split at the close of the <span> tag to keep below 80-characters.

[alexeypa (please no reviews), 2012-06-19 23:27:29]

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/breakpad...
File remoting/host/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:29: bool GetUsageStatsConsent(bool*
set_by_policy_opt, bool* allowed) {
On 2012/06/14 23:43:56, Jamie wrote:
> Is the _opt suffix intended to signify that the parameter is optional? If so,
> then maybe it should be a prefix as is used in Javascript. Also, I think it's
> more usual to make the "primary" return value the first parameter.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:59: *set_by_policy_opt = false;
On 2012/06/14 23:43:56, Jamie wrote:
> You're setting |set_by_policy_opt| to false in both cases. Is that correct?

Yes. set_by_policy is not really used in this CL. It will be used when we start
checking policy to enable crash dump reporting just like Chrome does. I added it
now because it should be present in the COM interface and I'd like to avoid
changing the COM interface twice.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller.h:137: const
GetUsageStatsConsentCallback& callback) = 0;
On 2012/06/14 23:43:56, Jamie wrote:
> There's no provision here for returning "unsupported", which we could use to
> remove the check-box on platforms that don't support usage stats.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:332:
control_.QueryInterface(IID_IDaemonControl2, control2_.ReceiveVoid());
On 2012/06/14 23:43:56, Jamie wrote:
> Does this result in an elevated IDaemonControl2 instance?

Yes, but the UAC prompt has been shown to the user. So it is legit. :-)

> That seems bad if we're saying that changing this setting should not require
admin.

The way the code work is:

- The COM controller will run under identity of the caller unless elevation
happened.
- The UAC prompt is shown if elevation is requested. If the user agreed to if we
can do admin tasks until the timeout expires. Currently 15 minutes.
- Omaha sets permissions on ClientStateMeduim key where we store the flag so
that interactive users can change values. It means that any user can enable and
disable crash reporting. It is assumed that it is safe enough.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:632: callback.Run(false, false);
On 2012/06/14 23:43:56, Jamie wrote:
> There should be a mechanism for reporting "not implemented" and we should use
it
> here.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:654: return;
On 2012/06/14 23:43:56, Jamie wrote:
> It would be better to report failure to the caller, and propagate it to the
> web-app, even if we just log an error for now.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.cc:775:
SetException("GetUsageStatsConsent: bad number of arguments");
On 2012/06/14 23:43:56, Jamie wrote:
> Nit: camelCase for the message (ie, the name as visible to Javascript, not the
> function name).

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.cc:781:
SetException("GetUsageStatsConsent: invalid callback parameter");
On 2012/06/14 23:43:56, Jamie wrote:
> And here.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
File remoting/host/plugin/host_script_object.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.h:167: uint32_t arg_count,
On 2012/06/14 23:43:56, Jamie wrote:
> Nit: Indentation.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/h...
remoting/host/plugin/host_script_object.h:173: uint32_t arg_count,
On 2012/06/14 23:43:56, Jamie wrote:
> Nit: Indentation.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_c...
File remoting/webapp/host_controller.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_c...
remoting/webapp/host_controller.js:70: * Callback to be called when done.
On 2012/06/14 23:43:56, Jamie wrote:
> Nit: Indent the description four spaces, and start it on the previous line if
> possible.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_c...
remoting/webapp/host_controller.js:78: * @param {boolean} Whether the user
allows crash dump reporting.
On 2012/06/14 23:43:56, Jamie wrote:
> You're missing an |allowed| in this JSDoc comment. Please run JSCompiler over
> this CL before committing (ask me for instructions).

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:143: * @param {boolean} set_by_policy True
if crash dump reporting is allowed by
On 2012/06/14 23:43:56, Jamie wrote:
> s/allowed/controlled/ (it might be disabled by policy)

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:144: *     the policy.
On 2012/06/14 23:43:56, Jamie wrote:
> s/the//

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:151:
this.hostController_.getConsent(onGetConsent);
On 2012/06/14 23:43:56, Jamie wrote:
> This belongs in showForStart. I suspect it won't make any real difference in
> practise, so we can cheat here if the asynchronous nature of getConsent is
going
> to make life difficult, but dialog initialization should take place in the
show
> method, not the constructor.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:159: };
On 2012/06/14 23:43:56, Jamie wrote:
> I think this would be better handled by deferring setting the policy until the
> host is started, rather than trying to do it as soon as the checkbox changes.
In
> other words, add the logic to the onsubmit handler.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:168:
remoting.HostSetupDialog.prototype.showForStart = function() {
On 2012/06/14 23:43:56, Jamie wrote:
> showForStart should show the check-box if crash reporting is enabled.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/host_s...
remoting/webapp/host_setup_dialog.js:188:
remoting.HostSetupDialog.prototype.showForPin = function() {
On 2012/06/14 23:43:56, Jamie wrote:
> showForPin should hide the check-box. I don't think it makes sense to offer to
> change this setting via the change-PIN dialog.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.css
File remoting/webapp/main.css (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.c...
remoting/webapp/main.css:468: #ask-pin-form label {
On 2012/06/14 23:43:56, Jamie wrote:
> I think it would be better to make this rule more specific, rather than
> overriding it below. Something like "#ask-pin-form table label" ought to do
the
> trick.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.c...
remoting/webapp/main.css:483: vertical-align: middle;
On 2012/06/14 23:43:56, Jamie wrote:
> Is this what causes the checkbox to be vertically-centered wrt the label? I

No, there is:

td {
  vertical-align: middle;
}

I did it differently this time. Take a look.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.h...
remoting/webapp/main.html:224: <table>
On 2012/06/14 23:43:56, Jamie wrote:
> Is a second table needed for the check-box? We shouldn't use tables for
> non-tabular data (I wasn't happy about using one to lay out the PIN entry and
> confirmation, but couldn't see any alternative).

I couldn't make it work without the table. I tries playing with float but it
didn't work out. I'm open to suggestions.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.h...
remoting/webapp/main.html:231: <span
i18n-content="HOST_SETUP_CRASH_REPORTING_MESSAGE"></span>
On 2012/06/14 23:43:56, Jamie wrote:
> Nit: Split at the close of the <span> tag to keep below 80-characters.

Done.

[Jamie, 2012-06-21 18:47:52]

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:332:
control_.QueryInterface(IID_IDaemonControl2, control2_.ReceiveVoid());
On 2012/06/19 23:27:29, alexeypa wrote:
> On 2012/06/14 23:43:56, Jamie wrote:
> > Does this result in an elevated IDaemonControl2 instance?
> 
> Yes, but the UAC prompt has been shown to the user. So it is legit. :-)
> 
> > That seems bad if we're saying that changing this setting should not require
> admin.
> 
> The way the code work is:
> 
> - The COM controller will run under identity of the caller unless elevation
> happened.
> - The UAC prompt is shown if elevation is requested. If the user agreed to if
we
> can do admin tasks until the timeout expires. Currently 15 minutes.
> - Omaha sets permissions on ClientStateMeduim key where we store the flag so
> that interactive users can change values. It means that any user can enable
and
> disable crash reporting. It is assumed that it is safe enough.

My point is that if the code is designed to be runnable as non-admin, then we
should never run it as admin. Can the code be rewritten to ensure this?

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.h...
remoting/webapp/main.html:224: <table>
On 2012/06/19 23:27:29, alexeypa wrote:
> On 2012/06/14 23:43:56, Jamie wrote:
> > Is a second table needed for the check-box? We shouldn't use tables for
> > non-tabular data (I wasn't happy about using one to lay out the PIN entry
and
> > confirmation, but couldn't see any alternative).
> 
> I couldn't make it work without the table. I tries playing with float but it
> didn't work out. I'm open to suggestions.

I think for this CL we can just take any approach that works. I'll take a look
at cleaning it up as a more focused effort (or you can do it yourself if you
want to brush up on HTML/CSS). Can you raise an M22 bug, assign it to one of us
and add a TODO here and/or to the CSS?

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/host/plug...
File remoting/host/plugin/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/host/plug...
remoting/host/plugin/daemon_controller.h:83: typedef base::Callback<void (bool,
bool, bool)> GetUsageStatsConsentCallback;
I think this typedef could do with documentation to explain what the three bools
represent.

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ho...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ho...
remoting/webapp/host_setup_dialog.js:156: * @param {boolean} supported True if
crash dumps reporting is supported by
Nit: s/dumps/dump/ here and elsewhere.

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ho...
remoting/webapp/host_setup_dialog.js:167: this.usageStats_.hidden = true;
Optional: I'm not sure hiding it and having it appear when the callback
completes is correct. Certainly once all platforms support this it won't be
right, and I think since Windows is our main platform it's probably better just
to disable it here. Having the state transistion from disabled to enabled is
likely to be less jarring that having it appear.

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
File remoting/webapp/main.css (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
remoting/webapp/main.css:479: float: left;
Floats should not be necessary for a simple layout like this, and I've tried to
avoid them where possible because they make the layout harder to reason about.

https://chromiumcodereview.appspot.com/10537182/diff/8004/remoting/webapp/hos...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/8004/remoting/webapp/hos...
remoting/webapp/host_setup_dialog.js:395: (this.usageStatsCheckbox_.value ==
"on");
Does the |checked| property not do what you want, or is it just that jscompiler
doesn't know about it? If it's the latter, then it might be a cross-browser
issue that we don't care about. In such cases, I've used to jscompiler_hacks.js
to tell the compiler that the property exists, rather than change the code to
something less elegant. It's not clear that that's warranted in this case--I'll
leave it to your judgement.

[alexeypa (please no reviews), 2012-06-21 23:30:25]

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:332:
control_.QueryInterface(IID_IDaemonControl2, control2_.ReceiveVoid());
On 2012/06/21 18:47:52, Jamie wrote:
> My point is that if the code is designed to be runnable as non-admin, then we
> should never run it as admin. Can the code be rewritten to ensure this?

The code was designed to be running as either non-admin or admin. non-admin
clients will have less functionality available because they will not be able to
do certain operations. It is all controlled by the ACLs at the end. The calling
code should not make decisions whether an operation will succeed or fail
depending on the its identity.

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.html
File remoting/webapp/main.html (right):

https://chromiumcodereview.appspot.com/10537182/diff/1/remoting/webapp/main.h...
remoting/webapp/main.html:224: <table>
On 2012/06/21 18:47:52, Jamie wrote:
> > I couldn't make it work without the table. I tries playing with float but it
> > didn't work out. I'm open to suggestions.
> I think for this CL we can just take any approach that works. I'll take a look
> at cleaning it up as a more focused effort (or you can do it yourself if you
> want to brush up on HTML/CSS). Can you raise an M22 bug, assign it to one of
us
> and add a TODO here and/or to the CSS?

I actually was able to got rid of table, just forgot to update this comment. It
should've been "Done".

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/host/plug...
File remoting/host/plugin/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/host/plug...
remoting/host/plugin/daemon_controller.h:83: typedef base::Callback<void (bool,
bool, bool)> GetUsageStatsConsentCallback;
On 2012/06/21 18:47:52, Jamie wrote:
> I think this typedef could do with documentation to explain what the three
bools
> represent.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ho...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ho...
remoting/webapp/host_setup_dialog.js:156: * @param {boolean} supported True if
crash dumps reporting is supported by
On 2012/06/21 18:47:52, Jamie wrote:
> Nit: s/dumps/dump/ here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ho...
remoting/webapp/host_setup_dialog.js:167: this.usageStats_.hidden = true;
On 2012/06/21 18:47:52, Jamie wrote:
> Optional: I'm not sure hiding it and having it appear when the callback
> completes is correct. Certainly once all platforms support this it won't be
> right, and I think since Windows is our main platform it's probably better
just
> to disable it here. Having the state transistion from disabled to enabled is
> likely to be less jarring that having it appear.

Done.

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
File remoting/webapp/main.css (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
remoting/webapp/main.css:479: float: left;
On 2012/06/21 18:47:52, Jamie wrote:
> Floats should not be necessary for a simple layout like this, and I've tried
to
> avoid them where possible because they make the layout harder to reason about.

I'll file an M22 bug on that.

https://chromiumcodereview.appspot.com/10537182/diff/8004/remoting/webapp/hos...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/8004/remoting/webapp/hos...
remoting/webapp/host_setup_dialog.js:395: (this.usageStatsCheckbox_.value ==
"on");
On 2012/06/21 18:47:53, Jamie wrote:
> Does the |checked| property not do what you want, or is it just that
jscompiler
> doesn't know about it? If it's the latter, then it might be a cross-browser
> issue that we don't care about. In such cases, I've used to
jscompiler_hacks.js
> to tell the compiler that the property exists, rather than change the code to
> something less elegant. It's not clear that that's warranted in this
case--I'll
> leave it to your judgement.

I've played with jscompiler_hacks.js and couldn't find a way to add the property
to HTMLInputElement type I need. At least jscompiler keeps complaining. Since
'value == "on"' and checked are equivalent I think I'll stick to the one that
works and jscompiler does not complain about.

[Jamie, 2012-06-21 23:49:31]

LGTM with a few final tweaks.

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
File remoting/webapp/main.css (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
remoting/webapp/main.css:479: float: left;
On 2012/06/21 23:30:26, alexeypa wrote:
> On 2012/06/21 18:47:52, Jamie wrote:
> > Floats should not be necessary for a simple layout like this, and I've tried
> to
> > avoid them where possible because they make the layout harder to reason
about.
> 
> I'll file an M22 bug on that.
> 

Could you add a TODO as well please?

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/host/usag...
File remoting/host/usage_stats_consent.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/host/usag...
remoting/host/usage_stats_consent.h:15: // usage statistics. In most cases the
returned value matched |allowed| returned
Nit: s/matched/matches/

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/host/usag...
remoting/host/usage_stats_consent.h:16: // by GetUsageStatsConsent(). When
GetUsageStatsConsent() fails this routine
Nit: s/When/If/

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/webapp/ho...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/webapp/ho...
remoting/webapp/host_setup_dialog.js:169: this.usageStatsCheckbox_.checked =
false;
To minimize potentially jarring changes after the dialog is shown, I think this
should be initialized to true. That will be the default, and I suspect most
users won't change it, so I think it makes more sense.

[alexeypa (please no reviews), 2012-06-22 00:09:27]

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
File remoting/webapp/main.css (right):

https://chromiumcodereview.appspot.com/10537182/diff/11001/remoting/webapp/ma...
remoting/webapp/main.css:479: float: left;
On 2012/06/21 23:49:31, Jamie wrote:
> On 2012/06/21 23:30:26, alexeypa wrote:
> > On 2012/06/21 18:47:52, Jamie wrote:
> > > Floats should not be necessary for a simple layout like this, and I've
tried
> > to
> > > avoid them where possible because they make the layout harder to reason
> about.
> > 
> > I'll file an M22 bug on that.
> > 
> 
> Could you add a TODO as well please?

Done. http://crbug.com/134063

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/host/usag...
File remoting/host/usage_stats_consent.h (right):

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/host/usag...
remoting/host/usage_stats_consent.h:15: // usage statistics. In most cases the
returned value matched |allowed| returned
On 2012/06/21 23:49:31, Jamie wrote:
> Nit: s/matched/matches/

Done.

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/host/usag...
remoting/host/usage_stats_consent.h:16: // by GetUsageStatsConsent(). When
GetUsageStatsConsent() fails this routine
On 2012/06/21 23:49:31, Jamie wrote:
> Nit: s/When/If/

Done.

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/webapp/ho...
File remoting/webapp/host_setup_dialog.js (right):

https://chromiumcodereview.appspot.com/10537182/diff/35001/remoting/webapp/ho...
remoting/webapp/host_setup_dialog.js:169: this.usageStatsCheckbox_.checked =
false;
On 2012/06/21 23:49:31, Jamie wrote:
> To minimize potentially jarring changes after the dialog is shown, I think
this
> should be initialized to true. That will be the default, and I suspect most
> users won't change it, so I think it makes more sense.

Done.