Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2928513004:
cros: Do not initialize gaia portal detector if not loading gaia (Closed)
|
Created:
3 years, 6 months ago by Qiang(Joe) Xu Modified:
3 years, 6 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Do not initialize gaia portal detector if not loading gaia
Changes:
Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should
not load gaia, so that portal detector will not get initialized and
enabled in this case.
BUG=730157
TEST=tested that gaia screen portal detector is not created and used in
user session.
Review-Url: https://codereview.chromium.org/2928513004
Cr-Commit-Position: refs/heads/master@{#477489}
Committed: https://chromium.googlesource.com/chromium/src/+/3d4f038889267bfd326967c840b22166b567c595
Patch Set 1 #
Total comments: 4
Patch Set 2 : feedback #
Total comments: 2
Patch Set 3 : comment #Messages
Total messages: 24 (13 generated)
Description was changed from ========== cros: Do not initialize gaia screen portal detector in user session Changes: Do not allow portal detector which is used for gaia to get initialized and enabled in user session. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ========== to ========== cros: Do not initialize gaia screen portal detector in user session Changes: Do not allow portal detector which is used for gaia to get initialized and enabled in user session. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ==========
warx@chromium.org changed reviewers: + jdufault@chromium.org, xiyuan@chromium.org
The CQ bit was checked by warx@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...
Hi all, PTAL thanks! This might not be the right fix. Is that right that MaybePreloadAuthExtension gets called when logged in? Do we need to pop the added check before if-condition?
On 2017/06/06 21:44:00, Qiang(Joe) Xu wrote: > Hi all, PTAL thanks! > > This might not be the right fix. Is that right that MaybePreloadAuthExtension > gets called when logged in? Do we need to pop the added check before > if-condition? MaybePreloadAuthExtension() should not be called when user has signed in. It is for Gaia and we don't support Gaia sign-in when user has signed in. That is, no chromeos user creation from within a user session. Gaia should not be used in lock screen or user adding screen.
https://codereview.chromium.org/2928513004/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/2928513004/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:247: !user_manager::UserManager::Get()->IsUserLoggedIn()) { Maybe move ShouldLoadGaia() before this and bail out early if it returns false ?
https://codereview.chromium.org/2928513004/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/2928513004/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:247: !user_manager::UserManager::Get()->IsUserLoggedIn()) { Add a comment explaining why we need this special case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== cros: Do not initialize gaia screen portal detector in user session Changes: Do not allow portal detector which is used for gaia to get initialized and enabled in user session. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ========== to ========== cros: Do not initialize gaia portal detector if not loading gaia Changes: Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should not load gaia, so that portal detector will be get initialized and enabled in this case. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ==========
PTAL thanks https://codereview.chromium.org/2928513004/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/2928513004/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:247: !user_manager::UserManager::Get()->IsUserLoggedIn()) { On 2017/06/06 21:54:29, xiyuan wrote: > Maybe move ShouldLoadGaia() before this and bail out early if it returns false ? Done. https://codereview.chromium.org/2928513004/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:247: !user_manager::UserManager::Get()->IsUserLoggedIn()) { On 2017/06/06 21:58:37, jdufault wrote: > Add a comment explaining why we need this special case. Done.
lgtm
Description was changed from ========== cros: Do not initialize gaia portal detector if not loading gaia Changes: Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should not load gaia, so that portal detector will be get initialized and enabled in this case. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ========== to ========== cros: Do not initialize gaia portal detector if not loading gaia Changes: Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should not load gaia, so that portal detector will not get initialized and enabled in this case. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ==========
On 2017/06/06 22:51:54, xiyuan wrote: > lgtm Please wrap CL description lines, ideally around char 75.
Description was changed from ========== cros: Do not initialize gaia portal detector if not loading gaia Changes: Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should not load gaia, so that portal detector will not get initialized and enabled in this case. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ========== to ========== cros: Do not initialize gaia portal detector if not loading gaia Changes: Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should not load gaia, so that portal detector will not get initialized and enabled in this case. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ==========
lgtm https://codereview.chromium.org/2928513004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/2928513004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:245: // polls captive portal checking URL if we don't need to load gaia. Please add a link to go/bad-portal here. // Don't create network portal detector if we don't need to load gaia. See go/bad-portal for more context.
https://codereview.chromium.org/2928513004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/2928513004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:245: // polls captive portal checking URL if we don't need to load gaia. On 2017/06/06 23:23:00, jdufault wrote: > Please add a link to go/bad-portal here. > > // Don't create network portal detector if we don't need to load gaia. See > go/bad-portal for more context. Done.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2928513004/#ps40001 (title: "comment")
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": 40001, "attempt_start_ts": 1496792817239670,
"parent_rev": "f679fa4452d67336adf21057e72205f491b70f61", "commit_rev":
"3d4f038889267bfd326967c840b22166b567c595"}
Message was sent while issue was closed.
Description was changed from ========== cros: Do not initialize gaia portal detector if not loading gaia Changes: Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should not load gaia, so that portal detector will not get initialized and enabled in this case. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. ========== to ========== cros: Do not initialize gaia portal detector if not loading gaia Changes: Early bail out in GaiaScreenHandler::MaybePreloadAuthExtension if should not load gaia, so that portal detector will not get initialized and enabled in this case. BUG=730157 TEST=tested that gaia screen portal detector is not created and used in user session. Review-Url: https://codereview.chromium.org/2928513004 Cr-Commit-Position: refs/heads/master@{#477489} Committed: https://chromium.googlesource.com/chromium/src/+/3d4f038889267bfd326967c840b2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3d4f038889267bfd326967c840b2... |
