Implement rename and delete.

Implement rename and delete.

BUG=None
TEST=Manual

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

[Jamie, 2011-11-17 21:42:09]

Simon, I think you're probably best placed to look at this. In particular, is my
generalization of the various xhr methods correct? I've assumed that GET and
DELETE put the data in the URL, whereas POST and PUT always put the data in the
message body (and POST needs the application/x-www-form-urlencoded header). That
was based on what worked for our APIs, but I wonder if it's a valid assumption
in general.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/choic...
File remoting/webapp/me2mom/choice.css (left):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/choic...
remoting/webapp/me2mom/choice.css:205: -webkit-transition: all 0.5s;
Animating colour changes proved to be more trouble than it was worth.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:45: };
I separated out the raw object (Host) we get back from the list API so that it
can be sent back as an update without worrying about the extra fields we add to
keep track of the UI (HostTableEntry).

[Wez, 2011-11-17 22:58:57]

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:220: function() {}, '', headers);
Why not refresh the host list when the XHR completes?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:290: 'content-type' : 'application/json;
charset=UTF-8'
nit: content-type capitalization.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:295: function(xhr) {},
Why not refresh when the XHR returns?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:312: var host = this.getHostForId(hostId);
How can the user have cancelled editing if the host doesn't exist?  Surely the
UI would have updated under their feet?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:339: remoting.hostList = null;
Newline change?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.js
File remoting/webapp/me2mom/xhr.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:125: * @param {string} url The base URL to POST,
excluding parameters.
nit: lose "to POST", and clarify that the URL should be URL-escaped already?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:153: parameterString =
remoting.xhr.urlencodeParamHash(opt_parameters);
Are you sure you should URL-encode the PUT body, give that the Content-type
won't be x-www-form-urlencoded?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:160: var useBody = (methodName == 'POST') ||
(methodName == 'PUT');
Do we ever POST a large enough chunk of data to justify putting it as a form,
rather than parameters?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:168: xhr.setRequestHeader('Content-type',
'application/x-www-form-urlencoded');
So presumably the caller _must_ specify the content-type for PUT?

[simonmorris, 2011-11-17 23:59:48]

I think REST dogma is that the URL is the name of the
resource, and a message body is the resource itself.
So I agree that GET and DELETE have no body, and POST and
PUT do. But maybe it would be clearer for the remoting.xhr
methods to take an opt_body argument, instead of sometimes
putting the opt_parameters in the body? If you want to
keep that out of the interface, then you could
put a REST interface on top of it, that maps UPDATE to PUT,
etc.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/clien...
File remoting/webapp/me2mom/client_screen.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_screen.js:396: if (hostTableEntry) {
"if (!hostTableEntry) { return; }" is simpler.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:45: };
On 2011/11/17 21:42:09, Jamie wrote:
> I separated out the raw object (Host) we get back from the list API so that it
> can be sent back as an update without worrying about the extra fields we add
to
> keep track of the UI (HostTableEntry).

Maybe HostModel and HostView would be more familiar styles
of name?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:154:
hostTableEntry.tableRow.appendChild(removeButton);
Clearer to make this new code a method of HostTableEntry
(aka HostView)?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:198: * Remove a host from the list, and
deregister it.
Won't the user expect this to have some effect on the
host itself? Are we happy with a user having a running
host whose heartbeats are failing, but is still connected
to Talk?

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:253: editBox.select();
This could be a method of HostView.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:285: hostTableEntry.host.status = 'OFFLINE';
Why is the status set here?

[Jamie, 2011-11-18 22:54:57]

Regarding adding a REST helper: I think I'd rather leave it as a xhr helper for
now, but I think it does need a bit of TLC to make it easier to understand.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/clien...
File remoting/webapp/me2mom/client_screen.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_screen.js:396: if (hostTableEntry) {
On 2011/11/17 23:59:48, simonmorris wrote:
> "if (!hostTableEntry) { return; }" is simpler.

Done.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:45: };
On 2011/11/17 23:59:48, simonmorris wrote:
> On 2011/11/17 21:42:09, Jamie wrote:
> > I separated out the raw object (Host) we get back from the list API so that
it
> > can be sent back as an update without worrying about the extra fields we add
> to
> > keep track of the UI (HostTableEntry).
> 
> Maybe HostModel and HostView would be more familiar styles
> of name?

