Initial landing of Screen, Terminal, and VT100 classes.

Initial landing of Screen, Terminal, and VT100 classes.

Some of this code is based on Cory Maccarrone's html terminal, developed
internally in Google as part of a different project.  Thanks to Cory for
allowing us to repurpose it here!

This gets us to a point where we have the core classes largely fleshed out.  The code passes 45 tests, mostly related to correct handling of vt100 escape sequences.

Still to come: network connectivity, keyboard input, and text attributes!

BUG=chromium-os:23271
TEST=test_harness.html:45/45 tests passed.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113066

[rginda, 2011-11-24 00:00:31]

Sorry about the hugeness of this CL.  The only way for me to feel comfortable
enough with the layout of the code was to piece all this together and see how it
fit.

Here's a breakdown of what's in the diff, in the order it appears:

test_harness.html:
* Added comments.
* Added <script> tags for the new files.

hterm.js: New file
* Defines the hterm.* namespace and some utilities.

mock_row_provider.js:
* Construct 'x-row's instead of plain 'div's.

options.js: New file
* Nearly verbatim copy from Cory's code.

screen.js: New file
* Represents a screen of addressable rows.
* Maintains the screen's cursor position, both as 2D coordinates, and pointers
  into the insertion point in the DOM.
* Basic overwrite/insert and cursor positioning commands.
* No knowledge of scrollback rows.

screen_tests.js: New file
* Unit tests for screen.js.

scrollport.js:
* Moved the scrollport into the hterm namespace.  Strictly speaking it's not
  really an hterm specific implementation, but I'd prefer to have it in some
  namespace rather than none at all, and I don't want to declare some sort of
  shared namespace right now.
* Made it a requirement that all rows use the 'x-row' tag.  I had previously
  depended on the CSS line-height of the 'x-screen' container to set the
  height of each row, but this doesn't work for rows without text.  Now we
  require 'x-row's, and we inject a CSS rule to ensure they all have the
  required height.
* Added the ability to get/set the default fore/background colors.
* Added invalidateRowRange, which causes rows within the range to be
  re-inserted in the document, without requiring a full-on redraw.
* Added syncScrollHeight(), which adjusts the scrollbar size when the number
  of rows changes, without requiring a full-on redraw.

scrollport_tests.js:
* Moved the scrollport into the hterm namespace.

terminal.js: New file
* Somewhat based on Cory's framebuffer class, with the same or similar method
  names in most cases.  Some implementations are very similar as well.
* Ties together the VT100, Screen and ScrollPort classes into a coherent
  terminal.

terminal_tests.js: New file
* Unit tests for terminal.js.

test_manager.js:
* Added postamble() method to the test suite to deal with teardown of things
  done in the preamble().
* Made opt_pattern default to ALL_TESTS in TestRun..selectSuite.
* Fix use of undefined 'testName' variable in selectSuite.
* Fix the return value of onUncaughtException_ so that it mutes uncaight test
  pass/fail exceptions.

vt100.js: New file
* Mostly copy/paste from Cory's vt100_interpreter.js.
* Moved into the hterm.* namespace.
* s/framebuffer/terminal.
* Added the ability to send spans of plain text to the terminal in a single
  call.
* Replaced String.fromCharCode/toCharCode with string constants.
* Store the pendingSequence in an array rather than a string.

vt100_tests.js: New file
* Unit (or arguably integration) tests for vt100.js.

[klimek, 2011-11-24 19:13:51]

FYI

