Issue 944183002: HostTableEntry refactor

Issue 944183002: HostTableEntry refactor (Closed)

Created:
5 years, 10 months ago by kelvinp

Modified:
5 years, 9 months ago

Reviewers:
Jamie

CC:
chromium-reviews, chromoting-reviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

HostTableEntry refactor The HostTableEntry is currently shared between the local host section and the host list. However, they have slightly different creation behavior. For one, the DOM is generated on the fly for the HostList whereas the local host section has its DOM hard coded in the ui_me2me.html. In addition, the existing HostTableEntry expects the host to be passed in during construction time. This is slightly awkward for the LocalHostSection as the host is null until the local host has started. After the refactoring, the HostTableEntry generates the DOM in its constructor. The host is passed in via the setHost() function that updates the DOM using querySelector and innerText. The refactor also combines updateStatus() and setHostName() into once single function updateUI() that refreshes the UI based on the underlying state (the host). This CL also separates the local host section in the hostlist into its own class. BUG=461539 Committed: https://crrev.com/f95677ac2cfa32589e20bd93be2f5320b39e31ad Cr-Commit-Position: refs/heads/master@{#319038}

Patch Set 1 #

Total comments: 17

Patch Set 2 : Reviewer's feedbacks with unit tests #

Total comments: 34

Patch Set 3 : Rebase + Merge with Gary's change #

Patch Set 4 : Reviewer's feedback (Jamie) #

Patch Set 5 : Update browser tests #

Total comments: 2

Patch Set 6 : Address Jamie's feedback #

Created: 5 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+706 lines, -439 lines)			Patch
M	chrome/test/remoting/remote_desktop_browsertest.cc	View	1 2 3 4	2 chunks	+7 lines, -6 lines	0 comments	Download
M	remoting/remoting_webapp_files.gypi	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	remoting/webapp/base/html/main.css	View	1 2 3 4	3 chunks	+12 lines, -2 lines	0 comments	Download
M	remoting/webapp/base/js/base.js	View	1 2	1 chunk	+29 lines, -0 lines	0 comments	Download
M	remoting/webapp/browser_test/browser_test.js	View	1 2 3	3 chunks	+7 lines, -4 lines	0 comments	Download
M	remoting/webapp/browser_test/update_pin_browser_test.js	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/webapp/crd/html/ui_me2me.html	View	1 2 3	2 chunks	+5 lines, -18 lines	0 comments	Download
M	remoting/webapp/crd/js/crd_event_handlers.js	View	1 2	2 chunks	+1 line, -11 lines	0 comments	Download
M	remoting/webapp/crd/js/crd_main.js	View	1 2 3	2 chunks	+1 line, -3 lines	0 comments	Download
M	remoting/webapp/crd/js/host_list.js	View	1 2 3	8 chunks	+22 lines, -98 lines	0 comments	Download
M	remoting/webapp/crd/js/host_table_entry.js	View	1 2 3 4 5	3 chunks	+244 lines, -291 lines	0 comments	Download
A	remoting/webapp/crd/js/local_host_section.js	View	1 2 3 4 5	1 chunk	+151 lines, -0 lines	0 comments	Download
M	remoting/webapp/js_proto/qunit_proto.js	View	1 2	1 chunk	+4 lines, -2 lines	0 comments	Download
M	remoting/webapp/js_proto/sinon_proto.js	View	1 2	1 chunk	+2 lines, -1 line	0 comments	Download
M	remoting/webapp/js_proto/sinon_stub_proto.js	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
A	remoting/webapp/unittests/host_table_entry_unittest.js	View	1 2 3	1 chunk	+212 lines, -0 lines	0 comments	Download
M	remoting/webapp/unittests/sinon_helpers.js	View	1 2	1 chunk	+3 lines, -2 lines	0 comments	Download

Messages

Total messages: 19 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

kelvinp

PTAL. Hey Jamie, I did this refactor as a ground work for improving the UI ...

5 years, 10 months ago (2015-02-20 23:48:44 UTC) #2

Jamie

