Bug 72038 – Lazily start DRT instances in NRWT

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

Lazily start DRT instances in NRWT

[Tony Chang]

Created attachment 114531 [details]
Patch

[Tony Chang]

Comments 24 through 26 on bug 60605 explain why I'm making this change.

https://bugs.webkit.org/show_bug.cgi?id=60605#c24

[Eric Seidel (no email)]

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.

[Tony Chang]

(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 . . .

[Dirk Pranke]

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

[Tony Chang]

Created attachment 114549 [details]
Patch

[Tony Chang]

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

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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.

[Tony Chang]

(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.

[Tony Chang]

(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.

[Eric Seidel (no email)]

Comment on attachment 114549 [details]
Patch

LGTM.  I don't think your test adds anythign and should jsut be removed.

[Tony Chang]

(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.

[Tony Chang]

Created attachment 114552 [details]
Patch for landing

[Dirk Pranke]

(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 ;)

[Tony Chang]

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

[Philippe Normand]

(In reply to comment #16)
> Committed r99907: <http://trac.webkit.org/changeset/99907>

That one broke GTK...
Landed http://trac.webkit.org/changeset/99935

[Eric Seidel (no email)]

(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.