It's not really MVC pattern though. I'd prefer to stick with these names unless
you have strong objections.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:154:
hostTableEntry.tableRow.appendChild(removeButton);
On 2011/11/17 23:59:48, simonmorris wrote:
> Clearer to make this new code a method of HostTableEntry
> (aka HostView)?

Good point, thanks for spotting this. I've refactored this quite extensively, so
please recheck it.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:198: * Remove a host from the list, and
deregister it.
On 2011/11/17 23:59:48, simonmorris wrote:
> Won't the user expect this to have some effect on the
> host itself? Are we happy with a user having a running
> host whose heartbeats are failing, but is still connected
> to Talk?

I think the long-term plan is to have hosts shut down if their heartbeats are
failing.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:220: function() {}, '', headers);
On 2011/11/17 22:58:58, Wez wrote:
> Why not refresh the host list when the XHR completes?

I'm not sure about that. It's only going to make a difference if the deletion
fails or if other hosts have been added in the interim. In both cases, I think
it would feel a little jarring.

One option would be to freeze the UI until we get a response, which would allow
us to report errors cleanly. I want to run that by Ken before I implement it
though, so I'd prefer to leave this as-is for now.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:253: editBox.select();
On 2011/11/17 23:59:48, simonmorris wrote:
> This could be a method of HostView.

My refactoring addresses this.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:285: hostTableEntry.host.status = 'OFFLINE';
On 2011/11/17 23:59:48, simonmorris wrote:
> Why is the status set here?

Left-over debugging code. Removed.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:290: 'content-type' : 'application/json;
charset=UTF-8'
On 2011/11/17 22:58:58, Wez wrote:
> nit: content-type capitalization.

Done.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:295: function(xhr) {},
On 2011/11/17 22:58:58, Wez wrote:
> Why not refresh when the XHR returns?

Same argument as above.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:312: var host = this.getHostForId(hostId);
On 2011/11/17 22:58:58, Wez wrote:
> How can the user have cancelled editing if the host doesn't exist?  Surely the
> UI would have updated under their feet?

It's just a sanity check. I'd make it a DCHECK if JS supported such things.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:339: remoting.hostList = null;
On 2011/11/17 22:58:58, Wez wrote:
> Newline change?

Yep. The original was missing a newline.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.js
File remoting/webapp/me2mom/xhr.js (right):

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:125: * @param {string} url The base URL to POST,
excluding parameters.
On 2011/11/17 22:58:58, Wez wrote:
> nit: lose "to POST", and clarify that the URL should be URL-escaped already?

I've done the first bit. I don't think the second is necessary since the comment
states that it's a URL.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:153: parameterString =
remoting.xhr.urlencodeParamHash(opt_parameters);
On 2011/11/17 22:58:58, Wez wrote:
> Are you sure you should URL-encode the PUT body, give that the Content-type
> won't be x-www-form-urlencoded?

Now that you mention it, no. Since we never currently pass in an object hash for
PUT, it doesn't matter, but I'll clean this up in a follow-up CL.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:160: var useBody = (methodName == 'POST') ||
(methodName == 'PUT');
On 2011/11/17 22:58:58, Wez wrote:
> Do we ever POST a large enough chunk of data to justify putting it as a form,
> rather than parameters?

No, but I don't think it works otherwise. I'll double check when I clean this
up.

http://codereview.chromium.org/8587050/diff/1008/remoting/webapp/me2mom/xhr.j...
remoting/webapp/me2mom/xhr.js:168: xhr.setRequestHeader('Content-type',
'application/x-www-form-urlencoded');
On 2011/11/17 22:58:58, Wez wrote:
> So presumably the caller _must_ specify the content-type for PUT?

Unless there's a default. We only call it in one place, and we do specify the
content type there.

[simonmorris, 2011-11-18 23:17:19]

LGTM

Only one nit.

http://codereview.chromium.org/8587050/diff/4003/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8587050/diff/4003/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:17: * be have a common parent that contains
only host-list UI as it will be hidden
s/be //

[Jamie, 2011-11-19 00:00:59]

http://codereview.chromium.org/8587050/diff/4003/remoting/webapp/me2mom/host_...
File remoting/webapp/me2mom/host_list.js (right):

http://codereview.chromium.org/8587050/diff/4003/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:17: * be have a common parent that contains
only host-list UI as it will be hidden
On 2011/11/18 23:17:19, simonmorris wrote:
> s/be //

Done.

[commit-bot: I haz the power, 2011-11-19 00:04:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/8587050/16003