Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(408)

Issue 9562044: Added connection history (minus the history data) (Closed)

Created:
8 years, 9 months ago by Jamie
Modified:
8 years, 9 months ago
Reviewers:
garykac
CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

Added connection history (minus the history data) Since this functionality is not yet available, I've removed the link to it on the home page, but I wanted to get it ready for when we have connection history data so that I can trim the CSS and get the strings translated. BUG=115350 TEST=Manual Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124556

Patch Set 1 #

Total comments: 6

Patch Set 2 : Don't instantiate the connection history dialog. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+397 lines, -19 lines) Patch
M remoting/remoting.gyp View 2 chunks +4 lines, -2 lines 0 comments Download
A remoting/resources/disclosure_arrow_down.png View 0 chunks +-1 lines, --1 lines 0 comments Download
A remoting/resources/disclosure_arrow_right.png View 0 chunks +-1 lines, --1 lines 0 comments Download
M remoting/webapp/_locales/en/messages.json View 7 chunks +40 lines, -0 lines 0 comments Download
A remoting/webapp/connection_history.css View 1 chunk +93 lines, -0 lines 0 comments Download
A remoting/webapp/connection_history.js View 1 chunk +208 lines, -0 lines 0 comments Download
M remoting/webapp/main.css View 3 chunks +1 line, -13 lines 0 comments Download
M remoting/webapp/main.html View 7 chunks +51 lines, -5 lines 0 comments Download
M remoting/webapp/ui_mode.js View 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 3 (0 generated)
Jamie
ptal
8 years, 9 months ago (2012-03-02 00:36:46 UTC) #1
garykac
LGTM http://codereview.chromium.org/9562044/diff/1/remoting/webapp/_locales/en/messages.json File remoting/webapp/_locales/en/messages.json (right): http://codereview.chromium.org/9562044/diff/1/remoting/webapp/_locales/en/messages.json#newcode116 remoting/webapp/_locales/en/messages.json:116: "description": "Column header in the connection history table ...
8 years, 9 months ago (2012-03-02 00:53:56 UTC) #2
Jamie
8 years, 9 months ago (2012-03-02 01:11:05 UTC) #3
http://codereview.chromium.org/9562044/diff/1/remoting/webapp/_locales/en/mes...
File remoting/webapp/_locales/en/messages.json (right):

http://codereview.chromium.org/9562044/diff/1/remoting/webapp/_locales/en/mes...
remoting/webapp/_locales/en/messages.json:116: "description": "Column header in
the connection history table showing the length of time for which a connection
was active, if available."
On 2012/03/02 00:53:56, garykac wrote:
> Do we have a translatable string that we show when this info is not available?

I did have one in an earlier revision, but I decided against singling out this
particular piece of information as being optional. If it turns out we can't get
this data in some cases, we can always fall back on display -- or something
similar (or just add a string if we can take afford the translation delay).

http://codereview.chromium.org/9562044/diff/1/remoting/webapp/connection_hist...
File remoting/webapp/connection_history.js (right):

http://codereview.chromium.org/9562044/diff/1/remoting/webapp/connection_hist...
remoting/webapp/connection_history.js:119: // space between them.
On 2012/03/02 00:53:56, garykac wrote:
> For the 2-column case, shouldn't the space between them be controlled by the
> <td> padding and margin?

<td> has no margin IIRC. I tried your suggestion of setting the widths of these
columns to 1px, which works as far as ensuring that the gap between them is
controllable, but you end up with too narrow a gap between the second and third
columns. A solution involving flexboxes in the first column is probably feasible
but doesn't seem worth it at this stage.

http://codereview.chromium.org/9562044/diff/1/remoting/webapp/remoting.js
File remoting/webapp/remoting.js (right):

http://codereview.chromium.org/9562044/diff/1/remoting/webapp/remoting.js#new...
remoting/webapp/remoting.js:45: remoting.connectionHistory = new
remoting.ConnectionHistory();
On 2012/03/02 00:53:56, garykac wrote:
> Comment out? Or do you want this here to make sure it inits correctly.

Done.

Powered by Google App Engine
This is Rietveld 408576698