Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697063004:
Fix of "login is not defined" error in OOBE (Closed)
|
Created:
3 years, 10 months ago by Wenzhao (Colin) Zang Modified:
3 years, 9 months ago Reviewers:
Alexander Alekseev CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, Rahul Chaturvedi Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix of "login is not defined" error in OOBE
The cause of the error is that by the time OnTouchViewToggled in SigninScreenHandler is called, oobe.js may not have completed intialization yet, so we should wait until the JS message is received.
BUG=692751
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2697063004
Cr-Commit-Position: refs/heads/master@{#454111}
Committed: https://chromium.googlesource.com/chromium/src/+/3dc4062eddbbe88c416a21bd92986645ddb43b50
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move CallJSOrDefer to OobeUI #
Total comments: 2
Patch Set 3 : Move deferred JS calls storage to a separate class #Patch Set 4 : Merge #Messages
Total messages: 31 (15 generated)
Description was changed from ========== Fix of "login is not defined" error in OOBE The cause of the error is that by the time OnTouchViewToggled in SigninScreenHandler is called, oobe.js may not have completed intialization yet, so we should wait until the JS message is received. Ideally all the CallJS in SigninScreenHandler should be replaced by CallJSOrDefer but IMHO it's an overkill, so for now a simple fix is used. The fix works on the condition that either OnTouchViewToggled is called only once or subsequent calls completely overwrite the states of previous calls. Otherwise we have to use CallJSOrDefer similar in CoreOobeHandler. BUG=692751 ========== to ========== Fix of "login is not defined" error in OOBE The cause of the error is that by the time OnTouchViewToggled in SigninScreenHandler is called, oobe.js may not have completed intialization yet, so we should wait until the JS message is received. Ideally all the CallJS in SigninScreenHandler should be replaced by CallJSOrDefer but IMHO it's an overkill, so for now a simple fix is used. The fix works on the condition that either OnTouchViewToggled is called only once or subsequent calls completely overwrite the states of previous calls. Otherwise we have to use CallJSOrDefer similar in CoreOobeHandler. BUG=692751 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
wzang@chromium.org changed reviewers: + alemate@chromium.org
https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/oobe.js:144: chrome.send('screenStateInitializeForSignin'); We try to avoid duplicating events, as this makes flow complicate. This is also slow, as this will have a lot of transformations before Chrome will call a simple handler for the event. This way, I think that replacing calls with CallJSOrDefer is going to be simpler and faster. All the allocations and copying will happen on one thread and will likely be faster as there will be no context switching.
The CQ bit was checked by alemate@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/oobe.js:144: chrome.send('screenStateInitializeForSignin'); In order to replace all CallJS with CallJSOrDefer in signin_screen_handler.cc, we still need to be able to get the message from oobe.js that the initialization is complete. It's a different class than CoreOobeHandler and can they two share the same event? In addition, I think some of the function calls in signin_screen_handler.cc has nothing to do with the OOBE process, so they are not supposed to be waiting for the initialization completion signal. Even for OnTouchViewToggled, if we change it to CallJSOrDefer, it actually forces all subsequent calls to this function to wait for an 'initialized' signal as well, which it may never get? So fixing this bug will probably produce more bugs? Please correct me if I'm wrong,
https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/oobe.js:144: chrome.send('screenStateInitializeForSignin'); We might need to separate CallJSOrDefer into a new class because it is a private function in CoreOobeHandler and the deferred JS calls are all stored inside the CoreOobeHandler object. I think the other way is to move everything related to CallJSOrDefer to the base class of CoreOobeHandler (CoreOobeActor), because SigninScreenHandler contains a pointer of the CoreOobeActor type, instead of CoreOobeHandler itself.
Description was changed from ========== Fix of "login is not defined" error in OOBE The cause of the error is that by the time OnTouchViewToggled in SigninScreenHandler is called, oobe.js may not have completed intialization yet, so we should wait until the JS message is received. Ideally all the CallJS in SigninScreenHandler should be replaced by CallJSOrDefer but IMHO it's an overkill, so for now a simple fix is used. The fix works on the condition that either OnTouchViewToggled is called only once or subsequent calls completely overwrite the states of previous calls. Otherwise we have to use CallJSOrDefer similar in CoreOobeHandler. BUG=692751 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix of "login is not defined" error in OOBE The cause of the error is that by the time OnTouchViewToggled in SigninScreenHandler is called, oobe.js may not have completed intialization yet, so we should wait until the JS message is received. BUG=692751 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Move CallJSOrDefer to OobeUI
https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1131: GetOobeUI()->CallJSOrDefer("login.AccountPickerScreen.setTouchViewState", GetOobeUI() is a member of BaseScreenHandler. This means that we can rename current OobeUI::CallJSOrDefer() to OobeUI::CallJSOrDeferImpl(), and implement BaseScreenHandler::CallJSOrDefer() that will be drop-down replacement for previous CallJSOrDefer() . WDYT?
https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1131: GetOobeUI()->CallJSOrDefer("login.AccountPickerScreen.setTouchViewState", On 2017/02/21 23:46:49, Alexander Alekseev wrote: > GetOobeUI() is a member of BaseScreenHandler. > > This means that we can rename current OobeUI::CallJSOrDefer() to > OobeUI::CallJSOrDeferImpl(), and implement BaseScreenHandler::CallJSOrDefer() > that will be drop-down replacement for previous CallJSOrDefer() . > > WDYT? If BaseScreenHandler calls CallJSOrDeferImpl() in OobeUI, which then calls CallJS() in BaseScreenHandler, we will have a circular dependency between the two classes. Maybe we should move CallJS() to OobeUI too (but that requires changing all the calls to OobeUI::CallJS()), or put all the relevant pieces of CallJS() to a separate file, and add wrappers around it in BaseScreenHandler and OobeUI respectively (But it will again result in lots of codes because all the templates need to be repeated).
On 2017/02/22 02:53:34, Wenzhao (Colin) Zang wrote: > https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): > > https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1131: > GetOobeUI()->CallJSOrDefer("login.AccountPickerScreen.setTouchViewState", > On 2017/02/21 23:46:49, Alexander Alekseev wrote: > > GetOobeUI() is a member of BaseScreenHandler. > > > > This means that we can rename current OobeUI::CallJSOrDefer() to > > OobeUI::CallJSOrDeferImpl(), and implement BaseScreenHandler::CallJSOrDefer() > > that will be drop-down replacement for previous CallJSOrDefer() . > > > > WDYT? > > If BaseScreenHandler calls CallJSOrDeferImpl() in OobeUI, which then calls > CallJS() in BaseScreenHandler, we will have a circular dependency between the > two classes. > > Maybe we should move CallJS() to OobeUI too (but that requires changing all the > calls to OobeUI::CallJS()), or put all the relevant pieces of CallJS() to a > separate file, and add wrappers around it in BaseScreenHandler and OobeUI > respectively (But it will again result in lots of codes because all the > templates need to be repeated). Sorry for the delayed answer. We can also split CallJS to CallJS + OobeUI::CallJSImpl .
I put the JS calls storage and the is_initialized bool to a separate helper class. I think we should keep CallJS implementation in where it is because CallJS uses web_ui() which reside in the base class of BaseScreenHandler. Then there will always be a circular dependency if we move the CallJSOrDefer implementation to OobeUI or any other class. Therefore, it's better if all the actual implementation remains in the same place, and the other class is only responsible for storing the js calls and the bool variable.
lgtm Thank you!
The CQ bit was checked by wzang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:55
error: chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc: patch does
not apply
Patch: chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
Index: chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
diff --git a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
index
0d6cac5715fbffa43fa2b56f62158b4182e23940..8dcd847a4e7f5f29da23f7acf1cd7bc7628072d0
100644
--- a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
+++ b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
@@ -55,8 +55,12 @@ namespace chromeos {
// Note that show_oobe_ui_ defaults to false because WizardController assumes
// OOBE UI is not visible by default.
-CoreOobeHandler::CoreOobeHandler(OobeUI* oobe_ui)
- : oobe_ui_(oobe_ui), version_info_updater_(this) {
+CoreOobeHandler::CoreOobeHandler(OobeUI* oobe_ui,
+ JSCallsContainer* js_calls_container)
+ : BaseScreenHandler(js_calls_container),
+ oobe_ui_(oobe_ui),
+ version_info_updater_(this) {
+ DCHECK(js_calls_container);
set_call_js_prefix(kJsScreenPath);
if (!chrome::IsRunningInMash()) {
AccessibilityManager* accessibility_manager = AccessibilityManager::Get();
@@ -164,37 +168,6 @@ void CoreOobeHandler::RegisterMessages() {
&CoreOobeHandler::HandleSetOobeBootstrappingSlave);
}
-template <typename... Args>
-void CoreOobeHandler::ExecuteDeferredJSCall(const std::string& function_name,
- std::unique_ptr<Args>... args) {
- CallJS(function_name, *args...);
-}
-
-template <typename... Args>
-void CoreOobeHandler::CallJSOrDefer(const std::string& function_name,
- const Args&... args) {
- if (is_initialized_) {
- CallJS(function_name, args...);
- } else {
- // Note that std::conditional is used here in order to obtain a sequence of
- // base::Value types with the length equal to sizeof...(Args); the C++
- // template parameter pack expansion rules require that the name of the
- // parameter pack appears in the pattern, even though the elements of the
- // Args pack are not actually in this code.
- deferred_js_calls_.push_back(base::Bind(
- &CoreOobeHandler::ExecuteDeferredJSCall<
- typename std::conditional<true, base::Value, Args>::type...>,
- base::Unretained(this), function_name,
- base::Passed(::login::MakeValue(args).CreateDeepCopy())...));
- }
-}
-
-void CoreOobeHandler::ExecuteDeferredJSCalls() {
- for (const auto& deferred_js_call : deferred_js_calls_)
- deferred_js_call.Run();
- deferred_js_calls_.clear();
-}
-
void CoreOobeHandler::ShowSignInError(
int login_attempts,
const std::string& error_text,
@@ -295,8 +268,6 @@ void CoreOobeHandler::SetClientAreaSize(int width, int
height) {
}
void CoreOobeHandler::HandleInitialized() {
- DCHECK(!is_initialized_);
- is_initialized_ = true;
ExecuteDeferredJSCalls();
oobe_ui_->InitializeHandlers();
}
The CQ bit was checked by wzang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:55
error: chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc: patch does
not apply
Patch: chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
Index: chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
diff --git a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
index
0d6cac5715fbffa43fa2b56f62158b4182e23940..8dcd847a4e7f5f29da23f7acf1cd7bc7628072d0
100644
--- a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
+++ b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
@@ -55,8 +55,12 @@ namespace chromeos {
// Note that show_oobe_ui_ defaults to false because WizardController assumes
// OOBE UI is not visible by default.
-CoreOobeHandler::CoreOobeHandler(OobeUI* oobe_ui)
- : oobe_ui_(oobe_ui), version_info_updater_(this) {
+CoreOobeHandler::CoreOobeHandler(OobeUI* oobe_ui,
+ JSCallsContainer* js_calls_container)
+ : BaseScreenHandler(js_calls_container),
+ oobe_ui_(oobe_ui),
+ version_info_updater_(this) {
+ DCHECK(js_calls_container);
set_call_js_prefix(kJsScreenPath);
if (!chrome::IsRunningInMash()) {
AccessibilityManager* accessibility_manager = AccessibilityManager::Get();
@@ -164,37 +168,6 @@ void CoreOobeHandler::RegisterMessages() {
&CoreOobeHandler::HandleSetOobeBootstrappingSlave);
}
-template <typename... Args>
-void CoreOobeHandler::ExecuteDeferredJSCall(const std::string& function_name,
- std::unique_ptr<Args>... args) {
- CallJS(function_name, *args...);
-}
-
-template <typename... Args>
-void CoreOobeHandler::CallJSOrDefer(const std::string& function_name,
- const Args&... args) {
- if (is_initialized_) {
- CallJS(function_name, args...);
- } else {
- // Note that std::conditional is used here in order to obtain a sequence of
- // base::Value types with the length equal to sizeof...(Args); the C++
- // template parameter pack expansion rules require that the name of the
- // parameter pack appears in the pattern, even though the elements of the
- // Args pack are not actually in this code.
- deferred_js_calls_.push_back(base::Bind(
- &CoreOobeHandler::ExecuteDeferredJSCall<
- typename std::conditional<true, base::Value, Args>::type...>,
- base::Unretained(this), function_name,
- base::Passed(::login::MakeValue(args).CreateDeepCopy())...));
- }
-}
-
-void CoreOobeHandler::ExecuteDeferredJSCalls() {
- for (const auto& deferred_js_call : deferred_js_calls_)
- deferred_js_call.Run();
- deferred_js_calls_.clear();
-}
-
void CoreOobeHandler::ShowSignInError(
int login_attempts,
const std::string& error_text,
@@ -295,8 +268,6 @@ void CoreOobeHandler::SetClientAreaSize(int width, int
height) {
}
void CoreOobeHandler::HandleInitialized() {
- DCHECK(!is_initialized_);
- is_initialized_ = true;
ExecuteDeferredJSCalls();
oobe_ui_->InitializeHandlers();
}
The CQ bit was checked by wzang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org Link to the patchset: https://codereview.chromium.org/2697063004/#ps60001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488411383886130,
"parent_rev": "45fac28e06443491e5764ef44f3136dbcd0efb54", "commit_rev":
"3dc4062eddbbe88c416a21bd92986645ddb43b50"}
Message was sent while issue was closed.
Description was changed from ========== Fix of "login is not defined" error in OOBE The cause of the error is that by the time OnTouchViewToggled in SigninScreenHandler is called, oobe.js may not have completed intialization yet, so we should wait until the JS message is received. BUG=692751 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix of "login is not defined" error in OOBE The cause of the error is that by the time OnTouchViewToggled in SigninScreenHandler is called, oobe.js may not have completed intialization yet, so we should wait until the JS message is received. BUG=692751 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2697063004 Cr-Commit-Position: refs/heads/master@{#454111} Committed: https://chromium.googlesource.com/chromium/src/+/3dc4062eddbbe88c416a21bd9298... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3dc4062eddbbe88c416a21bd9298... |
