| Summary: | new-run-webkit-tests: clean up host/systemhost interactions in the port subsystem | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, eric, levin, ojan, tony, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 74551, 74556, 74562, 74566, 74878 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Dirk Pranke
2011-12-08 16:23:03 PST
Created attachment 118488 [details]
Patch
Created attachment 118492 [details] merge to r102395 Comment on attachment 118488 [details]
Patch
I like the change. It's a bit difficult to take in at this size, but I'll just come back for another reading. :)
Yeah, unfortunately it was kind of an all-or-nothing thing in order to get all the tests to pass. I changed as little as I could while making the fundamental change to making the host parameter to Port() required :(. Take your time ... there's no real urgency on this. "blip" ... "blip" ... (keeping it on the radar) (In reply to comment #4) > Yeah, unfortunately it was kind of an all-or-nothing thing in order to get all the tests to pass. I changed as little as I could while making the fundamental change to making the host parameter to Port() required :(. Why not do patches leading up to making the host parameter to Port required? You could use host= at the calling sites and making it the first parameter (which seems possible because all parameters are optional now). The final patch would make the parameter required and remove host= in calling sites. okay, admittedly I could probably break this up into a few smaller patches, but I was hoping to not have to spend the time to do so. I believe Eric has looked at this a little bit, so I'll let him say if he thinks this is not unreasonable to review as is, or if he'd like me to break it up. The actual patch isn't very complicated; it's mostly just search&replace with a few other spots that have actual changes as noted in the ChangeLogs. (I can take a look this evening or tomorrow morning.) I've looked at it in part a couple times today. But never made it all the way through. I agree with Levin, this could be done in pieces. That certainly would make this a quicker review. Comment on attachment 118492 [details] merge to r102395 View in context: https://bugs.webkit.org/attachment.cgi?id=118492&action=review > Tools/ChangeLog:13 > + 2) Unit tests for the Port objects can use a full MockHost Seems slightly dangerous if in practice Port() can only be expected to have a SystemHost. Is this just a partial step along that goal? > Tools/ChangeLog:18 > + 3) the TestPort object no longer modifies the (mock) filesystem at > + all; clients are required to call > + add_unit_tests_to_mock_filesystem() to ensure that the system is set > + up correctly. The MockHost() object takes care of this for > + most callers. This is a fantastic change and could be done first. :) Okay, I will break this up into chunks and cancel the r? on this. I was able to break this patch into 4 separate things; they're listed as blockers for this now. Please take a look when you get a chance. Thanks! closing ... all of the individual patches have landed. |