Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2298073003:
Better access to offline login flow from user pods (Closed)
|
Created:
4 years, 3 months ago by Greg Levin Modified:
4 years, 3 months ago Reviewers:
Alexander Alekseev CC:
chromium-reviews, alemate+watch_chromium.org, jchaffraix+rendering, zoltan1, eae+blinkwatch, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, achuith+watch_chromium.org, leviw+renderwatch, szager+layoutwatch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, blink-reviews, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBetter access to offline login flow from user pods
BUG=641001
TEST=With device off-line, click Add person, check that "sign in as an
existing user" link is at the bottom of the "Network not available"
page, and sign in using this link.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a2b7b5a5d5bde185d6c1f803919ed551676ba6b6
Cr-Commit-Position: refs/heads/master@{#416295}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Cleaned up for review #
Messages
Total messages: 20 (10 generated)
Description was changed from ========== Better access to offline login flow from user pods (first draft) BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. ========== to ========== Better access to offline login flow from user pods (first draft) BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Better access to offline login flow from user pods (first draft) BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Better access to offline login flow from user pods (first draft) BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Better access to offline login flow from user pods (first draft) BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Better access to offline login flow from user pods (first draft) BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Better access to offline login flow from user pods (first draft) BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Better access to offline login flow from user pods BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
glevin@chromium.org changed reviewers: + alemate@chromium.org
Hey alemate@. Here's a preliminary draft for this issue. Could you have a quick look at the changes in screen_error_message.css and signin_screen_handler.cc, and see if this looks like the right approach? This CL works as intended, except for the DCHECK in CompositedLayerMapping.cpp, which I'm looking into. Do you know *why* the "sign in as an existing user" link was originally excluded from the No Network dialog when user pods are present? https://codereview.chromium.org/2298073003/diff/1/chrome/browser/chromeos/set... File chrome/browser/chromeos/settings/cros_settings.cc (right): https://codereview.chromium.org/2298073003/diff/1/chrome/browser/chromeos/set... chrome/browser/chromeos/settings/cros_settings.cc:170: bool* bool_value) const { *** Ignore this, it's only here for debugging purposes. https://codereview.chromium.org/2298073003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/screen_error_message.css (left): https://codereview.chromium.org/2298073003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/screen_error_message.css:22: .offline-login, These changes cause "sign in as an existing user" link to appear at the bottom of the "Network not available". Is this the *right* way to do this? https://codereview.chromium.org/2298073003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2298073003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:661: // DCHECK(layers[i].paintLayer->transformAncestor() == commonTransformAncestor); I don't intend to change this code. I'm just making a note here because this is DCHECKing about a second after the offline login flow switches to the password page. I'm investigating the cause.
Two notes: 1. The DCHECK in CompositedLayerMapping.cpp happens even without my changes, so it's not a problem for this CL. I'm working with the DCHECK's author to resolve it. 2. Not allowing offline login when user pods are present was a deliberate decision: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/chromeos/login/s... but I don't know why (the behavior seems to predate the CL that added this function). Any idea how we could determine why this was done originally?
alemate@ - Here's a better version of this fix, without extraneous debugging code. Please have a look!
lgtm
On 2016/09/01 16:07:51, Greg Levin wrote: > alemate@ - Here's a better version of this fix, without extraneous debugging > code. > > Please have a look! Sorry for the delay, I missed this CL. I also didn't find any evidence why it was done this way. Let's just fix this.
The CQ bit was checked by glevin@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
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by glevin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Better access to offline login flow from user pods BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Better access to offline login flow from user pods BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Better access to offline login flow from user pods BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Better access to offline login flow from user pods BUG=641001 TEST=With device off-line, click Add person, check that "sign in as an existing user" link is at the bottom of the "Network not available" page, and sign in using this link. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/a2b7b5a5d5bde185d6c1f803919ed551676ba6b6 Cr-Commit-Position: refs/heads/master@{#416295} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a2b7b5a5d5bde185d6c1f803919ed551676ba6b6 Cr-Commit-Position: refs/heads/master@{#416295} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
