Clean up threading in HTTP auth UI logic |
||
Issue descriptionIt's high time we did this. My plan is: 1. Outside of content, split all the CreateLoginDelegate implementations into a purely UI thread part and an IO thread proxy. Along the way, make LoginHandlerViews::DeleteDelegate simply "delete this" to cut down on another place where DeleteDelegate is needed. 2. Wait for the fix to issue #901809 to land. 3. Push the IO thread proxy into content and remove the thread-hopping from the net svc side of the world.
,
Dec 1
Stared at code and took a stab at (1), but the hairy ownership story makes it hard to reason about whether I'm adding reference cycles because content::LoginDelegate's interface itself is ref-counted. (Or if those reference cycles are problems! There are some intentional ones in here.) New thoughts on how to cut the knot:
1. Move the bulk of the logic in LoginHandler{,Views,Android} out into a uniquely-owned single-threaded class. This'll leave LoginHandler itself a refcounted IO/UI proxy. Refine android_webview and content/shell implementations to this shape as well (though they're much closer to this already).
2. Switch content::LoginDelegate to be uniquely-owned. OnRequestCanceled then even melts into the destructor, which is nice.
3. At this point, every implementation of LoginDelegate is really just a silly IO-thread proxy. Pull that inside content. The content/browser/loader implementation will just go through that proxy, while the network service path can do it straight from the UI thread. (Though the DevTools interception bits will still be on the IO thread, unless trivially movable, so we aren't completely saved from threading.)
,
Jan 4
The second plan seems to be a winner! https://chromium-review.googlesource.com/c/chromium/src/+/1388033/ https://chromium-review.googlesource.com/c/chromium/src/+/1388037/ https://chromium-review.googlesource.com/c/chromium/src/+/1388039/ https://chromium-review.googlesource.com/c/chromium/src/+/1388040/ https://chromium-review.googlesource.com/c/chromium/src/+/1388042/ https://chromium-review.googlesource.com/c/chromium/src/+/1388164/
,
Jan 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5252711b85ce30077832fc55ed52106c7b12cfee commit 5252711b85ce30077832fc55ed52106c7b12cfee Author: David Benjamin <davidben@chromium.org> Date: Wed Jan 16 01:32:52 2019 Push LoginModel observer code down to the dialog implementations. LoginHandler has generic code to manage LoginModel observing for the subclass, but only LoginHandlerAndroid uses it. LoginHandlerViews defers to LoginView. Mirror that in ChromeHttpAuthHandler to align the two paths. (LoginHandlerAndroid forwards the signal to ChromeHttpAuthHandler anyway, so the common code isn't really saving much.) The goal here is to simplify interactions between LoginHandler and its subclasses, along the way to cleaning up the ownership. Bug: 908926 Change-Id: If44e76f452966d2767022770ee3ab91d020d5f6c Reviewed-on: https://chromium-review.googlesource.com/c/1388033 Reviewed-by: Carlos IL <carlosil@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: David Benjamin <davidben@chromium.org> Cr-Commit-Position: refs/heads/master@{#622982} [modify] https://crrev.com/5252711b85ce30077832fc55ed52106c7b12cfee/chrome/browser/ui/android/chrome_http_auth_handler.cc [modify] https://crrev.com/5252711b85ce30077832fc55ed52106c7b12cfee/chrome/browser/ui/android/chrome_http_auth_handler.h [modify] https://crrev.com/5252711b85ce30077832fc55ed52106c7b12cfee/chrome/browser/ui/android/login_handler_android.cc [modify] https://crrev.com/5252711b85ce30077832fc55ed52106c7b12cfee/chrome/browser/ui/login/login_handler.cc [modify] https://crrev.com/5252711b85ce30077832fc55ed52106c7b12cfee/chrome/browser/ui/login/login_handler.h [modify] https://crrev.com/5252711b85ce30077832fc55ed52106c7b12cfee/chrome/browser/ui/views/login_handler_views.cc |
||
►
Sign in to add a comment |
||
Comment 1 by davidben@chromium.org
, Nov 27