Just a few comments from the peanut gallery.
I like the general architecture...

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:349: str.length - this.columnCount_,
Indent.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/terminal.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:11: * There are a number of
lower-level Terminal that can be directly called
Sentence doesn't parse.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:146:
this.scrollbackRows_[index].rowIndex = index;
This is unexpected (and seems a breach in the architecture at a first glance).
Why does a row need to know its index?

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/vt100.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:99: this.pendingSequence_.length)) {
Can't you save this whole 'interpret' variable by just inverting the if here and
pulling up the block below?

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:122: if (sequence.length == 0 ||
== 0 cannot happen after a push; or am I missing something fundamental?

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:123: sequence[0] != '\x1b' ||
Also, I don't see how this code can every push something other than \x1b at the
start of the sequence...

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:124: sequence.length < 2) {
... so this is basically if (sequence.length == 1) { ... }

[Vladislav Kaznacheev, 2011-11-28 14:00:07]

Just a few comments. I am still going though the new file.

  Vlad

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/scrollport.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/scrollport.js:272:
hterm.ScrollPort.prototype.getScreenHeight = function() {
Did I miss that in the original land CL? Shame on me:)

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/scrollport.js:581: //console.log('bottom
fold');
Debugging code left

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/scrollport.js:712: return
this.rowProvider_.getRowNode(rowIndex);
Dead code below, debugging leftovers?

[arv (Not doing code reviews), 2011-11-28 18:21:00]

Fun stuff.

I just could not resist looking at this :-) I tried not to comment too much.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:200: this.rowsArray.splice(index +
i, 0, rows[i]);
Array.prototype.splice.apply(this.rowsArray, index, 0, rows);

is O(n) instead of O(n * m)

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen_tests.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen_tests.js:26: // Push one at a time.
indentation is off in this file

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/vt100.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:282: this.metaSendsEscape_ = enable;
All these noop getters and setters are pretty silly. Why don't you just use data
properties and real getters and setters for the few cases where side effects are
wanted?

[Vladislav Kaznacheev, 2011-11-28 18:39:36]

I went through everything but the two VT100* files (which dgozman agreed to
review) and I liked what I saw. Just is a handful of minor comments.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/terminal.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:239: //console.log('ssd');
debugging code

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:695: this.redrawTimeout_ = null;
this -> self?

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:743: var minHeight =
(this.options_.originMode ? this.getVTScrollTop() : 0);
I am a bit concerned that cursorDown and cursorUp implementations only differ in
1 character. Can we generalize? Same with cursorLeft/cursorRight.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/terminal_tests.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal_tests.js:51: * test case, we use this
overridden addText method to add a proxy around the
addText -> addTest

[rginda, 2011-11-28 20:39:47]

Thanks everyone for the reviews, PTAL.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:200: this.rowsArray.splice(index +
i, 0, rows[i]);
On 2011/11/28 18:21:00, arv wrote:
> Array.prototype.splice.apply(this.rowsArray, index, 0, rows);
> 
> is O(n) instead of O(n * m)

That's the way I had written it at first, but it's not correct.  The apply()
method takes (function, args-array), not (function, ...).

I could unshift 'index' and '0' into the rows array, but that would change the
contract with the caller since I'd be trashing their array.  (Maybe if I shifted
them off after and said a few Hail Mary's that would suffice.)

I could make a new array, but that seemed like a lot of copying.  I didn't
benchmark it though, maybe copying the rows array would be faster.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:349: str.length - this.columnCount_,
On 2011/11/24 19:13:51, klimek wrote:
> Indent.

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen_tests.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen_tests.js:26: // Push one at a time.
On 2011/11/28 18:21:00, arv wrote:
> indentation is off in this file

This function body is actually part of a parameter to the addTest() method, so I
went with the four spaces (well, emacs did, but I figured it was probably
correct).

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/scrollport.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/scrollport.js:581: //console.log('bottom
fold');
On 2011/11/28 14:00:07, Vladislav Kaznacheev wrote:
> Debugging code left

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/scrollport.js:712: return
this.rowProvider_.getRowNode(rowIndex);
On 2011/11/28 14:00:07, Vladislav Kaznacheev wrote:
> Dead code below, debugging leftovers?

Oops.  This early return was me defeating the row cache when it got in my way. 
I've re-enabled it, and modified invalidateRowRange to keep it in sync.  I've
also re-enabled the 'node-recycler' test.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/terminal.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:11: * There are a number of
lower-level Terminal that can be directly called
On 2011/11/24 19:13:51, klimek wrote:
> Sentence doesn't parse.

It does if you read it fast enough :)  Fixed.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:146:
this.scrollbackRows_[index].rowIndex = index;
On 2011/11/24 19:13:51, klimek wrote:
> This is unexpected (and seems a breach in the architecture at a first glance).
> Why does a row need to know its index?

