Bug 78181 - nrwt: simplify worker interface
Summary: nrwt: simplify worker interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 78185 78187
  Show dependency treegraph
 
Reported: 2012-02-08 16:48 PST by Dirk Pranke
Modified: 2012-02-16 19:48 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.23 KB, patch)
2012-02-08 17:09 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge to r107884 (6.62 KB, patch)
2012-02-15 22:31 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix indentation (6.84 KB, patch)
2012-02-15 23:06 PST, Dirk Pranke
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-02-08 16:48:58 PST
nrwt: simplify worker interface
Comment 1 Dirk Pranke 2012-02-08 17:09:03 PST
Created attachment 126196 [details]
Patch
Comment 2 Dirk Pranke 2012-02-15 22:31:51 PST
Created attachment 127311 [details]
merge to r107884
Comment 3 Dirk Pranke 2012-02-15 22:33:46 PST
Tony, can you take a look at this?
Comment 4 Dirk Pranke 2012-02-15 23:06:55 PST
Created attachment 127314 [details]
fix indentation
Comment 5 Tony Chang 2012-02-16 10:21:30 PST
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.
Comment 6 Dirk Pranke 2012-02-16 11:45:13 PST
(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.
Comment 7 Tony Chang 2012-02-16 12:24:09 PST
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.
Comment 8 Dirk Pranke 2012-02-16 17:11:51 PST
Committed r108005: <http://trac.webkit.org/changeset/108005>
Comment 9 Dirk Pranke 2012-02-16 19:21:34 PST
(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.
Comment 10 Dirk Pranke 2012-02-16 19:48:58 PST
(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.