Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(170)

Issue 173193005: Revert 251503 "Implement inline signin with iframe" (Closed)

Created:
6 years, 10 months ago by guohui
Modified:
6 years, 10 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Revert 251503 "Implement inline signin with iframe" > Implement inline signin with iframe > > =+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+= > This is a dup of https://codereview.chromium.org/130963006/ since I cannot > upload to that issue. The only change is to address Xiyuan's two comments > in patchset 3 of that CL. > =+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+= > > Inline signin chrome://chrome-signin is currently implemented using webview embedded in webUI, which breaks a couple of features in webUI and has serious accessbility issues. Since webview will be reimplemented based on OOPIF in the near future, and all the issues we have today will no longer apply, thus it is not worth the effort to fix them as they are throw away work. Instead, as suggested by John and prototyped in https://codereview.chromium.org/141363006/, we decide to switch to iframe instead. A few issues worth to mention, > > 1. The iframe shares the same renderer as the embedder webUI, and thus could be potentially exposed to dangerous webUI privileges. John suggested a fix by assigning a unique storage partition ID to the inline signin page. As a result the inline signin and its embedded web content should never share the same renderer with other webUI pages. > > 2. webview provides a direct API to inject script and to monitor requests/responses, which is not (directly) available with iframe. The CL works around the issue using content script and background script, quite similar to what CrOS is doing for SAML flow today. Thus it is also the first step towards unifying SAML flows on CrOS and desktop. > > 3. with webview approach, we used to have a unique temporary partition for each instance of inline signin, in order to make sure multiple instances do not interfere with each other. This is more difficult with the iframe approach, since the partition ID is hardcoded in a quite low layer. In this CL, all inline signin pages share the same persistent partition, which means we have to handle the case when user loads the sign in page with a dirty cookie jar, and thus the newly connected user may not be stored in the primary session. The CL solves the issue by reading 'session_index' from 'google-accounts-signin' header. > > BUG=338127 > > Review URL: https://codereview.chromium.org/134263005 TBR=rogerta@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252159

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+236 lines, -537 lines) Patch
M trunk/src/chrome/browser/browser_resources.grd View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/chrome/browser/chrome_content_browser_client.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M trunk/src/chrome/browser/extensions/signin/gaia_auth_extension_loader.cc View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc View 2 chunks +0 lines, -9 lines 0 comments Download
M trunk/src/chrome/browser/resources/component_extension_resources.grd View 1 chunk +2 lines, -1 line 0 comments Download
M trunk/src/chrome/browser/resources/gaia_auth/background.js View 5 chunks +19 lines, -136 lines 0 comments Download
D trunk/src/chrome/browser/resources/gaia_auth/desktop_injected.js View 1 chunk +0 lines, -38 lines 0 comments Download
A + trunk/src/chrome/browser/resources/gaia_auth/inline_injected.js View 0 chunks +-1 lines, --1 lines 0 comments Download
A + trunk/src/chrome/browser/resources/gaia_auth/inline_main.html View 0 chunks +-1 lines, --1 lines 0 comments Download
M trunk/src/chrome/browser/resources/gaia_auth/main.js View 7 chunks +124 lines, -81 lines 0 comments Download
D trunk/src/chrome/browser/resources/gaia_auth/manifest_desktop.json View 1 chunk +0 lines, -32 lines 0 comments Download
A + trunk/src/chrome/browser/resources/gaia_auth/manifest_inline.json View 0 chunks +-1 lines, --1 lines 0 comments Download
M trunk/src/chrome/browser/resources/gaia_auth_host/gaia_auth_host.js View 9 chunks +57 lines, -10 lines 0 comments Download
M trunk/src/chrome/browser/resources/inline_login/inline_login.js View 1 chunk +4 lines, -1 line 0 comments Download
M trunk/src/chrome/browser/ui/webui/signin/inline_login_handler.h View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/chrome/browser/ui/webui/signin/inline_login_handler_impl.h View 1 chunk +2 lines, -1 line 0 comments Download
M trunk/src/chrome/browser/ui/webui/signin/inline_login_handler_impl.cc View 11 chunks +19 lines, -12 lines 0 comments Download
M trunk/src/chrome/browser/ui/webui/signin/inline_login_ui.cc View 3 chunks +1 line, -4 lines 0 comments Download
D trunk/src/chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc View 1 chunk +0 lines, -163 lines 0 comments Download
M trunk/src/chrome/chrome_tests.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M trunk/src/content/browser/webui/url_data_manager_backend.cc View 4 chunks +8 lines, -44 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
guohui
6 years, 10 months ago (2014-02-20 02:14:29 UTC) #1
guohui
6 years, 10 months ago (2014-02-20 02:15:08 UTC) #2
Message was sent while issue was closed.
Committed patchset #1 manually as r252159.

Powered by Google App Engine
This is Rietveld 408576698