Bug 72038

Created attachment 114531 [details] Patch

Comments 24 through 26 on bug 60605 explain why I'm making this change. https://bugs.webkit.org/show_bug.cgi?id=60605#c24

Comment on attachment 114531 [details] Patch I think we should have a way to test this. I like the change though. There is no need for us to have started these eagerly.

(In reply to comment #3) > (From update of attachment 114531 [details]) > I think we should have a way to test this. I like the change though. There is no need for us to have started these eagerly. Ok, I will try to come up with something . . .

This change seems reasonable to me, but please see comment #27 on bug 60605 for another potential way to solve the problem there.

Created attachment 114549 [details] Patch

Here's a unittest, but it's kind of pointless. Eric, I'm not sure exactly what behavior you want me to be testing.

Comment on attachment 114549 [details] Patch Well, the lazy part is the interesting part, but it looks like that _run_test_in_another_thread is one big function so it looks difficult to test that this is lazy.

(In reply to comment #7) > Here's a unittest, but it's kind of pointless. I'm not sure what that test is even testing. If I was going to write a test for this, I would test that calling create_worker() didn't actually start a DRT instance, and I would put that test in webkitpy/layout_tests/port/port_testcase.py. That said, I'm not sure that I think this change needs an additional test, either, since you're simply subtracting a method from the Port API; verifying that the existing tests all still pass seems sufficient to me.

(In reply to comment #8) > (From update of attachment 114549 [details]) > Well, the lazy part is the interesting part, but it looks like that _run_test_in_another_thread is one big function so it looks difficult to test that this is lazy. I don't think it's important that we lazily start DRT instances. If some port needed to start it early (say, in the Driver's __init__ method), that seems OK. That is, nothing really depends on this lazy behavior.

(In reply to comment #9) > (In reply to comment #7) > > Here's a unittest, but it's kind of pointless. > > I'm not sure what that test is even testing. Yup, I'm not sure what you guys want me to test :) > If I was going to write a test for this, I would test that calling create_worker() didn't actually start a DRT instance, and I would put that test in webkitpy/layout_tests/port/port_testcase.py. create_worker() didn't start a DRT instance before this change either. I'm not sure what that would be testing.

Comment on attachment 114549 [details] Patch LGTM. I don't think your test adds anythign and should jsut be removed.

(In reply to comment #12) > (From update of attachment 114549 [details]) > LGTM. I don't think your test adds anythign and should jsut be removed. Ok, I'll land the first patch without the test.

Created attachment 114552 [details] Patch for landing

(In reply to comment #11) > create_worker() didn't start a DRT instance before this change either. I'm not sure what that would be testing. Well, given that start() is gone, it would come close to ensuring that the only way a DRT could be started would be by calling run_test. I'm not saying that it would be a great test ;)

Committed r99907: <http://trac.webkit.org/changeset/99907>

(In reply to comment #16) > Committed r99907: <http://trac.webkit.org/changeset/99907> That one broke GTK... Landed http://trac.webkit.org/changeset/99935

(In reply to comment #17) > (In reply to comment #16) > > Committed r99907: <http://trac.webkit.org/changeset/99907> > > That one broke GTK... > Landed http://trac.webkit.org/changeset/99935 We need a unittest for this, or it will break again.