Issue 8782001: Refactored HostList to better support bookmarking.

Issue 8782001: Refactored HostList to better support bookmarking. (Closed)

Created:
9 years ago by Jamie

Modified:
9 years ago

Reviewers:
Wez

CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, sergeyu+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Refactored HostList to better support bookmarking. BUG=106218, 104368 TEST=Bookmark a host, then restart it. The bookmark should still work. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114116

Patch Set 1 #

Patch Set 2 : Linter and rebase. #

Total comments: 16

Patch Set 3 : Address reviewer comments #

Patch Set 4 : Add unexpected error message. #

Patch Set 5 : Rebase #

Patch Set 6 : Merge bug. #

Created: 9 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+134 lines, -69 lines)			Patch
M	remoting/webapp/me2mom/_locales/en/messages.json	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download
M	remoting/webapp/me2mom/choice.html	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/webapp/me2mom/client_screen.js	View	1 2 3 4	5 chunks	+54 lines, -17 lines	0 comments	Download
M	remoting/webapp/me2mom/host_list.js	View	1 2 3 4 5	9 chunks	+68 lines, -41 lines	0 comments	Download
M	remoting/webapp/me2mom/host_table_entry.js	View	1 2 3 4	1 chunk	+1 line, -4 lines	0 comments	Download
M	remoting/webapp/me2mom/remoting.js	View	1 2 3 4	2 chunks	+4 lines, -5 lines	0 comments	Download
M	remoting/webapp/me2mom/ui_mode.js	View		1 chunk	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Wez

Couple of issues regarding error handling and removed-host handling. I think it'd also be helpful ...

9 years ago (2011-12-03 01:26:32 UTC) #2

Jamie

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/client_screen.js File remoting/webapp/me2mom/client_screen.js (right): http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/client_screen.js#newcode40 remoting/webapp/me2mom/client_screen.js:40: * facilitate refresh/reconnect if the cached host information is ...

9 years ago (2011-12-05 20:49:08 UTC) #3

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

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_screen.js:40: *     facilitate refresh/reconnect
if the cached host information is stale.
On 2011/12/03 01:26:33, Wez wrote:
> nit: the Id is used when re-connecting, since the JID may have changed

Done.

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_screen.js:455: remoting.currentConnectionType =
remoting.ConnectionType.Me2Me;
On 2011/12/03 01:26:33, Wez wrote:
> nit: blank line here?

Done.

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_screen.js:460: var host =
remoting.hostList.getHostForId(hostId);
On 2011/12/03 01:26:33, Wez wrote:
> Presumably this will return null if the Host Id is no longer valid, e.g. if
the
> Host was removed?  We should detect that case and not retry, I think?

I'm not so sure. It's possible that a deleted host can come back, in which case
it's reasonable for bookmarks to that host to continue to work. If we don't
retry if the host is NULL, then the bookmark will work in this case iff the user
has visited the main host screen in the interim, which I don't think is very
intuitive. The cost of not optimising for the case is that it will take a couple
more seconds to fail if the bookmark is genuinely stale.

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

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:49: this.lastError_ = '';
On 2011/12/03 01:26:33, Wez wrote:
> nit: blank line here, please

Done.

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:52:
(window.localStorage.getItem(remoting.HostList.HOSTS_KEY));
On 2011/12/03 01:26:33, Wez wrote:
> nit: indent

Done.

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:130: this.lastError_ =
remoting.Error.GENERIC;
On 2011/12/03 01:26:33, Wez wrote:
> I specifically avoided sending them to the reauthorize page for non-400
errors,
> since it won't help them.

We have to show some error message in this case. I remember a conversation with
Hugh in which he requested that we never report an error without suggesting a
course of action, even if it might not work. I'd be happy to have a generic
error message that just suggests "Try again later" in this case.

Actually, talking about this makes me realize that the user has no means of
signing out from the error page, so I've fixed up the HTML to allow that.

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:137: this.lastError_ =
remoting.Error.GENERIC;
On 2011/12/03 01:26:33, Wez wrote:
> I specifically avoided barfing them to the reauthorize prompt in this case,
> since it's a protocol error, and so re-auth presumably won't help?

See above.

Wez

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/client_screen.js File remoting/webapp/me2mom/client_screen.js (right): http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/client_screen.js#newcode460 remoting/webapp/me2mom/client_screen.js:460: var host = remoting.hostList.getHostForId(hostId); On 2011/12/05 20:49:08, Jamie wrote: ...

9 years ago (2011-12-05 20:59:38 UTC) #4

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

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/clien...
remoting/webapp/me2mom/client_screen.js:460: var host =
remoting.hostList.getHostForId(hostId);
On 2011/12/05 20:49:08, Jamie wrote:
> On 2011/12/03 01:26:33, Wez wrote:
> > Presumably this will return null if the Host Id is no longer valid, e.g. if
> the
> > Host was removed?  We should detect that case and not retry, I think?
> 
> I'm not so sure. It's possible that a deleted host can come back, in which
case
> it's reasonable for bookmarks to that host to continue to work. If we don't
> retry if the host is NULL, then the bookmark will work in this case iff the
user
> has visited the main host screen in the interim, which I don't think is very
> intuitive. The cost of not optimising for the case is that it will take a
couple
> more seconds to fail if the bookmark is genuinely stale.

For a deleted host to "come back" it would have to be re-registered, so it'll
have a different key pair => not the same host.  So I don't think that can ever
happen.

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

http://codereview.chromium.org/8782001/diff/1001/remoting/webapp/me2mom/host_...
remoting/webapp/me2mom/host_list.js:130: this.lastError_ =
remoting.Error.GENERIC;
On 2011/12/05 20:49:08, Jamie wrote:
> On 2011/12/03 01:26:33, Wez wrote:
> > I specifically avoided sending them to the reauthorize page for non-400
> errors,
> > since it won't help them.
> 
> We have to show some error message in this case. I remember a conversation
with
> Hugh in which he requested that we never report an error without suggesting a
> course of action, even if it might not work. I'd be happy to have a generic
> error message that just suggests "Try again later" in this case.
> 
> Actually, talking about this makes me realize that the user has no means of
> signing out from the error page, so I've fixed up the HTML to allow that.

Suggesting courses of action that we know won't help is worse than suggesting
nothing, surely?  What we really mean is that we never expect such response
codes, so if we see them then it's a bug the user should report.

Jamie

As discussed, it is worth refreshing if the host is missing because it copes with ...

9 years ago (2011-12-12 20:02:51 UTC) #5

lgtm

Expand Messages | Collapse Messages