nrwt: simplify worker interface
Created attachment 126196 [details] Patch
Created attachment 127311 [details] merge to r107884
Tony, can you take a look at this?
Created attachment 127314 [details] fix indentation
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.
(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.
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.
Committed r108005: <http://trac.webkit.org/changeset/108005>
(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.
(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.