Please proceed. I like the direction this is going in. https://codereview.chromium.org/944183002/diff/1/remoting/webapp/crd/html/ui_me2me.html File remoting/webapp/crd/html/ui_me2me.html (right): https://codereview.chromium.org/944183002/diff/1/remoting/webapp/crd/html/ui_me2me.html#newcode68 ...

5 years, 10 months ago (2015-02-23 22:28:12 UTC) #3

kelvinp

PTAL https://codereview.chromium.org/944183002/diff/1/remoting/webapp/crd/html/ui_me2me.html File remoting/webapp/crd/html/ui_me2me.html (right): https://codereview.chromium.org/944183002/diff/1/remoting/webapp/crd/html/ui_me2me.html#newcode68 remoting/webapp/crd/html/ui_me2me.html:68: <div class='local-host-info'></div> On 2015/02/23 22:28:11, Jamie wrote: > ...

5 years, 10 months ago (2015-02-24 22:04:45 UTC) #4

Jamie

https://codereview.chromium.org/944183002/diff/1/remoting/webapp/crd/js/local_host_section.js File remoting/webapp/crd/js/local_host_section.js (right): https://codereview.chromium.org/944183002/diff/1/remoting/webapp/crd/js/local_host_section.js#newcode27 remoting/webapp/crd/js/local_host_section.js:27: var webappVersion = parseInt(chrome.runtime.getManifest().version, 10); On 2015/02/24 22:04:45, kelvinp ...

5 years, 9 months ago (2015-02-27 18:19:41 UTC) #7

kelvinp

PTAL Sorry about the rebasing, my change will no longer compile will Gary's JSCompile change. ...

5 years, 9 months ago (2015-03-03 21:57:54 UTC) #12

PTAL

Sorry about the rebasing, my change will no longer compile will Gary's JSCompile
change. To avoid sending out another iteration after an lgtm, I choose to rebase
first.

I have separated the rebase, feedback address and browser test fixing in
separate patch sets.  Hope that would make the reviewing process easier.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/base/htm...
File remoting/webapp/base/html/main.css (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/base/htm...
remoting/webapp/base/html/main.css:509: background-color: #f2f2f2;
On 2015/02/27 18:19:40, Jamie wrote:
> I would expect this CSS to be identical to the equivalent CSS for the other
> rows--spacing etc. might differ, but the hover colour should not. Can you just
> add a selector to the rule for the other rows rather than duplicating the
colour
> definition?

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_list.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_list.js:80: new
remoting.LocalHostSection.Controller(this));
On 2015/02/27 18:19:40, Jamie wrote:
> These elements (or a suitable parent through which they can be accessed)
should
> be passed to the ctor like all the other DOM elements are. In fact, it might
be
> cleaner to make all the host-list DOM accessible via querySelector and just
pass
> in the root (that can be a follow-up, but I would prefer that we don't add
calls
> to getElementById to this class).

