Chromium Code Reviews
Help | Chromium Project | Sign in
(48)

Issue 2697063004: Fix of "login is not defined" error in OOBE

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by Wenzhao (Colin) Zang
Modified:
3 days, 19 hours 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.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Move CallJSOrDefer to OobeUI #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -109 lines) Patch
M chrome/browser/ui/webui/chromeos/login/base_screen_handler.h View 1 2 chunks +17 lines, -17 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h View 1 2 chunks +0 lines, -27 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc View 1 8 chunks +33 lines, -63 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/oobe_ui.h View 1 4 chunks +48 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/chromeos/login/oobe_ui.cc View 1 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc View 1 1 chunk +2 lines, -1 line 2 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 14 (7 generated)
Wenzhao (Colin) Zang
1 week, 1 day ago (2017-02-17 03:35:13 UTC) #3
Alexander Alekseev
https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/chromeos/login/oobe.js File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/chromeos/login/oobe.js#newcode144 chrome/browser/resources/chromeos/login/oobe.js:144: chrome.send('screenStateInitializeForSignin'); We try to avoid duplicating events, as this ...
1 week, 1 day ago (2017-02-17 03:56:38 UTC) #4
Wenzhao (Colin) Zang
https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/chromeos/login/oobe.js File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/chromeos/login/oobe.js#newcode144 chrome/browser/resources/chromeos/login/oobe.js:144: chrome.send('screenStateInitializeForSignin'); In order to replace all CallJS with CallJSOrDefer ...
1 week, 1 day ago (2017-02-17 19:51:56 UTC) #9
Wenzhao (Colin) Zang
https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/chromeos/login/oobe.js File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2697063004/diff/1/chrome/browser/resources/chromeos/login/oobe.js#newcode144 chrome/browser/resources/chromeos/login/oobe.js:144: chrome.send('screenStateInitializeForSignin'); We might need to separate CallJSOrDefer into a ...
1 week ago (2017-02-17 22:58:16 UTC) #10
Wenzhao (Colin) Zang
Move CallJSOrDefer to OobeUI
3 days, 23 hours ago (2017-02-21 23:35:04 UTC) #12
Alexander Alekseev
https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2697063004/diff/20001/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc#newcode1131 chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1131: GetOobeUI()->CallJSOrDefer("login.AccountPickerScreen.setTouchViewState", GetOobeUI() is a member of BaseScreenHandler. This means ...
3 days, 22 hours ago (2017-02-21 23:46:49 UTC) #13
Wenzhao (Colin) Zang
3 days, 19 hours ago (2017-02-22 02:53:34 UTC) #14
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).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd