Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13533007:
Test extension reloading behavior. (Closed)
|
Created:
7 years, 8 months ago by Jeffrey Yasskin Modified:
7 years, 8 months ago CC:
chromium-reviews, creis+watch_chromium.org, jam, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, ajwong+watch_chromium.org, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTest extension reloading behavior.
By testing the actual interaction with renderer processes and the
notification system, I can expose the fact that we're starting a whole
extra process and loading the extension around 3 times.
BUG=178542
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193974
Patch Set 1 #
Total comments: 33
Patch Set 2 : Fix Pawel's and Matt's comments #
Total comments: 3
Patch Set 3 : Add ForTesting to SetRenderProcessHostFactory #
Total comments: 11
Patch Set 4 : Fix Pawel's review comments #Patch Set 5 : Filter IPC messages to extension messages #Patch Set 6 : Define a local_state (PrefService) so tests don't crash when it's null #
Total comments: 3
Patch Set 7 : Fix Aura tests by using RenderViewHostTestEnabler and splitting out the IO thread #
Total comments: 2
Patch Set 8 : Fix the rest of the tests #Patch Set 9 : Sync #
Total comments: 1
Patch Set 10 : Put back the ScopedTestingLocalState #Patch Set 11 : Add a ScopedOleInitializer for Windows #Messages
Total messages: 23 (0 generated)
I'd like to spread this sort of testing out across more of the ExtensionService tests, but I got just one working as a proof of concept so you can correct my course before I spend even more time on it. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3667: notifications.ListenForAll(chrome::NOTIFICATION_EXTENSIONS_READY); I think it makes a lot of sense to assert against the EXTENSION notifications, since they're part of the API ExtensionService exposes. The RENDERER_PROCESS ones are less obviously-correct: they help me test how ExtensionService creates processes, but maybe I should do that entirely through the ProcessObserver (which would require extending the MockRenderProcessHost interface a bit, I think). https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3697: testing::Matcher<uint32> expected_types[] = { The long lists of notifications and message types should get shorter as I fix bugs here, but they'll still be annoyingly long. I'd welcome suggestions for more concise ways to test this aspect of ExtensionService behavior. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3700: ExtensionMsg_Loaded::ID, Why do we send Loaded/ActivateExtension 3 times? Probably a bug, the fixing of which is the reason I'm writing this test. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3702: testing::_, // ViewMsg_New::ID, DEPS prevents me from #including content/common/view_messages.h, so I can't check exactly how the ExtensionService tells its renderers to start up. I could check that these are View messages in general, or filter to just the ExtensionMsgs, or add something to the MockRenderProcessHost to report view operations semantically, somehow. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3740: service_->ReloadExtension(extension_id); Commenting this line out has no effect on the test, so the extra reload below is a pure bug in ExtensionService. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3754: chrome::NOTIFICATION_EXTENSION_UNLOADED, Note the extra reload here. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3760: ASSERT_EQ(3u, observer.process_messages.size()); This should become "2" when I fix the extra-reload bug. https://codereview.chromium.org/13533007/diff/1/content/browser/site_instance... File content/browser/site_instance_impl.h (left): https://codereview.chromium.org/13533007/diff/1/content/browser/site_instance... content/browser/site_instance_impl.h:48: // passed on to SiteInstances spawned by this one. This wasn't actually true, but I've made it true with the change to SiteInstanceImpl::GetRelatedSiteInstance() in this CL. https://codereview.chromium.org/13533007/diff/1/ipc/ipc_test_sink.h File ipc/ipc_test_sink.h (right): https://codereview.chromium.org/13533007/diff/1/ipc/ipc_test_sink.h#newcode74 ipc/ipc_test_sink.h:74: class TestSink : public Channel, public Listener { This allows chaining TestSinks.
This looks like a good thing to test. I don't think we should add it to all the tests - it would add too much extra noise. Maybe there's a select few that we really care about. https://codereview.chromium.org/13533007/diff/1/chrome/browser/chrome_content... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/chrome_content... chrome/browser/chrome_content_browser_client.cc:902: // Tests may not set up a profile manager. Can we fix that instead? https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3667: notifications.ListenForAll(chrome::NOTIFICATION_EXTENSIONS_READY); On 2013/04/04 09:34:35, Jeffrey Yasskin wrote: > I think it makes a lot of sense to assert against the EXTENSION notifications, > since they're part of the API ExtensionService exposes. > > The RENDERER_PROCESS ones are less obviously-correct: they help me test how > ExtensionService creates processes, but maybe I should do that entirely through > the ProcessObserver (which would require extending the MockRenderProcessHost > interface a bit, I think). Yeah, I really like testing for notifications and messages explicitly. We've had bugs in the past where we changed the order of some notifications, and things broke. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3679: service_->Init(); So this test used to work without ever calling Init() on ExtensionService? https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3697: testing::Matcher<uint32> expected_types[] = { On 2013/04/04 09:34:35, Jeffrey Yasskin wrote: > The long lists of notifications and message types should get shorter as I fix > bugs here, but they'll still be annoyingly long. I'd welcome suggestions for > more concise ways to test this aspect of ExtensionService behavior. Some messages we probably don't care about. Might be good to add a testing primitive that says: "actual results must match the order specified in this expected list, but ignore anything that shows up in actual that expected doesn't mention." https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3700: ExtensionMsg_Loaded::ID, On 2013/04/04 09:34:35, Jeffrey Yasskin wrote: > Why do we send Loaded/ActivateExtension 3 times? Probably a bug, the fixing of > which is the reason I'm writing this test. Yikes! Good find. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3739: // shouldn't cause an extra reload. I'm not sure I agree with the second half. It makes sense that calling ReloadExtension twice should trigger 2 reloads. https://codereview.chromium.org/13533007/diff/1/content/browser/browsing_inst... File content/browser/browsing_instance.h (right): https://codereview.chromium.org/13533007/diff/1/content/browser/browsing_inst... content/browser/browsing_instance.h:70: SiteInstanceImpl* GetSiteInstanceForURL(const GURL& url); why change these?
https://codereview.chromium.org/13533007/diff/1/chrome/browser/chrome_content... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/chrome_content... chrome/browser/chrome_content_browser_client.cc:902: // Tests may not set up a profile manager. On 2013/04/04 20:54:26, Matt Perry wrote: > Can we fix that instead? Probably, see TestingBrowserProcess::SetProfileManager. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_process_manager.h (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_process_manager.h:146: void SetRenderProcessHostFactoryForTest( Why not just use SiteInstance directly? More layers of indirection make the code harder to follow. https://codereview.chromium.org/13533007/diff/1/content/public/browser/site_i... File content/public/browser/site_instance.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/browser/site_i... content/public/browser/site_instance.h:115: virtual void set_render_process_host_factory( If it's virtual, I think it should change from unix_hacker to CamelCase style. https://codereview.chromium.org/13533007/diff/1/content/public/test/mock_rend... File content/public/test/mock_render_process_host.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/test/mock_rend... content/public/test/mock_render_process_host.h:123: factory->AddObserver(this); nit: Why this code doesn't move the implementation to the .cc file? (there is a general rule about doing that). https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... File content/public/test/test_notification_tracker.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... content/public/test/test_notification_tracker.h:48: // Given notifications type(sp, returns true if the list of notifications If you're adding methods below, please remove ones here that are now redundant.
On 2013/04/04 20:54:26, Matt Perry wrote: > This looks like a good thing to test. I don't think we should add it to all the > tests - it would add too much extra noise. Maybe there's a select few that we > really care about. I'm planning to add this sort of assertion to one install, reload, and update test, and maybe add an "uninstall" test with notification assertions. I agree that we shouldn't add it to all the tests. https://codereview.chromium.org/13533007/diff/1/chrome/browser/chrome_content... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/chrome_content... chrome/browser/chrome_content_browser_client.cc:902: // Tests may not set up a profile manager. On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > On 2013/04/04 20:54:26, Matt Perry wrote: > > Can we fix that instead? > > Probably, see TestingBrowserProcess::SetProfileManager. Some of the uses of that function re-set it to NULL when they're done, and other don't. I'll reset it in this test, but it could probably use a better API. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_process_manager.h (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_process_manager.h:146: void SetRenderProcessHostFactoryForTest( On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > Why not just use SiteInstance directly? More layers of indirection make the code > harder to follow. i.e. GetSiteInstanceForTest()->SetRenderProcessHostFactory()? ok. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3679: service_->Init(); On 2013/04/04 20:54:26, Matt Perry wrote: > So this test used to work without ever calling Init() on ExtensionService? Yep. I it required the omission, or else it hit the ProfileManager and then needed several other bits of the system to be set up. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3697: testing::Matcher<uint32> expected_types[] = { On 2013/04/04 20:54:26, Matt Perry wrote: > On 2013/04/04 09:34:35, Jeffrey Yasskin wrote: > > The long lists of notifications and message types should get shorter as I fix > > bugs here, but they'll still be annoyingly long. I'd welcome suggestions for > > more concise ways to test this aspect of ExtensionService behavior. > > Some messages we probably don't care about. Might be good to add a testing > primitive that says: "actual results must match the order specified in this > expected list, but ignore anything that shows up in actual that expected doesn't > mention." That's not quite right, since we want to assert that there aren't any extra ExtensionMsgs that we didn't expect. I can easily filter out messages by class so we'd only get ExtensionMsgs, although I think there's an argument that we care about these ViewMsgs too. https://codereview.chromium.org/13533007/diff/1/chrome/browser/extensions/ext... chrome/browser/extensions/extension_service_unittest.cc:3739: // shouldn't cause an extra reload. On 2013/04/04 20:54:26, Matt Perry wrote: > I'm not sure I agree with the second half. It makes sense that calling > ReloadExtension twice should trigger 2 reloads. I think that would be plausible behavior, but it would actually be harder to achieve given the current implementation. ReloadExtension() just unloads the extension and posts tasks saying to reload it, and in order to cause a second reload, we'd have to attach another task starting the reload to run at the end of the first load. In any case, the current behavior is that you always get 2 reloads, no matter how many times you call ReloadExtension() before the first reload finishes, and I just want to get that down to 1 reload. Making extra ReloadExtension() calls cause extra reloads is out of scope. I could reword the comment if you want, to say that the "no effect" behavior is just an artifact of the current implementation. https://codereview.chromium.org/13533007/diff/1/content/browser/browsing_inst... File content/browser/browsing_instance.h (right): https://codereview.chromium.org/13533007/diff/1/content/browser/browsing_inst... content/browser/browsing_instance.h:70: SiteInstanceImpl* GetSiteInstanceForURL(const GURL& url); On 2013/04/04 20:54:26, Matt Perry wrote: > why change these? No good reason anymore. I was thinking that I might want to keep the non-virtual set_render_process_host_factory() on SiteInstanceImpl (and have the virtual SetRenderProcessHostFactory delegate to it), and then if this returns SiteInstanceImpl we can avoid the virtual call overhead, but I haven't kept that method, so returning a more precise type doesn't really gain anything. https://codereview.chromium.org/13533007/diff/1/content/public/browser/site_i... File content/public/browser/site_instance.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/browser/site_i... content/public/browser/site_instance.h:115: virtual void set_render_process_host_factory( On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > If it's virtual, I think it should change from unix_hacker to CamelCase style. Done. https://codereview.chromium.org/13533007/diff/1/content/public/test/mock_rend... File content/public/test/mock_render_process_host.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/test/mock_rend... content/public/test/mock_render_process_host.h:123: factory->AddObserver(this); On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > nit: Why this code doesn't move the implementation to the .cc file? (there is a > general rule about doing that). Done. https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... File content/public/test/test_notification_tracker.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... content/public/test/test_notification_tracker.h:48: // Given notifications type(sp, returns true if the list of notifications On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > If you're adding methods below, please remove ones here that are now redundant. There are 81 calls to Check1AndReset across the codebase, so I'm definitely not removing these in the same CL as I add the extension test. That's enough uses that I think it's better to remove them in a 3-step sequence, so I can commit to replacing them with EXPECT_THAT in a series of subsequent changes.
lgtm
https://codereview.chromium.org/13533007/diff/2002/content/public/test/test_n... File content/public/test/test_notification_tracker.h (right): https://codereview.chromium.org/13533007/diff/2002/content/public/test/test_n... content/public/test/test_notification_tracker.h:68: std::vector<int> GetTypes() const; hmm, we shouldn't be adding more dependencies on notifications. Pawel is working on removing the remaining notifications out of content.
On Apr 7, 2013 11:29 PM, <jam@chromium.org> wrote: > > > https://codereview.chromium.org/13533007/diff/2002/content/public/test/test_n... > File content/public/test/test_notification_tracker.h (right): > > https://codereview.chromium.org/13533007/diff/2002/content/public/test/test_n... > content/public/test/test_notification_tracker.h:68: std::vector<int> > GetTypes() const; > hmm, we shouldn't be adding more dependencies on notifications. Pawel is > working on removing the remaining notifications out of content. I agree about trying to remove use of notifications, but that shouldn't prevent us from testing the ones we already have. Any place we manage to kill a use of notifications, the use of this function from a test will naturally go away too. > https://codereview.chromium.org/13533007/
On 2013/04/07 23:46:29, Jeffrey Yasskin wrote: > On Apr 7, 2013 11:29 PM, <mailto:jam@chromium.org> wrote: > > > > > > > https://codereview.chromium.org/13533007/diff/2002/content/public/test/test_n... > > File content/public/test/test_notification_tracker.h (right): > > > > > https://codereview.chromium.org/13533007/diff/2002/content/public/test/test_n... > > content/public/test/test_notification_tracker.h:68: std::vector<int> > > GetTypes() const; > > hmm, we shouldn't be adding more dependencies on notifications. Pawel is > > working on removing the remaining notifications out of content. > > I agree about trying to remove use of notifications, but that shouldn't > prevent us from testing the ones we already have. Any place we manage to > kill a use of notifications, the use of this function from a test will > naturally go away too. ok. https://codereview.chromium.org/13533007/diff/2002/content/public/browser/sit... File content/public/browser/site_instance.h (right): https://codereview.chromium.org/13533007/diff/2002/content/public/browser/sit... content/public/browser/site_instance.h:115: virtual void SetRenderProcessHostFactory( nit: add a ForTesting suffix to the name
Thanks for taking a look. https://codereview.chromium.org/13533007/diff/2002/content/public/browser/sit... File content/public/browser/site_instance.h (right): https://codereview.chromium.org/13533007/diff/2002/content/public/browser/sit... content/public/browser/site_instance.h:115: virtual void SetRenderProcessHostFactory( On 2013/04/08 15:49:06, jam wrote: > nit: add a ForTesting suffix to the name Done. https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... content/browser/site_instance_impl.cc:196: result->SetRenderProcessHostFactoryForTesting(render_process_host_factory_); I got a "calling functions intended only for testing" warning from this, but I think that's ok in this case?
btw looks fine to me, i'll wait for pawel's lgtm https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... content/browser/site_instance_impl.cc:196: result->SetRenderProcessHostFactoryForTesting(render_process_host_factory_); On 2013/04/08 16:30:48, Jeffrey Yasskin wrote: > I got a "calling functions intended only for testing" warning from this, but I > think that's ok in this case? ah, since there's a call path in a non-test file, you can't use it. ignore my message then
https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... File content/public/test/test_notification_tracker.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... content/public/test/test_notification_tracker.h:48: // Given notifications type(sp, returns true if the list of notifications On 2013/04/05 13:14:01, Jeffrey Yasskin wrote: > On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > > If you're adding methods below, please remove ones here that are now > redundant. > > There are 81 calls to Check1AndReset across the codebase, so I'm definitely not > removing these in the same CL as I add the extension test. That's enough uses > that I think it's better to remove them in a 3-step sequence, so I can commit to > replacing them with EXPECT_THAT in a series of subsequent changes. Please add TODOs then. https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/extension_process_manager.h (right): https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... chrome/browser/extensions/extension_process_manager.h:144: content::SiteInstance* GetSiteInstanceForTest() { nit: I don't think this needs to be inline. Please move the implementation to .cc file (or convert to hacker_style for a simple accessor). https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... content/browser/site_instance_impl.cc:196: result->SetRenderProcessHostFactoryForTesting(render_process_host_factory_); By the way, calling Set, a mutating method in a method called Get is a _terrible_ idea, testing or not.
https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... File content/public/test/test_notification_tracker.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... content/public/test/test_notification_tracker.h:48: // Given notifications type(sp, returns true if the list of notifications On 2013/04/08 20:03:51, Paweł Hajdan Jr. wrote: > On 2013/04/05 13:14:01, Jeffrey Yasskin wrote: > > On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > > > If you're adding methods below, please remove ones here that are now > > redundant. > > > > There are 81 calls to Check1AndReset across the codebase, so I'm definitely > not > > removing these in the same CL as I add the extension test. That's enough uses > > that I think it's better to remove them in a 3-step sequence, so I can commit > to > > replacing them with EXPECT_THAT in a series of subsequent changes. > > Please add TODOs then. Will do. https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/extension_process_manager.h (right): https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... chrome/browser/extensions/extension_process_manager.h:144: content::SiteInstance* GetSiteInstanceForTest() { On 2013/04/08 20:03:51, Paweł Hajdan Jr. wrote: > nit: I don't think this needs to be inline. Please move the implementation to > .cc file (or convert to hacker_style for a simple accessor). You're saying that if I call it "site_instance_for_test()", it can stay in the header? https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... content/browser/site_instance_impl.cc:196: result->SetRenderProcessHostFactoryForTesting(render_process_host_factory_); On 2013/04/08 20:03:51, Paweł Hajdan Jr. wrote: > By the way, calling Set, a mutating method in a method called Get is a > _terrible_ idea, testing or not. What? The Set() is on the newly-constructed result object, not *this. https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... content/browser/site_instance_impl.cc:196: result->SetRenderProcessHostFactoryForTesting(render_process_host_factory_); On 2013/04/08 16:54:53, jam wrote: > On 2013/04/08 16:30:48, Jeffrey Yasskin wrote: > > I got a "calling functions intended only for testing" warning from this, but I > > think that's ok in this case? > > ah, since there's a call path in a non-test file, you can't use it. ignore my > message then Well, I could put back the changes to browsing_instance.h and then modify result->render_process_host_factory_ directly. Which do you prefer?
https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/extension_process_manager.h (right): https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... chrome/browser/extensions/extension_process_manager.h:144: content::SiteInstance* GetSiteInstanceForTest() { On 2013/04/08 22:09:38, Jeffrey Yasskin wrote: > On 2013/04/08 20:03:51, Paweł Hajdan Jr. wrote: > > nit: I don't think this needs to be inline. Please move the implementation to > > .cc file (or convert to hacker_style for a simple accessor). > > You're saying that if I call it "site_instance_for_test()", it can stay in the > header? Yes, that would be fine for me. https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... content/browser/site_instance_impl.cc:196: result->SetRenderProcessHostFactoryForTesting(render_process_host_factory_); On 2013/04/08 22:09:38, Jeffrey Yasskin wrote: > On 2013/04/08 20:03:51, Paweł Hajdan Jr. wrote: > > By the way, calling Set, a mutating method in a method called Get is a > > _terrible_ idea, testing or not. > > What? The Set() is on the newly-constructed result object, not *this. Oops, this makes more sense. Please ignore my earlier comment.
I have some tests to investigate and fix, but that'll have to wait for morning MUC time. https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... File content/public/test/test_notification_tracker.h (right): https://codereview.chromium.org/13533007/diff/1/content/public/test/test_noti... content/public/test/test_notification_tracker.h:48: // Given notifications type(sp, returns true if the list of notifications On 2013/04/08 20:03:51, Paweł Hajdan Jr. wrote: > On 2013/04/05 13:14:01, Jeffrey Yasskin wrote: > > On 2013/04/04 21:30:05, Paweł Hajdan Jr. wrote: > > > If you're adding methods below, please remove ones here that are now > > redundant. > > > > There are 81 calls to Check1AndReset across the codebase, so I'm definitely > not > > removing these in the same CL as I add the extension test. That's enough uses > > that I think it's better to remove them in a 3-step sequence, so I can commit > to > > replacing them with EXPECT_THAT in a series of subsequent changes. > > Please add TODOs then. Done. https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/extension_process_manager.h (right): https://codereview.chromium.org/13533007/diff/26001/chrome/browser/extensions... chrome/browser/extensions/extension_process_manager.h:144: content::SiteInstance* GetSiteInstanceForTest() { On 2013/04/08 22:17:38, Paweł Hajdan Jr. wrote: > On 2013/04/08 22:09:38, Jeffrey Yasskin wrote: > > On 2013/04/08 20:03:51, Paweł Hajdan Jr. wrote: > > > nit: I don't think this needs to be inline. Please move the implementation > to > > > .cc file (or convert to hacker_style for a simple accessor). > > > > You're saying that if I call it "site_instance_for_test()", it can stay in the > > header? > > Yes, that would be fine for me. Done. https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... File content/browser/site_instance_impl.cc (right): https://codereview.chromium.org/13533007/diff/26001/content/browser/site_inst... content/browser/site_instance_impl.cc:196: result->SetRenderProcessHostFactoryForTesting(render_process_host_factory_); On 2013/04/08 16:54:53, jam wrote: > On 2013/04/08 16:30:48, Jeffrey Yasskin wrote: > > I got a "calling functions intended only for testing" warning from this, but I > > think that's ok in this case? > > ah, since there's a call path in a non-test file, you can't use it. ignore my > message then Done, although I can switch to directly modifying the member if you prefer that option.
Here's another attempt. The reload test should work on all platforms now, but the change breaks a bunch of other extension tests by switching the type of message loop they're running under. Could you all let me know if this is the direction I should be going before I fix those tests, especially given that patch 7 may compromise how well this test can expose bugs. https://codereview.chromium.org/13533007/diff/40003/chrome/browser/extensions... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/40003/chrome/browser/extensions... chrome/browser/extensions/extension_service_unittest.cc:3709: ExtensionMsg_Loaded::ID, Why does this extra Loaded/ActivateExtension pair go away under RenderViewHostTestEnabler? Is it because of a bug in the interaction between the real RenderViewHostImpl and the fake unittest environment that's been fixed by using TestRenderViewHostFactory instead, or is it a bug in the live code that's just hidden because TestRenderViewHostFactory doesn't send as many events as RenderViewHostImpl? If it's the second, that means we're losing some testing power by using RenderViewHostTestEnabler. A point in favor of the second is that the NotifyRenderViewType event also disappears because it's fired from an observer called by RenderViewHostImpl::CreateRenderView but not TestRenderViewHost::CreateRenderView. I gdb'ed a chrome reloading an unpacked extension and got 4 ExtensionMsg_Loaded sent on 4 different RenderProcessHostImpls, and then 6 more (for a total of 7) on the 4th host.
lgtm https://codereview.chromium.org/13533007/diff/40003/chrome/browser/extensions... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/40003/chrome/browser/extensions... chrome/browser/extensions/extension_service_unittest.cc:3709: ExtensionMsg_Loaded::ID, On 2013/04/09 17:58:32, Jeffrey Yasskin wrote: > Why does this extra Loaded/ActivateExtension pair go away under > RenderViewHostTestEnabler? Is it because of a bug in the interaction between the > real RenderViewHostImpl and the fake unittest environment that's been fixed by > using TestRenderViewHostFactory instead, or is it a bug in the live code that's > just hidden because TestRenderViewHostFactory doesn't send as many events as > RenderViewHostImpl? If it's the second, that means we're losing some testing > power by using RenderViewHostTestEnabler. > > A point in favor of the second is that the NotifyRenderViewType event also > disappears because it's fired from an observer called by > RenderViewHostImpl::CreateRenderView but not > TestRenderViewHost::CreateRenderView. > > I gdb'ed a chrome reloading an unpacked extension and got 4 ExtensionMsg_Loaded > sent on 4 different RenderProcessHostImpls, and then 6 more (for a total of 7) > on the 4th host. Your experience with gdb makes me wonder if the type of host has an effect. Things like: is the host a regular renderer or extension process, or does the host have content scripts running in it? https://codereview.chromium.org/13533007/diff/45001/chrome/browser/extensions... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/45001/chrome/browser/extensions... chrome/browser/extensions/extension_service_unittest.cc:3968: loop_.Run(); nit: could you move this to a global helper function?
LGTM
https://codereview.chromium.org/13533007/diff/40003/chrome/browser/extensions... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/40003/chrome/browser/extensions... chrome/browser/extensions/extension_service_unittest.cc:3709: ExtensionMsg_Loaded::ID, On 2013/04/09 20:20:03, Matt Perry wrote: > On 2013/04/09 17:58:32, Jeffrey Yasskin wrote: > > Why does this extra Loaded/ActivateExtension pair go away under > > RenderViewHostTestEnabler? Is it because of a bug in the interaction between > the > > real RenderViewHostImpl and the fake unittest environment that's been fixed by > > using TestRenderViewHostFactory instead, or is it a bug in the live code > that's > > just hidden because TestRenderViewHostFactory doesn't send as many events as > > RenderViewHostImpl? If it's the second, that means we're losing some testing > > power by using RenderViewHostTestEnabler. > > > > A point in favor of the second is that the NotifyRenderViewType event also > > disappears because it's fired from an observer called by > > RenderViewHostImpl::CreateRenderView but not > > TestRenderViewHost::CreateRenderView. > > > > I gdb'ed a chrome reloading an unpacked extension and got 4 > ExtensionMsg_Loaded > > sent on 4 different RenderProcessHostImpls, and then 6 more (for a total of 7) > > on the 4th host. > > Your experience with gdb makes me wonder if the type of host has an effect. > Things like: is the host a regular renderer or extension process, or does the > host have content scripts running in it? Right, a live chrome has some other non-extension processes around which perhaps get notified (although they probably shouldn't be for an extension with no content scripts; there's space for more tests along these lines). The profile was blank except for this extension, so I don't think there were other content scripts running. https://codereview.chromium.org/13533007/diff/45001/chrome/browser/extensions... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/45001/chrome/browser/extensions... chrome/browser/extensions/extension_service_unittest.cc:3968: loop_.Run(); On 2013/04/09 20:20:03, Matt Perry wrote: > nit: could you move this to a global helper function? Done. That helper could probably stand to move to base/test, but I don't want to propose that in this CL. https://codereview.chromium.org/13533007/diff/59001/chrome/browser/extensions... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/13533007/diff/59001/chrome/browser/extensions... chrome/browser/extensions/extension_service_unittest.cc:430: io_thread_(main_loop_type == MessageLoop::TYPE_IO ? Ew. What we actually need to do is split the helper functions out of the fixture and let each test set up its threads appropriately instead of inheriting from ExtensionServiceTestBase. I filed http://crbug.com/229874 in the hope that we can make setting up threads easier too.
I think I still need jam's lgtm for ipc/. Rietveld thinks I have it, but that was just him saying he'd wait for Pawel.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/13533007/79011
Message was sent while issue was closed.
Change committed as 193974
Message was sent while issue was closed.
This appears to have created a whole bundle of races in another test using ExtensionServiceTestBase and the io thread: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28t... The io thread appears to running freely and accessing the std::map while the main thread is tearing down the test. |