This is the visual representation of a row.  It's the DOM node that the
ScrollPort inserts into the visible document.  It needs to know its index so
that the ScrollPort can reverse lookup the rowIndex for a visible row, to
quickly determine whether or not it's in the right place.

This shows up in a few places in the code (see moveRows_, and appendRows_).

Having said that, this code doesn't need to be setting the rowIndex property. 
It used to, before I got careful about exactly how new rows are created, but now
that appendRows_ is the only correct way to create a new row, and moveRows_ is
the only correct way to move them, this particular rowIndex related code is
obsolete.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:239: //console.log('ssd');
On 2011/11/28 18:39:36, Vladislav Kaznacheev wrote:
> debugging code

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:695: this.redrawTimeout_ = null;
On 2011/11/28 18:39:36, Vladislav Kaznacheev wrote:
> this -> self?

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal.js:743: var minHeight =
(this.options_.originMode ? this.getVTScrollTop() : 0);
On 2011/11/28 18:39:36, Vladislav Kaznacheev wrote:
> I am a bit concerned that cursorDown and cursorUp implementations only differ
in
> 1 character. Can we generalize? Same with cursorLeft/cursorRight.

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/vt100.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:99: this.pendingSequence_.length)) {
On 2011/11/24 19:13:51, klimek wrote:
> Can't you save this whole 'interpret' variable by just inverting the if here
and
> pulling up the block below?

This wonkyness is a result of some changes I made to the original code.  I had
meant to revisit this code but forgot to.  Fixed now.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:122: if (sequence.length == 0 ||
On 2011/11/24 19:13:51, klimek wrote:
> == 0 cannot happen after a push; or am I missing something fundamental?

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:123: sequence[0] != '\x1b' ||
On 2011/11/24 19:13:51, klimek wrote:
> Also, I don't see how this code can every push something other than \x1b at
the
> start of the sequence...

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:124: sequence.length < 2) {
On 2011/11/24 19:13:51, klimek wrote:
> ... so this is basically if (sequence.length == 1) { ... }

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/vt100.js:282: this.metaSendsEscape_ = enable;
On 2011/11/28 18:21:00, arv wrote:
> All these noop getters and setters are pretty silly. Why don't you just use
data
> properties and real getters and setters for the few cases where side effects
are
> wanted?

I avoid real getters and setters like the plague (sorry, I know you're into
them).  I'm not a fan of the fact that they look like a simple get/set, but
actually have side-effects.  I've been bitten in enough in various languages --
by myself and others -- and now I just avoid them everywhere as a matter of
course.  They're also pretty ugly in programmatic class construction.  (Sorry
again, we agree that programmatic get/setters are ugly, but I know you prefer to
declare your objects in literal notation!)

But, I can agree that the accessor methods on VT100 are unnecessary :)  I've
converted them all to public properties.

Promoting (demoting?) the setters related to Terminal.options_ will have much
more fallout.  I'll do that in another CL.

[arv (Not doing code reviews), 2011-11-28 21:33:22]

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:200: this.rowsArray.splice(index +
i, 0, rows[i]);
On 2011/11/28 20:39:47, rginda wrote:
> On 2011/11/28 18:21:00, arv wrote:
> > Array.prototype.splice.apply(this.rowsArray, index, 0, rows);
> > 
> > is O(n) instead of O(n * m)
> 
> That's the way I had written it at first, but it's not correct.  The apply()
> method takes (function, args-array), not (function, ...).

