Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8574020:
Fix GViewRequestInterceptorTest on CrOS. (Closed)
|
Created:
9 years, 1 month ago by Robert Sesek Modified:
9 years, 1 month ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix GViewRequestInterceptorTest on CrOS.
BUG=chromium-os:22447
TEST=GViewRequestInterceptorTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110325
Patch Set 1 #
Total comments: 7
Patch Set 2 : Allow NULL in ChromePluginServiceFilter #
Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:114: PluginService::GetInstance()->SetPluginListForTesting(&plugin_list_); What is going to happen when |plugin_list_| is destroyed? http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:193: TestingProfile profile_; Why do we need to set up a TestingProfile now instead of a TestingPrefService? I'd prefer for PluginPrefs not to require a profile, so if it does, we should fix that :)
http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:114: PluginService::GetInstance()->SetPluginListForTesting(&plugin_list_); On 2011/11/15 23:00:20, Bernhard Bauer wrote: > What is going to happen when |plugin_list_| is destroyed? ShadowingAtExitManager will delete the PluginService. http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:193: TestingProfile profile_; On 2011/11/15 23:00:20, Bernhard Bauer wrote: > Why do we need to set up a TestingProfile now instead of a TestingPrefService? > I'd prefer for PluginPrefs not to require a profile, so if it does, we should > fix that :) Because the NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED gets posted and in the plugin filter, the Source is NULL which causes a deref crash.
http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:114: PluginService::GetInstance()->SetPluginListForTesting(&plugin_list_); On 2011/11/15 23:02:02, rsesek wrote: > On 2011/11/15 23:00:20, Bernhard Bauer wrote: > > What is going to happen when |plugin_list_| is destroyed? > > ShadowingAtExitManager will delete the PluginService. Ah, I overlooked that one. http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:193: TestingProfile profile_; On 2011/11/15 23:02:02, rsesek wrote: > On 2011/11/15 23:00:20, Bernhard Bauer wrote: > > Why do we need to set up a TestingProfile now instead of a TestingPrefService? > > I'd prefer for PluginPrefs not to require a profile, so if it does, we should > > fix that :) > > Because the NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED gets posted and in the > plugin filter, the Source is NULL which causes a deref crash. Hm, maybe we could just ignore a NULL source there. That shouldn't be a big change.
http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... chrome/browser/chromeos/gview_request_interceptor_unittest.cc:193: TestingProfile profile_; On 2011/11/15 23:14:15, Bernhard Bauer wrote: > On 2011/11/15 23:02:02, rsesek wrote: > > On 2011/11/15 23:00:20, Bernhard Bauer wrote: > > > Why do we need to set up a TestingProfile now instead of a > TestingPrefService? > > > I'd prefer for PluginPrefs not to require a profile, so if it does, we > should > > > fix that :) > > > > Because the NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED gets posted and in the > > plugin filter, the Source is NULL which causes a deref crash. > > Hm, maybe we could just ignore a NULL source there. That shouldn't be a big > change. Yeah, I wasn't sure if we wanted to do that. It seems like papering over a case that can only happen in testing, though. I generally think that if you have to make changes to production code for a test, it's probably wrong.
On 2011/11/15 23:15:36, rsesek wrote: > http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... > File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): > > http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... > chrome/browser/chromeos/gview_request_interceptor_unittest.cc:193: > TestingProfile profile_; > On 2011/11/15 23:14:15, Bernhard Bauer wrote: > > On 2011/11/15 23:02:02, rsesek wrote: > > > On 2011/11/15 23:00:20, Bernhard Bauer wrote: > > > > Why do we need to set up a TestingProfile now instead of a > > TestingPrefService? > > > > I'd prefer for PluginPrefs not to require a profile, so if it does, we > > should > > > > fix that :) > > > > > > Because the NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED gets posted and in the > > > plugin filter, the Source is NULL which causes a deref crash. > > > > Hm, maybe we could just ignore a NULL source there. That shouldn't be a big > > change. > > Yeah, I wasn't sure if we wanted to do that. It seems like papering over a case > that can only happen in testing, though. I generally think that if you have to > make changes to production code for a test, it's probably wrong. I think not making any changes at all to production code for tests is a slightly too noble goal ;-) In this case, PluginPrefs only uses the profile internally as an opaque pointer, so allowing it to be NULL seems reasonable to me, even if that will be the case only in tests (for example, PluginService::PurgePluginListCache is already able to deal with a NULL browser_context).
On 2011/11/16 09:43:45, Bernhard Bauer wrote: > On 2011/11/15 23:15:36, rsesek wrote: > > > http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... > > File chrome/browser/chromeos/gview_request_interceptor_unittest.cc (right): > > > > > http://codereview.chromium.org/8574020/diff/1/chrome/browser/chromeos/gview_r... > > chrome/browser/chromeos/gview_request_interceptor_unittest.cc:193: > > TestingProfile profile_; > > On 2011/11/15 23:14:15, Bernhard Bauer wrote: > > > On 2011/11/15 23:02:02, rsesek wrote: > > > > On 2011/11/15 23:00:20, Bernhard Bauer wrote: > > > > > Why do we need to set up a TestingProfile now instead of a > > > TestingPrefService? > > > > > I'd prefer for PluginPrefs not to require a profile, so if it does, we > > > should > > > > > fix that :) > > > > > > > > Because the NOTIFICATION_PLUGIN_ENABLE_STATUS_CHANGED gets posted and in > the > > > > plugin filter, the Source is NULL which causes a deref crash. > > > > > > Hm, maybe we could just ignore a NULL source there. That shouldn't be a big > > > change. > > > > Yeah, I wasn't sure if we wanted to do that. It seems like papering over a > case > > that can only happen in testing, though. I generally think that if you have to > > make changes to production code for a test, it's probably wrong. > > I think not making any changes at all to production code for tests is a slightly > too noble goal ;-) Noble goals are a Good Thing if they're easily achievable ;). > In this case, PluginPrefs only uses the profile internally as an opaque pointer, > so allowing it to be NULL seems reasonable to me, even if that will be the case > only in tests (for example, PluginService::PurgePluginListCache is already able > to deal with a NULL browser_context). Fair enough. Done.
LGTM, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/8574020/6001
Change committed as 110325 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
