Summary: | nrwt: simplify worker interface | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||||
Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, eric, ojan, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 78185, 78187 | ||||||||||
Attachments: |
|
Description
Dirk Pranke
2012-02-08 16:48:58 PST
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. |