Issue 233623003: Remember and restore the account last used destination is registered for.

Issue 233623003: Remember and restore the account last used destination is registered for. (Closed)

Created:
6 years, 8 months ago by Aleksey Shlyapnikov

Modified:
6 years, 8 months ago

Reviewers:
Toscano, Vitaly Buka (NO REVIEWS)

CC:
chromium-reviews, arv+watch_chromium.org

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

Visibility:
Public.

More Reviews

Description

Remember and restore the account last used destination is registered for. Other changes: - Decouple active user change and destinations reload; - Properly pass XSRF token for POST requests and store them per user account; - Remove deprecated IS_SELECTED_DESTINATION_LOCAL setting; - Fix typo in AppState.Field.SELECTED_DESTINATION_CAPABILITIES; - Move initial default destination decision to more appropriate place (AppState); - Make AppState the source of the last selected destination info; - Handle selectedDestination == null case; BUG=139170 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264172

Patch Set 1 #

Total comments: 24

Patch Set 2 : Address CL comments. #

Total comments: 2

Patch Set 3 : Handle uninitialized user info case. #

Patch Set 4 : Cancel outstanding search requests only when another search request was made. #

Total comments: 2

Patch Set 5 : Attribure received XSRF token to the user account set in the response, not the account set in reque… #

Patch Set 6 : Adjust PrintPreview unit test. #

Created: 6 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+498 lines, -367 lines)			Patch
M	chrome/browser/resources/print_preview/cloud_print_interface.js	View	1 2 3 4	15 chunks	+160 lines, -70 lines	0 comments	Download
M	chrome/browser/resources/print_preview/data/app_state.js	View	1	3 chunks	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/print_preview/data/destination_store.js	View	1	27 chunks	+237 lines, -230 lines	0 comments	Download
M	chrome/browser/resources/print_preview/data/user_info.js	View	1 2	4 chunks	+16 lines, -7 lines	0 comments	Download
M	chrome/browser/resources/print_preview/print_header.js	View		2 chunks	+11 lines, -8 lines	0 comments	Download
M	chrome/browser/resources/print_preview/print_preview.js	View	1	2 chunks	+5 lines, -3 lines	0 comments	Download
M	chrome/browser/resources/print_preview/search/destination_search.js	View	1 2	9 chunks	+28 lines, -19 lines	0 comments	Download
M	chrome/browser/resources/print_preview/settings/destination_settings.js	View		1 chunk	+33 lines, -29 lines	0 comments	Download
M	chrome/test/data/webui/print_preview.js	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 16 (0 generated)

Expand Messages | Collapse Messages

Vitaly Buka (NO REVIEWS)

lgtm https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js#newcode291 chrome/browser/resources/print_preview/cloud_print_interface.js:291: params, inconsistent https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js#newcode544 chrome/browser/resources/print_preview/cloud_print_interface.js:544: printerList.push(cloudprint.CloudDestinationParser.parse( maybe extractPrinters_ https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js#newcode551 ...

6 years, 8 months ago (2014-04-11 02:32:57 UTC) #2

Toscano

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js#newcode207 chrome/browser/resources/print_preview/cloud_print_interface.js:207: this.userInfo_.activeUser, Hmm, seems like a hidden contract that the ...

6 years, 8 months ago (2014-04-11 04:23:59 UTC) #3

Aleksey Shlyapnikov

6 years, 8 months ago (2014-04-11 19:09:29 UTC) #4

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:207:
this.userInfo_.activeUser,
On 2014/04/11 04:24:00, Toscano wrote:
> Hmm, seems like a hidden contract that the search function always searches for
> the active user. I'm not sure it makes sense for cloud print interface to have
a
> dependency on "active user". Why not have the caller decide who's destinations
> to search for, just like it already decides which origin to use. Also, using
the
> active user here makes it inconsistent with the "printer" method.

