Add basic debug log to host page.

Add basic debug log to host page.

BUG=none
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88409

[Jamie, 2011-06-07 18:58:31]

LGTM with nits.

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/debug...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/debug...
remoting/webapp/me2mom/debug_log.js:13: if (!debugLog.style.display ||
debugLog.style.display == 'none') {
I've used class='hidden' elsewhere to avoid changing the style directly. I'm not
really sure which is better TBH, but if you want to be consistent, we should do
the same in both places. Adopting the class-based approach will entail moving
the add/removeClass functions into the remoting namespace so that they're
accessible here,

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/debug...
remoting/webapp/me2mom/debug_log.js:15: toggleButton.value = 'Hide Debug Log';
Is this the only place we have display strings in both the HTML and the JS? I
know we haven't really decided on a localisation strategy yet, but I think I'd
be happier if all our user-facing strings were in the HTML. Perhaps just change
the button to say "Toggle debug log" and don't both changing the string.

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/remot...
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting.js:214: window.alert('Unknown state -> ' +
state);
It seems redundant to do both of these.

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting.js:219: addToDebugLog('Canceling share');
pico-nit: Add an elipsis to this (and the next) log to match the other two
action notifications?

[garykac, 2011-06-07 20:44:32]

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/debug...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/debug...
remoting/webapp/me2mom/debug_log.js:13: if (!debugLog.style.display ||
debugLog.style.display == 'none') {
On 2011/06/07 18:58:32, Jamie wrote:
> I've used class='hidden' elsewhere to avoid changing the style directly. I'm
not
> really sure which is better TBH, but if you want to be consistent, we should
do
> the same in both places. Adopting the class-based approach will entail moving
> the add/removeClass functions into the remoting namespace so that they're
> accessible here,

Good point. However, it doesn't seem worth entangling this code with the
add/remove class code -- especially since it's debug code.

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/debug...
remoting/webapp/me2mom/debug_log.js:15: toggleButton.value = 'Hide Debug Log';
On 2011/06/07 18:58:32, Jamie wrote:
> Is this the only place we have display strings in both the HTML and the JS? I
> know we haven't really decided on a localisation strategy yet, but I think I'd
> be happier if all our user-facing strings were in the HTML. Perhaps just
change
> the button to say "Toggle debug log" and don't both changing the string.

Changed to simply "Debug log" so that the button isn't too large.

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/remot...
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting.js:214: window.alert('Unknown state -> ' +
state);
On 2011/06/07 18:58:32, Jamie wrote:
> It seems redundant to do both of these.

Done.

http://codereview.chromium.org/7108007/diff/2008/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting.js:219: addToDebugLog('Canceling share');
On 2011/06/07 18:58:32, Jamie wrote:
> pico-nit: Add an elipsis to this (and the next) log to match the other two
> action notifications?

Done.

[Jamie, 2011-06-07 20:47:07]

LGTM