Include the session id in feedback logs.

Include the session id in feedback logs.

BUG=b/19776978

Committed: https://crrev.com/2d5dba6c3020b037973da36a1808ffae361805f1
Cr-Commit-Position: refs/heads/master@{#330007}

[Jamie, 2015-05-13 20:40:07]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2015-05-13 20:40:08]

PTAL.

This involves plumbing the ClientSession through to the ApplicationContextMenu.
If you can think of a cleaner way, I'm open to suggestions.

[kelvinp, 2015-05-13 22:36:53]

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/app_connected_view.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/app_connected_view.js:39: connectionInfo,
clientSession can be accessed through connectionInfo.session()

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/application_context_menu.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/application_context_menu.js:23:
remoting.ApplicationContextMenu = function(adapter, plugin, clientSession) {
what about just passing the sessionId?

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/base/js/cli...
File remoting/webapp/base/js/client_session.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/base/js/cli...
remoting/webapp/base/js/client_session.js:336: return
this.logToServer_.getSessionId();
use this.sessionId_ instead.

[Jamie, 2015-05-13 23:41:24]

ptal

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/app_connected_view.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/app_connected_view.js:39: connectionInfo,
On 2015/05/13 22:36:53, kelvinp wrote:
> clientSession can be accessed through connectionInfo.session()

Done.

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/application_context_menu.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/application_context_menu.js:23:
remoting.ApplicationContextMenu = function(adapter, plugin, clientSession) {
On 2015/05/13 22:36:53, kelvinp wrote:
> what about just passing the sessionId?

That would work in the majority of cases, but since the session id is reset
every 24hrs, it's not perfect, and I don't think it simplifies the code very
much either.

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/base/js/cli...
File remoting/webapp/base/js/client_session.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/base/js/cli...
remoting/webapp/base/js/client_session.js:336: return
this.logToServer_.getSessionId();
On 2015/05/13 22:36:53, kelvinp wrote:
> use this.sessionId_ instead.

I hadn't realized there was already a sessionId_ member. Unfortunately, it's not
the one we want. I've renamed the accessor to be more descriptive.

[kelvinp, 2015-05-14 01:18:05]

lgtm once my comments are addressed.

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
File remoting/webapp/app_remoting/js/application_context_menu.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/app_remotin...
remoting/webapp/app_remoting/js/application_context_menu.js:23:
remoting.ApplicationContextMenu = function(adapter, plugin, clientSession) {
On 2015/05/13 23:41:24, Jamie wrote:
> On 2015/05/13 22:36:53, kelvinp wrote:
> > what about just passing the sessionId?
> 
> That would work in the majority of cases, but since the session id is reset
> every 24hrs, it's not perfect, and I don't think it simplifies the code very
> much either.

Acknowledged.

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/base/js/cli...
File remoting/webapp/base/js/client_session.js (right):

https://codereview.chromium.org/1131853004/diff/1/remoting/webapp/base/js/cli...
remoting/webapp/base/js/client_session.js:336: return
this.logToServer_.getSessionId();
On 2015/05/13 23:41:24, Jamie wrote:
> On 2015/05/13 22:36:53, kelvinp wrote:
> > use this.sessionId_ instead.
> 
> I hadn't realized there was already a sessionId_ member. Unfortunately, it's
not
> the one we want. I've renamed the accessor to be more descriptive.

On I see, in this, case, you can access the logger from the clientSession via
logger() as well.

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/application_context_menu.js (right):

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/application_context_menu.js:103: sessionId:
that.clientSession_.getLogSessionId()
that.clientSession_.logger().getSessionId();

[Jamie, 2015-05-14 01:38:07]

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/application_context_menu.js (right):

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/application_context_menu.js:103: sessionId:
that.clientSession_.getLogSessionId()
On 2015/05/14 01:18:05, kelvinp wrote:
> that.clientSession_.logger().getSessionId();

ClientSession does not have a logger() method. Are you saying you think that
would be a cleaner design than adding an explicit accessor for the log id?

[kelvinp, 2015-05-14 01:44:45]

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/application_context_menu.js (right):

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/application_context_menu.js:103: sessionId:
that.clientSession_.getLogSessionId()
On 2015/05/14 01:38:07, Jamie wrote:
> On 2015/05/14 01:18:05, kelvinp wrote:
> > that.clientSession_.logger().getSessionId();
> 
> ClientSession does not have a logger() method. Are you saying you think that
> would be a cleaner design than adding an explicit accessor for the log id?

My bad, s/logger()/getLogger()

[Jamie, 2015-05-14 16:39:40]

PTAL

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
File remoting/webapp/app_remoting/js/application_context_menu.js (right):

https://codereview.chromium.org/1131853004/diff/20001/remoting/webapp/app_rem...
remoting/webapp/app_remoting/js/application_context_menu.js:103: sessionId:
that.clientSession_.getLogSessionId()
On 2015/05/14 01:44:45, kelvinp wrote:
> On 2015/05/14 01:38:07, Jamie wrote:
> > On 2015/05/14 01:18:05, kelvinp wrote:
> > > that.clientSession_.logger().getSessionId();
> > 
> > ClientSession does not have a logger() method. Are you saying you think that
> > would be a cleaner design than adding an explicit accessor for the log id?
> 
> My bad, s/logger()/getLogger()

My checkout wasn't up-to-date. Done.

[kelvinp, 2015-05-14 21:54:58]

Looks good.  Just some white spaces nit.

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/client_session.js (right):

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/client_session.js:606: };
Unintended removal of blank line

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/log_to_server.js (right):

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/log_to_server.js:120: 
Remove this blank line.

[Jamie, 2015-05-15 00:26:23]

FYI

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/client_session.js (right):

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/client_session.js:606: };
On 2015/05/14 21:54:57, kelvinp wrote:
> Unintended removal of blank line

It was unintended (I think Emacs cleans up this sort of thing automatically),
but there's no reason to have a blank line at the end of the file.

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
File remoting/webapp/base/js/log_to_server.js (right):

https://codereview.chromium.org/1131853004/diff/40001/remoting/webapp/base/js...
remoting/webapp/base/js/log_to_server.js:120: 
On 2015/05/14 21:54:58, kelvinp wrote:
> Remove this blank line.

Done.

[Jamie, 2015-05-15 00:26:27]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2015-05-15 00:26:28]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1131853004/#ps60001
(title: "Removed blank line.")

[commit-bot: I haz the power, 2015-05-15 00:28:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131853004/60001

[commit-bot: I haz the power, 2015-05-15 01:21:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-15 01:21:49]

Patchset 4 (id:??) landed as
https://crrev.com/2d5dba6c3020b037973da36a1808ffae361805f1
Cr-Commit-Position: refs/heads/master@{#330007}