Acknowledged. I simply moved the line to query the daemon control by Id from
line 290 to here.  Currently, the daemon-control is not a child of the host list
element that is passed in the constructor.  I can do this in a follow-up change.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_table_entry.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_table_entry.js:122:
container.querySelector('.box-spacer input'));
On 2015/02/27 18:19:40, Jamie wrote:
> This should be accessed explicitly by class name, not by its current position
in
> the DOM; the latter could conceivably change with no obvious indication that
> this call site would need to change with it.

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_table_entry.js:139: /** HTMLElement */ button, /**
Function */ callback) {
On 2015/02/27 18:19:41, Jamie wrote:
> Don't you need @type to annotate parameters in this syntax?

No, this is magic of the new syntax that John shows me :)

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_table_entry.js:166: if (!this.isRenaming_() &&
this.isOnline_()) {
On 2015/02/27 18:19:40, Jamie wrote:
> Add a comment explaining why we don't connect if we're renaming (presumably
its
> because this event fires if the user hits Enter after typing a new name)?

That's exactly it :) Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_table_entry.js:233:
base.dispose(this.renameInputEventHooks_);
On 2015/02/27 18:19:41, Jamie wrote:
> You're using the existence of renameInputHooks_ elsewhere as an indication the
> renaming is not taking place, so it seems like it should always be null on
entry
> to beginRename. Maybe an assert would be better here?

An assert may not work as it will fire if the user clicks on the rename icon
while an existing rename is in progress.  I added an extra check instead.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_table_entry.js:262: // Update the tool-top and event
handler.
On 2015/02/27 18:19:40, Jamie wrote:
> s/top/top/

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/l...
File remoting/webapp/crd/js/local_host_section.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/l...
remoting/webapp/crd/js/local_host_section.js:38: var changePINButton =
rootElement.querySelector('#change-daemon-pin');
On 2015/02/27 18:19:41, Jamie wrote:
> Can you query these by class instead of id? Querying by id breaks the
> reusability abstraction that you gain by passing in the root element to the
> ctor.

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/l...
remoting/webapp/crd/js/local_host_section.js:44: new
base.DomEventHook(changePINButton, 'click', controller.changePIN, false)
On 2015/02/27 18:19:41, Jamie wrote:
> I think these are all missing bind()s.

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/l...
remoting/webapp/crd/js/local_host_section.js:93: (this.isEnabled_() ||
!this.hasError_);
On 2015/02/27 18:19:41, Jamie wrote:
> This conditional is hard to parse. Can you break it into assignments to
> appropriately named variables, and then return a simple boolean function of
> those variables?

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/l...
remoting/webapp/crd/js/local_host_section.js:121:
remoting.hostSetupDialog.showForStart();
On 2015/02/27 18:19:41, Jamie wrote:
> Maybe pass the HostSetupDialog to the ctor for better encapsulation?

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/unittest...
File remoting/webapp/unittests/host_table_entry_unittest.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/host_table_entry_unittest.js:63: test('Clicking
confirm on confirm dialog deletes the host', function() {
On 2015/02/27 18:19:41, Jamie wrote:
> Should there also be a test for clicking Cancel?

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/host_table_entry_unittest.js:93: test('Host renaming
is canceled when input field lost focus.', function() {
On 2015/02/27 18:19:41, Jamie wrote:
> s/lost/loses/

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/host_table_entry_unittest.js:107: test('Host renaming
commits on ENTER.', function() {
On 2015/02/27 18:19:41, Jamie wrote:
> Add a test for Escape as well?

Done.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/host_table_entry_unittest.js:160: 
On 2015/02/27 18:19:41, Jamie wrote:
> Can you add a test for host-out-of-date as well? It should verify the display
> name and that the upgrade interstitial is shown.

Done.  Unfortunately, the host out of date warning pop-up is not currently owned
by this class, so that part won't be verified here.

Jamie

Mostly looks good. https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/host_list.js File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/host_list.js#newcode80 remoting/webapp/crd/js/host_list.js:80: new remoting.LocalHostSection.Controller(this)); On 2015/03/03 21:57:52, kelvinp ...

5 years, 9 months ago (2015-03-04 00:09:25 UTC) #13

Mostly looks good.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_list.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_list.js:80: new
remoting.LocalHostSection.Controller(this));
On 2015/03/03 21:57:52, kelvinp wrote:
> On 2015/02/27 18:19:40, Jamie wrote:
> > These elements (or a suitable parent through which they can be accessed)
> should
> > be passed to the ctor like all the other DOM elements are. In fact, it might
> be
> > cleaner to make all the host-list DOM accessible via querySelector and just
> pass
> > in the root (that can be a follow-up, but I would prefer that we don't add
> calls
> > to getElementById to this class).
> 
> Acknowledged. I simply moved the line to query the daemon control by Id from
> line 290 to here.  Currently, the daemon-control is not a child of the host
list
> element that is passed in the constructor.  I can do this in a follow-up
change.

"this" being what, in this context? Refactoring the DOM so that it is a child?

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_table_entry.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_table_entry.js:233:
base.dispose(this.renameInputEventHooks_);
On 2015/03/03 21:57:53, kelvinp wrote:
> On 2015/02/27 18:19:41, Jamie wrote:
> > You're using the existence of renameInputHooks_ elsewhere as an indication
the
> > renaming is not taking place, so it seems like it should always be null on
> entry
> > to beginRename. Maybe an assert would be better here?
> 
> An assert may not work as it will fire if the user clicks on the rename icon
> while an existing rename is in progress.  I added an extra check instead.

I don't think that's true. Clicking a second rename button should cause a blur
event on the edit box, dismissing it and disposing of the hooks; then they will
be null when the click event fires.

The reason I don't like calling dispose "just in case" is that it's either
always a no-op (in which case an assert more clearly documents the assumptions
under which the code was written), or you're acknowledging that the object might
still be live, which raises the question of what happens when the code that
creates it tries to clean it up (answer: it cleans up your replacement instead).

https://codereview.chromium.org/944183002/diff/180001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/local_host_section.js (right):

https://codereview.chromium.org/944183002/diff/180001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/local_host_section.js:97: remoting.isMe2MeInstallable());
Shouldn't this be isMe2MeInstallable() && state == NOT_INSTALLED?

kelvinp

PTAL https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/host_list.js File remoting/webapp/crd/js/host_list.js (right): https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/host_list.js#newcode80 remoting/webapp/crd/js/host_list.js:80: new remoting.LocalHostSection.Controller(this)); On 2015/03/04 00:09:24, Jamie wrote: > ...

5 years, 9 months ago (2015-03-04 02:02:28 UTC) #14

PTAL

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_list.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_list.js:80: new
remoting.LocalHostSection.Controller(this));
On 2015/03/04 00:09:24, Jamie wrote:
> On 2015/03/03 21:57:52, kelvinp wrote:
> > On 2015/02/27 18:19:40, Jamie wrote:
> > > These elements (or a suitable parent through which they can be accessed)
> > should
> > > be passed to the ctor like all the other DOM elements are. In fact, it
might
> > be
> > > cleaner to make all the host-list DOM accessible via querySelector and
just
> > pass
> > > in the root (that can be a follow-up, but I would prefer that we don't add
> > calls
> > > to getElementById to this class).
> > 
> > Acknowledged. I simply moved the line to query the daemon control by Id from
> > line 290 to here.  Currently, the daemon-control is not a child of the host
> list
> > element that is passed in the constructor.  I can do this in a follow-up
> change.
> 
> "this" being what, in this context? Refactoring the DOM so that it is a child?

Correct, what I meant is that I can refactor the DOM so that the
|daemon-control| will be a child of the rootElement passed into
remoting.HostList.

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_table_entry.js (right):

https://codereview.chromium.org/944183002/diff/40001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_table_entry.js:233:
base.dispose(this.renameInputEventHooks_);
On 2015/03/04 00:09:25, Jamie wrote:
> On 2015/03/03 21:57:53, kelvinp wrote:
> > On 2015/02/27 18:19:41, Jamie wrote:
> > > You're using the existence of renameInputHooks_ elsewhere as an indication
> the
> > > renaming is not taking place, so it seems like it should always be null on
> > entry
> > > to beginRename. Maybe an assert would be better here?
> > 
> > An assert may not work as it will fire if the user clicks on the rename icon
> > while an existing rename is in progress.  I added an extra check instead.
> 
> I don't think that's true. Clicking a second rename button should cause a blur
> event on the edit box, dismissing it and disposing of the hooks; then they
will
> be null when the click event fires.
> 
> The reason I don't like calling dispose "just in case" is that it's either
> always a no-op (in which case an assert more clearly documents the assumptions
> under which the code was written), or you're acknowledging that the object
might
> still be live, which raises the question of what happens when the code that
> creates it tries to clean it up (answer: it cleans up your replacement
instead).

Done.

https://codereview.chromium.org/944183002/diff/180001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/local_host_section.js (right):

https://codereview.chromium.org/944183002/diff/180001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/local_host_section.js:97: remoting.isMe2MeInstallable());
On 2015/03/04 00:09:25, Jamie wrote:
> Shouldn't this be isMe2MeInstallable() && state == NOT_INSTALLED?

Done.

commit-bot: I haz the power

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

5 years, 9 months ago (2015-03-04 07:06:00 UTC) #17

commit-bot: I haz the power

5 years, 9 months ago (2015-03-04 08:58:54 UTC) #19

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/f95677ac2cfa32589e20bd93be2f5320b39e31ad
Cr-Commit-Position: refs/heads/master@{#319038}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages