Issue 8775066: The webapp uses localStorage to decide whether logging to the server

Issue 8775066: The webapp uses localStorage to decide whether logging to the server (Closed)

Created:
9 years ago by simonmorris

Modified:
9 years ago

Reviewers:
Jamie

CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, sergeyu+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

The webapp uses localStorage to decide whether logging to the server is enabled, and for a pseudonymous ID to attach to logged events. BUG=106208 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112811

Patch Set 1 #

Total comments: 4

Created: 9 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+90 lines, -4 lines)			Patch
M	remoting/webapp/me2mom/debug_log.js	View	3 chunks	+6 lines, -2 lines	2 comments	Download
M	remoting/webapp/me2mom/log_to_server.js	View	4 chunks	+72 lines, -2 lines	2 comments	Download
M	remoting/webapp/me2mom/server_log_entry.js	View	2 chunks	+12 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Jamie

LGTM, with a couple of suggestions. http://codereview.chromium.org/8775066/diff/1/remoting/webapp/me2mom/debug_log.js File remoting/webapp/me2mom/debug_log.js (right): http://codereview.chromium.org/8775066/diff/1/remoting/webapp/me2mom/debug_log.js#newcode690 remoting/webapp/me2mom/debug_log.js:690: this.logIndent(1, 'id: ' ...

9 years ago (2011-12-02 20:44:20 UTC) #2

simonmorris

9 years ago (2011-12-02 21:05:58 UTC) #3

FYI

http://codereview.chromium.org/8775066/diff/1/remoting/webapp/me2mom/debug_lo...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8775066/diff/1/remoting/webapp/me2mom/debug_lo...
remoting/webapp/me2mom/debug_log.js:690: this.logIndent(1, 'id: ' +
remoting_id);
On 2011/12/02 20:44:20, Jamie wrote:
> Do we want to treat id differently to all the other parameters?

I think most of the other parameters will also be shown
on separate lines. The call to logIndent immediately above
this is a special case, because the fields printed there
belong together.

http://codereview.chromium.org/8775066/diff/1/remoting/webapp/me2mom/log_to_s...
File remoting/webapp/me2mom/log_to_server.js (right):

http://codereview.chromium.org/8775066/diff/1/remoting/webapp/me2mom/log_to_s...
remoting/webapp/me2mom/log_to_server.js:131:
idArray.push(this.ID_ALPHABET_.slice(index, index + 1));
On 2011/12/02 20:44:20, Jamie wrote:
> Why not just:
> 
>   idArray.push(this.ID_ALPHABET_[index]);
> 
> Or even (shorter, but less clear IMO):
> 
>   idArray[i] = this.ID_ALPHABET_[index];

I did it this way only because join() joins strings, not
characters, and I think slice() will use a reference to
the alphabet string. So this way avoids creating lots of
small strings.

Expand Messages | Collapse Messages