cros: Only preload the lock screen; do not reuse it.

cros: Only preload the lock screen; do not reuse it.

BUG=669638
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2610373002
Cr-Commit-Position: refs/heads/master@{#443725}
Committed: https://chromium.googlesource.com/chromium/src/+/284264846b3b7131c5f1efc1482d42e06878dd98

[jdufault, 2017-01-05 19:59:02]

Description was changed from

==========
cros: Only preload the lock screen; do not reuse it.

BUG=669638
==========

to

==========
cros: Only preload the lock screen; do not reuse it.

BUG=669638
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jdufault, 2017-01-05 20:00:48]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 20:01:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2017-01-05 20:06:57]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 20:07:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 21:11:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 21:11:50]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-01-05 22:24:05]

Patchset #1 (id:1) has been deleted

[jdufault, 2017-01-05 22:24:43]

Description was changed from

==========
cros: Only preload the lock screen; do not reuse it.

BUG=669638
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
cros: Only preload the lock screen; do not reuse it.

BUG=669638
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jdufault, 2017-01-05 22:24:43]

jdufault@chromium.org changed reviewers:
+ xiyuan@chromium.org

[jdufault, 2017-01-05 22:25:09]

xiyuan@, PTAL when you get the chance.

[xiyuan, 2017-01-06 17:14:25]

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.h (right):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.h:99: static
std::unique_ptr<views::WebView> DoPreload(Profile* profile);
nit: move this after "friend" to match existing code

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/preloaded_web_view.cc (right):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/preloaded_web_view.cc:46: return
std::move(preloaded_instance_);
We should not use std::move on return statement unless the var type and return
type do not exactly match.

https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D....

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/preloaded_web_view.h (right):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/preloaded_web_view.h:38:
base::OnceCallback<std::unique_ptr<views::WebView>(Profile*)>;
nit: move this to the beginning of all declarations (before ctor). 

Style guide prefers the following order: types (including typedef, using, and
nested structs and classes), constants, factory functions, constructors,
assignment operators, destructor, all other methods, data members.

https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/lock.js (left):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/lock.js:88:
chrome.send('accountPickerReady');
Would we lost this signal? This signal triggers ScreenLocker::ScreenLockReady
that notifies session manager and sets session state.

How would it work in preloaded case?

[jdufault, 2017-01-09 22:00:37]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 22:01:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 23:53:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 23:53:41]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-01-13 19:46:58]

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/webui_screen_locker.h (right):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/webui_screen_locker.h:99: static
std::unique_ptr<views::WebView> DoPreload(Profile* profile);
On 2017/01/06 17:14:25, xiyuan wrote:
> nit: move this after "friend" to match existing code

Done.

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/preloaded_web_view.cc (right):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/preloaded_web_view.cc:46: return
std::move(preloaded_instance_);
On 2017/01/06 17:14:25, xiyuan wrote:
> We should not use std::move on return statement unless the var type and return
> type do not exactly match.
> 
>
https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D....

Unfortunately to elide the std::move the data needs to live on the stack. So for
this case, without the std::move the return invokes the copy ctor which is
deleted inside of unique_ptr.

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/preloaded_web_view.h (right):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/preloaded_web_view.h:38:
base::OnceCallback<std::unique_ptr<views::WebView>(Profile*)>;
On 2017/01/06 17:14:25, xiyuan wrote:
> nit: move this to the beginning of all declarations (before ctor). 
> 
> Style guide prefers the following order: types (including typedef, using, and
> nested structs and classes), constants, factory functions, constructors,
> assignment operators, destructor, all other methods, data members.
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/lock.js (left):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/lock.js:88:
chrome.send('accountPickerReady');
On 2017/01/06 17:14:25, xiyuan wrote:
> Would we lost this signal? This signal triggers ScreenLocker::ScreenLockReady
> that notifies session manager and sets session state.
> 
> How would it work in preloaded case?

Loading the webui doesn't actually initialize of the session specific data. I
believe before this was only needed if the webui had already been displayed once
before. So this change should be ok.

Tracing through everything, this normally gets sent via

  JS ScreenAccountPicker.onShow
  ...
  JS UserPodRow.handleAfterShow
  ...
  JS Oobe.showScreen {"data":{"disableAddUser":false},"id":"account-picker"}
  #1 chromeos::BaseScreenHandler::ShowScreenWithData()
  #2 chromeos::SigninScreenHandler::ShowImpl()
  #3 chromeos::SigninScreenHandler::Show()
  #4 chromeos::OobeUI::ShowSigninScreen()
  #5 chromeos::WebUIScreenLocker::LockScreen()
  #6 chromeos::ScreenLocker::Init()
  #7 chromeos::ScreenLocker::Show()
  #8 chromeos::ScreenLocker::HandleLockScreenRequest()