My bad.

You could use bind but at this point I'm not sure it is worth all the extra
trouble.

this.rowsArray.splice.bind(this.rowsArray, index, 0).apply(null, rows)

[dgozman, 2011-11-29 11:35:42]

Unfortunately, I am not familiar with all those control sequences parsed in
VT100. I've just checked the code is logical.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:142:
rows.unshift.apply(this.rowsArray, rows);
Why rows.unshift.apply, and not this.rowsArray.unshift.apply?

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:170: var start =
this.rowsArray.length;
start not used

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:180: var start =
this.rowsArray.length;
start not used

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:200: this.rowsArray.splice(index +
i, 0, rows[i]);
Maybe, splice.apply will be faster? It's just two array operations instead of
rows.length.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:255:
row.appendChild(row.ownerDocument.createTextNode(''))
semicolon

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:313: node = node.nextSibling;
This code assumes that each row may contain only direct child nodes with text,
right?
We can remove this requirement (if needed), using recursive traversal of nodes.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen_tests.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen_tests.js:34:
result.assertEQ(ary.length, this.screen.rowsArray.length);
this.screen.getHeight ?

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen_tests.js:253:
this.screen.setCursorPosition(0, 0);
What about inserting at (0, 1)?

[rginda, 2011-11-29 19:26:12]

> Unfortunately, I am not familiar with all those control 
> sequences parsed in
> VT100. I've just checked the code is logical.

Not a problem.  The specific escape sequences and the bulk of the parsing code
is copied verbatim from Cory's code, which is already in production internally
at G.  It should already be in pretty good shape.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:142:
rows.unshift.apply(this.rowsArray, rows);
On 2011/11/29 11:35:42, dgozman wrote:
> Why rows.unshift.apply, and not this.rowsArray.unshift.apply?

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:170: var start =
this.rowsArray.length;
On 2011/11/29 11:35:42, dgozman wrote:
> start not used

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:180: var start =
this.rowsArray.length;
On 2011/11/29 11:35:42, dgozman wrote:
> start not used

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:200: this.rowsArray.splice(index +
i, 0, rows[i]);
On 2011/11/29 11:35:42, dgozman wrote:
> Maybe, splice.apply will be faster? It's just two array operations instead of
> rows.length.

If I modify the rows array it'll trash something the caller owns, unless I
"unmodify" it after.  That seems dirty though.  I'll revisit this code later if
it turns out to be an actual performance issue.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:255:
row.appendChild(row.ownerDocument.createTextNode(''))
On 2011/11/29 11:35:42, dgozman wrote:
> semicolon

Done.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen.js:313: node = node.nextSibling;
On 2011/11/29 11:35:42, dgozman wrote:
> This code assumes that each row may contain only direct child nodes with text,
> right?
> We can remove this requirement (if needed), using recursive traversal of
nodes.

We're going to want to keep this requirement to simplify the handling of text
attributes, once they're implemented.

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/screen_tests.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/screen_tests.js:253:
this.screen.setCursorPosition(0, 0);
On 2011/11/29 11:35:42, dgozman wrote:
> What about inserting at (0, 1)?

I'm not sure I understand the question.  I test inserting at a non-zero column
above (at line 240, inserting into (0, 3)).  Here I decided to insert at (0, 0)
so that the 'XXXX' string would just overflow.  I could insert at (0, 1) and
test that ' XXXX' overflowed, but I'm not sure why that would be better?

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
File chrome/browser/resources/hterm/js/terminal_tests.js (right):

http://codereview.chromium.org/8680034/diff/1/chrome/browser/resources/hterm/...
chrome/browser/resources/hterm/js/terminal_tests.js:51: * test case, we use this
overridden addText method to add a proxy around the
On 2011/11/28 18:39:36, Vladislav Kaznacheev wrote:
> addText -> addTest

Done.

