Bug 78181 – nrwt: simplify worker interface

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt: simplify worker interface

[Dirk Pranke]

Created attachment 126196 [details]
Patch

[Dirk Pranke]

Created attachment 127311 [details]
merge to r107884

[Dirk Pranke]

Tony, can you take a look at this?

[Dirk Pranke]

Created attachment 127314 [details]
fix indentation

[Tony Chang]

Comment on attachment 127314 [details]
fix indentation

Should you remove duplicate methods in _TestWorker?

Since there's only one real Worker implementation, it's impossible to tell what should be shared and what shouldn't be shared.  There's nothing wrong with this code change, but I can't evaluate if it's better or worse than before.

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 127314 [details])
> Should you remove duplicate methods in _TestWorker?
> 

Good catch, will do.

> Since there's only one real Worker implementation, it's impossible to tell what should be shared and what shouldn't be shared.  There's nothing wrong with this code change, but I can't evaluate if it's better or worse than before.

for a second implementation, check out the Worker implementation that could be used in test-webkitpy in https://bugs.webkit.org/attachment.cgi?id=126445&action=review (part of bug 78185).

Ultimately the whole message-passing aspect of this will get hidden and I can cut a couple hundred lines out of manager_worker_broker.py, dramatically simplifying the implementation, but I don't quite have that patch ready for review yet.

[Tony Chang]

I was trying to make 2 points:
1) An abstract interface or base class with a single implementation don't add value. _TestWorker should inherit from Worker.
2) When creating an abstract interface or base class, it's easier to understand when there are two implementations.  Which is to say, I would rather review (a) a patch which adds the new implementation, then refactors to share code or (b) a patch that adds the second implementation and base class at the same time.  I'm sure when you implemented this, you did either (a) or (b).  In the current patch, I don't see any of that, so I can't evaluate whether this is an improvement.

This patch is fine, but it doesn't make sense without future context.

[Dirk Pranke]

Committed r108005: <http://trac.webkit.org/changeset/108005>

[Dirk Pranke]

(In reply to comment #7)
> I was trying to make 2 points:
> 1) An abstract interface or base class with a single implementation don't add value. _TestWorker should inherit from Worker.

I'm not sure I agree with the first sentence, but this bug is probably not the ideal place to discuss such a broad statement about design :). I have considered your point, though.

> 2) When creating an abstract interface or base class, it's easier to understand when there are two implementations.  Which is to say, I would rather review (a) a patch which adds the new implementation, then refactors to share code or (b) a patch that adds the second implementation and base class at the same time.  I'm sure when you implemented this, you did either (a) or (b).  In the current patch, I don't see any of that, so I can't evaluate whether this is an improvement.
>
> This patch is fine, but it doesn't make sense without future context.

Understood. Unfortunately, this can be hard to do in practice. I will attempt to do better in the future.

[Dirk Pranke]

(In reply to comment #9)
> I'm not sure I agree with the first sentence, but this bug is probably not the ideal place to discuss such a broad statement about design :). I have considered your point, though.
> 

Note that I would like to understand what you're getting at here better; this was not meant to be dismissive.

Also, I forgot to actually fix/update the test code ... see bug 78870.