I'm guessing that one of those functions is doing caching and not calling the
rest of the sequence if the screen is already set to 'account-picker'.

[xiyuan, 2017-01-13 20:14:18]

lgtm

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/preloaded_web_view.cc (right):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/preloaded_web_view.cc:46: return
std::move(preloaded_instance_);
On 2017/01/13 19:46:58, jdufault wrote:
> On 2017/01/06 17:14:25, xiyuan wrote:
> > We should not use std::move on return statement unless the var type and
return
> > type do not exactly match.
> > 
> >
>
https://www.chromium.org/rvalue-references#TOC-11.-Don-t-return-references.-D....
> 
> Unfortunately to elide the std::move the data needs to live on the stack. So
for
> this case, without the std::move the return invokes the copy ctor which is
> deleted inside of unique_ptr.

Acknowledged.

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/lock.js (left):

https://codereview.chromium.org/2610373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/lock.js:88:
chrome.send('accountPickerReady');
On 2017/01/13 19:46:58, jdufault wrote:
> On 2017/01/06 17:14:25, xiyuan wrote:
> > Would we lost this signal? This signal triggers
ScreenLocker::ScreenLockReady
> > that notifies session manager and sets session state.
> > 
> > How would it work in preloaded case?
> 
> Loading the webui doesn't actually initialize of the session specific data. I
> believe before this was only needed if the webui had already been displayed
once
> before. So this change should be ok.
> 
> Tracing through everything, this normally gets sent via
> 
>   JS ScreenAccountPicker.onShow
>   ...
>   JS UserPodRow.handleAfterShow
>   ...
>   JS Oobe.showScreen {"data":{"disableAddUser":false},"id":"account-picker"}
>   #1 chromeos::BaseScreenHandler::ShowScreenWithData()
>   #2 chromeos::SigninScreenHandler::ShowImpl()
>   #3 chromeos::SigninScreenHandler::Show()
>   #4 chromeos::OobeUI::ShowSigninScreen()
>   #5 chromeos::WebUIScreenLocker::LockScreen()
>   #6 chromeos::ScreenLocker::Init()
>   #7 chromeos::ScreenLocker::Show()
>   #8 chromeos::ScreenLocker::HandleLockScreenRequest()
> 
> I'm guessing that one of those functions is doing caching and not calling the
> rest of the sequence if the screen is already set to 'account-picker'.

Acknowledged.

[jdufault, 2017-01-13 20:34:21]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2017-01-13 20:34:31]

The CQ bit was unchecked by jdufault@chromium.org

[jdufault, 2017-01-13 20:34:49]

jdufault@chromium.org changed reviewers:
+ achuith@chromium.org

[jdufault, 2017-01-13 20:35:25]

+achuith@ optionally if you want to take a look.

[achuithb, 2017-01-13 20:57:52]

lgtm

https://codereview.chromium.org/2610373002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/2610373002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.h:49: bool check_for_preload;
should this be initialized to false?

[jdufault, 2017-01-13 22:12:28]

https://codereview.chromium.org/2610373002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.h (right):

https://codereview.chromium.org/2610373002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.h:49: bool check_for_preload;
On 2017/01/13 20:57:52, achuithb wrote:
> should this be initialized to false?

Done.

[jdufault, 2017-01-13 22:12:34]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2017-01-13 22:12:34]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2610373002/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-13 22:13:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 23:34:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484345554421160,
"parent_rev": "67b5583718d2ed35011af6d7468068e3c0237166", "commit_rev":
"284264846b3b7131c5f1efc1482d42e06878dd98"}

[commit-bot: I haz the power, 2017-01-13 23:34:37]

Description was changed from

==========
cros: Only preload the lock screen; do not reuse it.

BUG=669638
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
cros: Only preload the lock screen; do not reuse it.

BUG=669638
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2610373002
Cr-Commit-Position: refs/heads/master@{#443725}
Committed:
https://chromium.googlesource.com/chromium/src/+/284264846b3b7131c5f1efc1482d...
==========

[commit-bot: I haz the power, 2017-01-13 23:34:39]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/284264846b3b7131c5f1efc1482d...