Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
596613002:
Remove content dependencies from content settings providers. (Closed)
DescriptionRemove content dependencies from content settings providers.
BUG=384876
R=bauerb@chromium.org, markusheintz@chromium.org, vabr@chromium.org, rlp@chromium.org
TEST=no functional changes
Committed: https://crrev.com/55fab14d5efed15eec5d91b536a80607f58800d6
Cr-Commit-Position: refs/heads/master@{#297288}
Patch Set 1 #Patch Set 2 : typo #Patch Set 3 : shutdown fix #
Total comments: 17
Patch Set 4 : addressing comments #Patch Set 5 : ThreadChecker #Patch Set 6 : browser_tests fix #
Total comments: 5
Patch Set 7 : thread checker -> MessageLoopForUI::IsCurrent() #Patch Set 8 : remains browserthread::ui #
Messages
Total messages: 43 (8 generated)
mukai@chromium.org changed reviewers: + markusheintz@chromium.org, vabr@chromium.org - markusheinz@chromium.org
PTAL One a bit controversy thing is replacing DCHECK_CURRENTLY_ON(BrowserThread::UI) by CalledOnValidThread(). Based on the TODO in content_settings_policy_provider_unittest.cc, I believe the current approach is better and cleaner.
blundell@chromium.org changed reviewers: + blundell@chromium.org
lgtm assuming all the tests pass https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.h:50: public base::NonThreadSafe, This is odd. base::NonThreadSafe directly above base::RefCountedThreadSafe?
oops, not lgtm pending the question I had in the review
https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_default_provider.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_default_provider.cc:12: #include "base/bind.h" Why did you include this? (Also in other files.) https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.cc:127: DCHECK(CalledOnValidThread()); Instead of trying to get this functionality from NonThreadSafe, you can include a thread checker as a data member, and check that, exactly as NonThreadSafe does.
https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_policy_provider_unittest.cc (left): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:45: content::TestBrowserThread ui_thread_; There is content::TestBrowserThreadBundle now to replace these two. https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_pref_provider.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_pref_provider.cc:125: DCHECK(CalledOnValidThread()); Hm... I'm not sure if I actually prefer this. PrefProvider is not just a class that can only be used on a single thread, it has to be used on the UI thread, because the PrefService has to be used on the UI thread. https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.h:50: public base::NonThreadSafe, On 2014/09/23 07:27:00, blundell wrote: > This is odd. base::NonThreadSafe directly above base::RefCountedThreadSafe? :D It's actually not that uncommon, it just looks a bit strange: RefCountedThreadSafe doesn't mean that the object itself is thread-safe, just that the refcounting is. There are classes that should only be used on a single thread but have thread-safe refcounting to allow passing them to other threads. In this case though, HostContentSettingsMap can be used on multiple threads, it's just the setter methods that are UI-thread only. So NonThreadSafe would be somewhat misleading. https://codereview.chromium.org/596613002/diff/40001/components/content_setti... File components/content_settings/core/browser/content_settings_provider.h (right): https://codereview.chromium.org/596613002/diff/40001/components/content_setti... components/content_settings/core/browser/content_settings_provider.h:27: class ProviderInterface : public base::NonThreadSafe { This class is supposed to be an empty interface, and deriving from NonThreadSafe (or adding a ThreadChecker member) would change that.
https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_default_provider.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_default_provider.cc:12: #include "base/bind.h" On 2014/09/23 07:51:00, vabr (Chromium) wrote: > Why did you include this? > (Also in other files.) Bind is used below (line 173 in this file). It was implicitly included through browser_thread.h (browser_thread.h -> base/task_runner_util.h -> base/bind.h) https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_policy_provider_unittest.cc (left): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:45: content::TestBrowserThread ui_thread_; On 2014/09/23 10:08:19, Bernhard Bauer wrote: > There is content::TestBrowserThreadBundle now to replace these two. The point of this CL is to remove content/ dependency as far as possible. The motivation is iOS support; if a file depends on content/ layer, it gets unavailable on iOS. If TestBrowserThreadBundle is a good solution if it needs to check BrowserThread, but as far as I see, I don't think it needs. The check was translated as "these methods need to run in the thread the provider was created" (and Profile code verifies it's truly created on BrowserThread::UI). So this threading code can be removed. https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_pref_provider.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_pref_provider.cc:125: DCHECK(CalledOnValidThread()); On 2014/09/23 10:08:19, Bernhard Bauer wrote: > Hm... I'm not sure if I actually prefer this. PrefProvider is not just a class > that can only be used on a single thread, it has to be used on the UI thread, > because the PrefService has to be used on the UI thread. Hmm. As I wrote in another comment, my motivation is to remove BrowserThread things if possible. As far as I see in base/pref_service, PrefService also doesn't care the name of the thread. It simply belongs to the thread where it was created, so I think it would be okay to use the same logic here. If you want to specify the thread name, I think we can use MessageLoopForUI::IsCurrent() instead of BrowserThread::UI. Which do you prefer? https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.cc:127: DCHECK(CalledOnValidThread()); On 2014/09/23 07:51:00, vabr (Chromium) wrote: > Instead of trying to get this functionality from NonThreadSafe, you can include > a thread checker as a data member, and check that, exactly as NonThreadSafe > does. That is an idea. Please see Bauer's comment. https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.h (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.h:50: public base::NonThreadSafe, On 2014/09/23 10:08:19, Bernhard Bauer wrote: > On 2014/09/23 07:27:00, blundell wrote: > > This is odd. base::NonThreadSafe directly above base::RefCountedThreadSafe? > > :D > > It's actually not that uncommon, it just looks a bit strange: > RefCountedThreadSafe doesn't mean that the object itself is thread-safe, just > that the refcounting is. There are classes that should only be used on a single > thread but have thread-safe refcounting to allow passing them to other threads. > > In this case though, HostContentSettingsMap can be used on multiple threads, > it's just the setter methods that are UI-thread only. So NonThreadSafe would be > somewhat misleading. Or simply add ThreadChecker as a field instead (as vabr suggested in .cc file). Which is better? https://codereview.chromium.org/596613002/diff/40001/components/content_setti... File components/content_settings/core/browser/content_settings_provider.h (right): https://codereview.chromium.org/596613002/diff/40001/components/content_setti... components/content_settings/core/browser/content_settings_provider.h:27: class ProviderInterface : public base::NonThreadSafe { On 2014/09/23 10:08:19, Bernhard Bauer wrote: > This class is supposed to be an empty interface, and deriving from NonThreadSafe > (or adding a ThreadChecker member) would change that. Removed
https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_policy_provider_unittest.cc (left): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_policy_provider_unittest.cc:45: content::TestBrowserThread ui_thread_; On 2014/09/23 20:48:44, Jun Mukai wrote: > On 2014/09/23 10:08:19, Bernhard Bauer wrote: > > There is content::TestBrowserThreadBundle now to replace these two. > > The point of this CL is to remove content/ dependency as far as possible. The > motivation is iOS support; if a file depends on content/ layer, it gets > unavailable on iOS. > > If TestBrowserThreadBundle is a good solution if it needs to check > BrowserThread, but as far as I see, I don't think it needs. The check was > translated as "these methods need to run in the thread the provider was created" > (and Profile code verifies it's truly created on BrowserThread::UI). So this > threading code can be removed. Fair enough. Ideally, we'd have something to enforce that this class is used on the same thread as PrefService (independent of which thread that is), but yeah, using ThreadChecker works too. https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.cc:127: DCHECK(CalledOnValidThread()); On 2014/09/23 20:48:44, Jun Mukai wrote: > On 2014/09/23 07:51:00, vabr (Chromium) wrote: > > Instead of trying to get this functionality from NonThreadSafe, you can > include > > a thread checker as a data member, and check that, exactly as NonThreadSafe > > does. > > That is an idea. Please see Bauer's comment. Yeah, that probably makes the most sense. You could put the ThreadChecker into ObservableProvider, which is the base implementation for all the providers.
lgtm pending resolution of others' comments
LGTM, Thanks for your work on this! Vaclav https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/content_settings_default_provider.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/content_settings_default_provider.cc:12: #include "base/bind.h" On 2014/09/23 20:48:44, Jun Mukai wrote: > On 2014/09/23 07:51:00, vabr (Chromium) wrote: > > Why did you include this? > > (Also in other files.) > > Bind is used below (line 173 in this file). It was implicitly included through > browser_thread.h (browser_thread.h -> base/task_runner_util.h -> base/bind.h) Thanks for explanation, makes sense. https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... File chrome/browser/content_settings/host_content_settings_map.cc (right): https://codereview.chromium.org/596613002/diff/40001/chrome/browser/content_s... chrome/browser/content_settings/host_content_settings_map.cc:127: DCHECK(CalledOnValidThread()); On 2014/09/24 08:15:31, Bernhard Bauer wrote: > On 2014/09/23 20:48:44, Jun Mukai wrote: > > On 2014/09/23 07:51:00, vabr (Chromium) wrote: > > > Instead of trying to get this functionality from NonThreadSafe, you can > > include > > > a thread checker as a data member, and check that, exactly as NonThreadSafe > > > does. > > > > That is an idea. Please see Bauer's comment. > > Yeah, that probably makes the most sense. You could put the ThreadChecker into > ObservableProvider, which is the base implementation for all the providers. +1 to putting the ThreadChecker (or NonThreadSafe) into ObservableProvider; but I'm also fine with the current situation
Changed to ThreadChecker approach. PTAL. Note that host_content_settings_unittest.cc needs to have BrowserThreads because of cookie_settings.cc (previously I missed that failure).
LGTM On 2014/09/24 22:35:51, Jun Mukai wrote: > Changed to ThreadChecker approach. PTAL. > > Note that host_content_settings_unittest.cc needs to have BrowserThreads because > of cookie_settings.cc (previously I missed that failure). I guess that CookieSettings will stay in chrome/? In that case (once we move the other classes out) it might make sense to split out the unit tests that use CookieSettings.
On 2014/09/25 08:40:24, Bernhard Bauer wrote: > LGTM > > On 2014/09/24 22:35:51, Jun Mukai wrote: > > Changed to ThreadChecker approach. PTAL. > > > > Note that host_content_settings_unittest.cc needs to have BrowserThreads > because > > of cookie_settings.cc (previously I missed that failure). > > I guess that CookieSettings will stay in chrome/? In that case (once we move the > other classes out) it might make sense to split out the unit tests that use > CookieSettings. I don't see any reason CookieSettings wouldn't be componentized as well. vabr@, did you have thoughts here?
On 2014/09/25 09:08:41, blundell wrote: > On 2014/09/25 08:40:24, Bernhard Bauer wrote: > > LGTM > > > > On 2014/09/24 22:35:51, Jun Mukai wrote: > > > Changed to ThreadChecker approach. PTAL. > > > > > > Note that host_content_settings_unittest.cc needs to have BrowserThreads > > because > > > of cookie_settings.cc (previously I missed that failure). > > > > I guess that CookieSettings will stay in chrome/? In that case (once we move > the > > other classes out) it might make sense to split out the unit tests that use > > CookieSettings. > > I don't see any reason CookieSettings wouldn't be componentized as well. vabr@, > did you have thoughts here? Componentising CookieSettings is currently planned http://crbug.com/387074. I don't see a reason against componentising them, but happy to hear some. Having said that, if moving tests blocks this, let's keep the tests where they are, and revisit that later. Important is moving the production code in the first phase. Cheers, Vaclav
On 2014/09/25 10:46:10, vabr (Chromium) wrote: > On 2014/09/25 09:08:41, blundell wrote: > > On 2014/09/25 08:40:24, Bernhard Bauer wrote: > > > LGTM > > > > > > On 2014/09/24 22:35:51, Jun Mukai wrote: > > > > Changed to ThreadChecker approach. PTAL. > > > > > > > > Note that host_content_settings_unittest.cc needs to have BrowserThreads > > > because > > > > of cookie_settings.cc (previously I missed that failure). > > > > > > I guess that CookieSettings will stay in chrome/? In that case (once we move > > the > > > other classes out) it might make sense to split out the unit tests that use > > > CookieSettings. > > > > I don't see any reason CookieSettings wouldn't be componentized as well. > vabr@, > > did you have thoughts here? > > Componentising CookieSettings is currently planned http://crbug.com/387074. I > don't see a reason against componentising them, but happy to hear some. > > Having said that, if moving tests blocks this, let's keep the tests where they > are, and revisit that later. Important is moving the production code in the > first phase. > > Cheers, > Vaclav I think eventually cookie_settings could be componentized, but I don't think it needs to be at the same time as host_content_settings and/or content_settings provider implementation. And if it really needs to be tied with UI thread, it should move into a different library which can depend on content/. These things are unclear, and that's the reason I left it as is.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/596613002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/09/25 19:30:26, Jun Mukai wrote: > On 2014/09/25 10:46:10, vabr (Chromium) wrote: > > On 2014/09/25 09:08:41, blundell wrote: > > > On 2014/09/25 08:40:24, Bernhard Bauer wrote: > > > > LGTM > > > > > > > > On 2014/09/24 22:35:51, Jun Mukai wrote: > > > > > Changed to ThreadChecker approach. PTAL. > > > > > > > > > > Note that host_content_settings_unittest.cc needs to have BrowserThreads > > > > because > > > > > of cookie_settings.cc (previously I missed that failure). > > > > > > > > I guess that CookieSettings will stay in chrome/? In that case (once we > move > > > the > > > > other classes out) it might make sense to split out the unit tests that > use > > > > CookieSettings. > > > > > > I don't see any reason CookieSettings wouldn't be componentized as well. > > vabr@, > > > did you have thoughts here? > > > > Componentising CookieSettings is currently planned http://crbug.com/387074. I > > don't see a reason against componentising them, but happy to hear some. > > > > Having said that, if moving tests blocks this, let's keep the tests where they > > are, and revisit that later. Important is moving the production code in the > > first phase. > > > > Cheers, > > Vaclav > > I think eventually cookie_settings could be componentized, but I don't think it > needs to be at the same time as host_content_settings and/or content_settings > provider implementation. > And if it really needs to be tied with UI thread, it should move into a > different library which can depend on content/. > These things are unclear, and that's the reason I left it as is. No, I don't think there's a reason why CookieSettings needs to depend on the UI thread. I'm okay with keeping its dependency for now, and componentizing it later.
mukai@chromium.org changed reviewers: + rlp@chromium.org
rlp -- please review for chrome/browser/profiles/
https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_impl.cc:357: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Is there a reason not to have this at the start of the function? Should it ever not be called on the UI thread? https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_impl.cc:1126: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); same question as above
https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_impl.cc:357: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2014/09/26 03:46:34, rpetterson wrote: > Is there a reason not to have this at the start of the function? Should it ever > not be called on the UI thread? The motivation of this CL is to strip off the dependency on content/ from these files. And the content/ dependency is because of iOS support -- iOS cannot depend on content/
https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_impl.cc:357: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2014/09/26 07:45:14, Jun Mukai wrote: > On 2014/09/26 03:46:34, rpetterson wrote: > > Is there a reason not to have this at the start of the function? Should it > ever > > not be called on the UI thread? > > The motivation of this CL is to strip off the dependency on content/ from these > files. And the content/ dependency is because of iOS support -- iOS cannot > depend on content/ I think the question is why this DCHECK shouldn't be before line 355.
https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/off_the_record_profile_impl.cc (right): https://codereview.chromium.org/596613002/diff/100001/chrome/browser/profiles... chrome/browser/profiles/off_the_record_profile_impl.cc:357: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2014/09/26 07:50:41, blundell wrote: > On 2014/09/26 07:45:14, Jun Mukai wrote: > > On 2014/09/26 03:46:34, rpetterson wrote: > > > Is there a reason not to have this at the start of the function? Should it > > ever > > > not be called on the UI thread? > > > > The motivation of this CL is to strip off the dependency on content/ from > these > > files. And the content/ dependency is because of iOS support -- iOS cannot > > depend on content/ > > I think the question is why this DCHECK shouldn't be before line 355. Ah... HostContentSettings is thread-safe object and can be used by ProfileIOData. It's simply initialization process -- but indeed this is weird. If some non-UI thread calls GetHostContentSettingsMap() first, it would lead to an unexpected result. The reason why it is okay on the current code base is because HostContentSettingsMap is required in DoFinalInit (through the call stack of GetSpecialStoragePolicy() -> CookieSettings ctor -> GetHostContentSettings()). But relying on this structure implicitly wouldn't be great. Let me think about this.
considering about the initialization, I realized it's not yet ready to use thread checker correctly to these classes. Since the goal of this CL is remove content/ code path, I've rewritten to use MessageLoopForUI::IsCurrent(). bauerb, vabr, could you take another look? rlp, this CL doesn't change profiles anymore. Sorry for bothering you.
Still LGTM
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/596613002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
LGTM, thanks also for explaining the chosen way. I'm not completely clear on whether the MessageLoopForUI message loop specialisation is only used for the UI thread, but I guess the main purpose of the DCHECKs is to avoid running on the IO thread, which has a different m. l. specialisation. Cheers, Vaclav
On 2014/09/29 07:39:16, vabr (Chromium) wrote: > LGTM, thanks also for explaining the chosen way. > I'm not completely clear on whether the MessageLoopForUI message loop > specialisation is only used for the UI thread, but I guess the main purpose of > the DCHECKs is to avoid running on the IO thread, which has a different m. l. > specialisation. > Cheers, > Vaclav Ugha, the test failures are real, some tests initializes BrowserThread::UI on MessageLoopForIO as a test utility... :-/ I didn't know that. Let me give up, I'll remain the all BrowserThread::UI check for now.
On 2014/09/29 18:58:54, Jun Mukai wrote: > On 2014/09/29 07:39:16, vabr (Chromium) wrote: > > LGTM, thanks also for explaining the chosen way. > > I'm not completely clear on whether the MessageLoopForUI message loop > > specialisation is only used for the UI thread, but I guess the main purpose of > > the DCHECKs is to avoid running on the IO thread, which has a different m. l. > > specialisation. > > Cheers, > > Vaclav > > Ugha, the test failures are real, some tests initializes BrowserThread::UI on > MessageLoopForIO as a test utility... :-/ > I didn't know that. > Let me give up, I'll remain the all BrowserThread::UI check for now. Why did you have to move away from the ThreadChecker approach? I didn't get that. In the worst case, it should work to inject a SingleThreadTaskRunner into the affected classes' constructors and then have them DCHECK that they're running on that task runner, right? That's basically what the code is doing now, just with a convenience function to avoid the need to inject the thread.
On 2014/09/29 19:03:32, blundell wrote: > On 2014/09/29 18:58:54, Jun Mukai wrote: > > On 2014/09/29 07:39:16, vabr (Chromium) wrote: > > > LGTM, thanks also for explaining the chosen way. > > > I'm not completely clear on whether the MessageLoopForUI message loop > > > specialisation is only used for the UI thread, but I guess the main purpose > of > > > the DCHECKs is to avoid running on the IO thread, which has a different m. > l. > > > specialisation. > > > Cheers, > > > Vaclav > > > > Ugha, the test failures are real, some tests initializes BrowserThread::UI on > > MessageLoopForIO as a test utility... :-/ > > I didn't know that. > > Let me give up, I'll remain the all BrowserThread::UI check for now. > > Why did you have to move away from the ThreadChecker approach? I didn't get > that. > > In the worst case, it should work to inject a SingleThreadTaskRunner into the > affected classes' constructors and then have them DCHECK that they're running on > that task runner, right? That's basically what the code is doing now, just with > a convenience function to avoid the need to inject the thread. ThreadChecker approach needs to refine Profile to make sure that the HostContentSettingsMap is truly created in the UI thread (or adding InitializeOnUIThread method). That would change the code a lot. It would be worth doing, but probably better to be done by another CL.
On 2014/09/29 19:58:49, Jun Mukai wrote: > On 2014/09/29 19:03:32, blundell wrote: > > On 2014/09/29 18:58:54, Jun Mukai wrote: > > > On 2014/09/29 07:39:16, vabr (Chromium) wrote: > > > > LGTM, thanks also for explaining the chosen way. > > > > I'm not completely clear on whether the MessageLoopForUI message loop > > > > specialisation is only used for the UI thread, but I guess the main > purpose > > of > > > > the DCHECKs is to avoid running on the IO thread, which has a different m. > > l. > > > > specialisation. > > > > Cheers, > > > > Vaclav > > > > > > Ugha, the test failures are real, some tests initializes BrowserThread::UI > on > > > MessageLoopForIO as a test utility... :-/ > > > I didn't know that. > > > Let me give up, I'll remain the all BrowserThread::UI check for now. > > > > Why did you have to move away from the ThreadChecker approach? I didn't get > > that. > > > > In the worst case, it should work to inject a SingleThreadTaskRunner into the > > affected classes' constructors and then have them DCHECK that they're running > on > > that task runner, right? That's basically what the code is doing now, just > with > > a convenience function to avoid the need to inject the thread. > > ThreadChecker approach needs to refine Profile to make sure that the > HostContentSettingsMap is truly created in the UI thread (or adding > InitializeOnUIThread method). > That would change the code a lot. It would be worth doing, but probably better > to be done by another CL. Ah, makes sense. In that case injecting the UI thread seems like it would work?
On 2014/09/29 20:00:45, blundell wrote: > On 2014/09/29 19:58:49, Jun Mukai wrote: > > On 2014/09/29 19:03:32, blundell wrote: > > > On 2014/09/29 18:58:54, Jun Mukai wrote: > > > > On 2014/09/29 07:39:16, vabr (Chromium) wrote: > > > > > LGTM, thanks also for explaining the chosen way. > > > > > I'm not completely clear on whether the MessageLoopForUI message loop > > > > > specialisation is only used for the UI thread, but I guess the main > > purpose > > > of > > > > > the DCHECKs is to avoid running on the IO thread, which has a different > m. > > > l. > > > > > specialisation. > > > > > Cheers, > > > > > Vaclav > > > > > > > > Ugha, the test failures are real, some tests initializes BrowserThread::UI > > on > > > > MessageLoopForIO as a test utility... :-/ > > > > I didn't know that. > > > > Let me give up, I'll remain the all BrowserThread::UI check for now. > > > > > > Why did you have to move away from the ThreadChecker approach? I didn't get > > > that. > > > > > > In the worst case, it should work to inject a SingleThreadTaskRunner into > the > > > affected classes' constructors and then have them DCHECK that they're > running > > on > > > that task runner, right? That's basically what the code is doing now, just > > with > > > a convenience function to avoid the need to inject the thread. > > > > ThreadChecker approach needs to refine Profile to make sure that the > > HostContentSettingsMap is truly created in the UI thread (or adding > > InitializeOnUIThread method). > > That would change the code a lot. It would be worth doing, but probably better > > to be done by another CL. > > Ah, makes sense. In that case injecting the UI thread seems like it would work? haven't decided the actual design, but that would be a choice.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/596613002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as eb0ec865eb01c0bd9450dd17b3f2366e24cd5d43
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/55fab14d5efed15eec5d91b536a80607f58800d6 Cr-Commit-Position: refs/heads/master@{#297288} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