Done.

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:291: params,
On 2014/04/11 02:32:57, Vitaly Buka wrote:
> inconsistent

Done.

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:434: var
activeUser = request.result['request']['user'];
On 2014/04/11 04:24:00, Toscano wrote:
> Maybe move this to right above line 440 since it's only used there?

Done.

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:467:
this.abortSearchRequests_([print_preview.Destination.Origin.COOKIES]);
The thing is, search request reply _is_ the source of information about the
current state of user sessions, so changing the user, we should make sure there
are no search requests in flight, otherwise, as soon as they are done, the
active user will be changed accordingly to the user info in the reply,
overriding our state with stale info (there's no guarantee in the order replies
arrive).

On 2014/04/11 04:24:00, Toscano wrote:
> Hmm, wouldn't it be simpler to only abort if and only if another search
request
> was made? Seems like a coupling of userInfo active user change and search for
> printers is unnecessary. For example, if there was another way to change the
> active user that didn't require an immediate search request would have to deal
> with this dependency.

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:544:
printerList.push(cloudprint.CloudDestinationParser.parse(
This is the only place printer list is parsed, I'd rather keep it here.

On 2014/04/11 02:32:57, Vitaly Buka wrote:
> maybe extractPrinters_

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:551:
this.setUsers_(request);
It updates userInfo_ state, not just extracts user info from the request.
Changed the comment to make it more explicit.

On 2014/04/11 02:32:57, Vitaly Buka wrote:
> setUsers_ -> extractUsers_

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:599: var users =
request.result['request']['users'];
Done.

On 2014/04/11 04:24:00, Toscano wrote:
> Let's keep user parsing logic in place. Why not this.setUsers_(request) and
then
> use the userSessionIndex map below?

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:666: *     is
sent on behalf of the primary user.
I don't see the benefit of optional here, it's a private class.

On 2014/04/11 04:24:00, Toscano wrote:
> If it's nullable, maybe it should be opt_account?

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/app_state.js (right):

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/app_state.js:145:
this.state_[AppState.Field.SELECTED_DESTINATION_ID] =
On 2014/04/11 04:24:00, Toscano wrote:
> On 2014/04/11 02:32:57, Vitaly Buka wrote:
> > this could be separate CL
> 
> agreed

Done.

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/app_state.js:145:
this.state_[AppState.Field.SELECTED_DESTINATION_ID] =
On 2014/04/11 02:32:57, Vitaly Buka wrote:
> this could be separate CL

Done.

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/data/destination_store.js (left):

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/data/destination_store.js:291: } else if
(systemDefaultDestinationId) {
On 2014/04/11 02:32:57, Vitaly Buka wrote:
> separate change?

Done.

Toscano

6 years, 8 months ago (2014-04-12 02:09:08 UTC) #5

lgtm

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:467:
this.abortSearchRequests_([print_preview.Destination.Origin.COOKIES]);
But wait, how can you change the user if the first reply hasn't finished yet?
Are you talking about the case where recent search finishes, and then the user
switches, and then the longer search finishes? But in this case, why does the
longer search replace the set of user sessions?

On 2014/04/11 19:09:29, Aleksey Shlyapnikov wrote:
> The thing is, search request reply _is_ the source of information about the
> current state of user sessions, so changing the user, we should make sure
there
> are no search requests in flight, otherwise, as soon as they are done, the
> active user will be changed accordingly to the user info in the reply,
> overriding our state with stale info (there's no guarantee in the order
replies
> arrive).
> 
> On 2014/04/11 04:24:00, Toscano wrote:
> > Hmm, wouldn't it be simpler to only abort if and only if another search
> request
> > was made? Seems like a coupling of userInfo active user change and search
for
> > printers is unnecessary. For example, if there was another way to change the
> > active user that didn't require an immediate search request would have to
deal
> > with this dependency.
>

https://codereview.chromium.org/233623003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/233623003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:602:
request.account != request.result['request']['user']) {
Hmm, but you left all of this parsing logic above. When is this false? When the
printer doesn't exist? In that case, can't you check the
request.result['success'] field?

Aleksey Shlyapnikov

I made more changes, including XSRF handling. PTAL. https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/print_preview/cloud_print_interface.js#newcode467 chrome/browser/resources/print_preview/cloud_print_interface.js:467: this.abortSearchRequests_([print_preview.Destination.Origin.COOKIES]); ...

6 years, 8 months ago (2014-04-12 03:44:02 UTC) #6

I made more changes, including XSRF handling. PTAL.

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/233623003/diff/1/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/cloud_print_interface.js:467:
this.abortSearchRequests_([print_preview.Destination.Origin.COOKIES]);
It's assumed that all search requests issued under the same active account
return the same set of sessions (which is not true in general, but close enough
for our purpose here). To the first part of your question, I was concerned about
printer caps request getting too long (the case when it switches active
account), but playing with it I realized that it's more user friendly to let
search requests finish in this case, so I agree, let's abort only when another
search request was made.

On 2014/04/12 02:09:08, Toscano wrote:
> But wait, how can you change the user if the first reply hasn't finished yet?
> Are you talking about the case where recent search finishes, and then the user
> switches, and then the longer search finishes? But in this case, why does the
> longer search replace the set of user sessions?
> 
> On 2014/04/11 19:09:29, Aleksey Shlyapnikov wrote:
> > The thing is, search request reply _is_ the source of information about the
> > current state of user sessions, so changing the user, we should make sure
> there
> > are no search requests in flight, otherwise, as soon as they are done, the
> > active user will be changed accordingly to the user info in the reply,
> > overriding our state with stale info (there's no guarantee in the order
> replies
> > arrive).
> > 
> > On 2014/04/11 04:24:00, Toscano wrote:
> > > Hmm, wouldn't it be simpler to only abort if and only if another search
> > request
> > > was made? Seems like a coupling of userInfo active user change and search
> for
> > > printers is unnecessary. For example, if there was another way to change
the
> > > active user that didn't require an immediate search request would have to
> deal
> > > with this dependency.
> > 
>

https://codereview.chromium.org/233623003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/print_preview/cloud_print_interface.js (right):

https://codereview.chromium.org/233623003/diff/20001/chrome/browser/resources...
chrome/browser/resources/print_preview/cloud_print_interface.js:602:
request.account != request.result['request']['user']) {
When the response was not created by GCP server. Only the GCP server populates
user and users fields
On 2014/04/12 02:09:09, Toscano wrote:
> Hmm, but you left all of this parsing logic above. When is this false? When
the
> printer doesn't exist? In that case, can't you check the
> request.result['success'] field?

Toscano

lgtm https://codereview.chromium.org/233623003/diff/60001/chrome/browser/resources/print_preview/cloud_print_interface.js File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/233623003/diff/60001/chrome/browser/resources/print_preview/cloud_print_interface.js#newcode500 chrome/browser/resources/print_preview/cloud_print_interface.js:500: this.xsrfTokens_[request.account] = request.result['xsrf_token']; Should we get rid of ...

6 years, 8 months ago (2014-04-14 20:53:44 UTC) #7

Aleksey Shlyapnikov

https://codereview.chromium.org/233623003/diff/60001/chrome/browser/resources/print_preview/cloud_print_interface.js File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/233623003/diff/60001/chrome/browser/resources/print_preview/cloud_print_interface.js#newcode500 chrome/browser/resources/print_preview/cloud_print_interface.js:500: this.xsrfTokens_[request.account] = request.result['xsrf_token']; Yep, I did, please check the ...

6 years, 8 months ago (2014-04-15 01:20:12 UTC) #8

Aleksey Shlyapnikov

The CQ bit was checked by alekseys@chromium.org

6 years, 8 months ago (2014-04-15 01:20:44 UTC) #9