[mdhayter, 2011-11-30 05:07:47]

Looking out of interest based on your status report. I think I found a typo.

Mark

http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
File chrome/browser/resources/hterm/js/hterm.js (right):

http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
chrome/browser/resources/hterm/js/hterm.js:148: return this.row = that.row &&
this.column == that.column;
this.row == that.row?

[dgozman, 2011-11-30 07:57:01]

LGTM

http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
File chrome/browser/resources/hterm/js/terminal.js (right):

http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
chrome/browser/resources/hterm/js/terminal.js:704: var f =
this.scheduleScrollDown_;
If I'm not mistaken, |f| will be the same object for multiple Terminal
instances. Timeouts may interfere here. Same for other places.

[Vladislav Kaznacheev, 2011-11-30 12:31:20]

lgtm

[rginda, 2011-11-30 22:50:17]

On 2011/11/30 05:07:47, mdhayter wrote:
> Looking out of interest based on your status report. I think I found a typo.
> 
> Mark
> 
>
http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
> File chrome/browser/resources/hterm/js/hterm.js (right):
> 
>
http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
> chrome/browser/resources/hterm/js/hterm.js:148: return this.row = that.row &&
> this.column == that.column;
> this.row == that.row?

Nice catch, thanks.  I had copy/pasted the same typo into hterm.Size.  Fixed in
both places.

[rginda, 2011-11-30 22:51:12]

On 2011/11/30 07:57:01, dgozman wrote:
> LGTM
> 
>
http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
> File chrome/browser/resources/hterm/js/terminal.js (right):
> 
>
http://codereview.chromium.org/8680034/diff/11004/chrome/browser/resources/ht...
> chrome/browser/resources/hterm/js/terminal.js:704: var f =
> this.scheduleScrollDown_;
> If I'm not mistaken, |f| will be the same object for multiple Terminal
> instances. Timeouts may interfere here. Same for other places.

Whoops.  Fixed.

[commit-bot: I haz the power, 2011-12-02 18:42:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rginda@chromium.org/8680034/21001

[commit-bot: I haz the power, 2011-12-02 18:42:48]

Presubmit check for 8680034-21001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  chrome/browser/resources/hterm/js/terminal.js

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/hterm/html/test_harness.html,chrome/browser/resources/hterm/js/scrollport.js,chrome/browser/resources/hterm/js/terminal_tests.js,chrome/browser/resources/hterm/js/screen.js,chrome/browser/resources/hterm/js/vt100.js,chrome/browser/resources/hterm/js/terminal.js,chrome/browser/resources/hterm/js/options.js,chrome/browser/resources/hterm/js/scrollport_tests.js,chrome/browser/resources/hterm/js/hterm.js,chrome/browser/resources/hterm/js/mock_row_provider.js,chrome/browser/resources/hterm/js/screen_tests.js,chrome/browser/resources/hterm/js/test_manager.js,chrome/browser/resources/hterm/js/vt100_tests.js

Presubmit checks took 1.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[arv (Not doing code reviews), 2011-12-02 21:14:03]

LGTM rubberstamp

[commit-bot: I haz the power, 2011-12-02 21:48:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rginda@chromium.org/8680034/21001

[commit-bot: I haz the power, 2011-12-02 21:48:27]

Presubmit check for 8680034-21001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  chrome/browser/resources/hterm/js/terminal.js

Presubmit checks took 1.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2011-12-02 21:53:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rginda@chromium.org/8680034/29001

[commit-bot: I haz the power, 2011-12-02 23:55:48]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 112797, job name
was 8680034-29001 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-12-05 17:53:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rginda@chromium.org/8680034/29001

[commit-bot: I haz the power, 2011-12-05 18:57:07]

Try job failure for 8680034-29001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-12-05 21:59:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rginda@chromium.org/8680034/29001

[commit-bot: I haz the power, 2011-12-05 23:50:32]

Change committed as 113066