Bug 50381

Created attachment 75362 [details] Patch

*** Bug 49567 has been marked as a duplicate of this bug. ***

Created attachment 75363 [details] ignore - wrong bug

Comment on attachment 75362 [details] Patch whoops ... wrong bug

Comment on attachment 75362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75362&action=review Other than the super() call, I think this is OK. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:370 > + super(_MultiProcessBroker._Worker, self).__init__(broker, Interesting. I was just reading this: http://fuhm.net/super-harmful/ I'm not sure super() is what we want in this MI setting.

Comment on attachment 75362 [details] Patch r- since that constructor isn't probably doing what you want (it's only causing one of the parent classes to be __init__'d too... which may or may not matter.

(In reply to comment #6) > (From update of attachment 75362 [details]) > r- since that constructor isn't probably doing what you want (it's only causing one of the parent classes to be __init__'d too... which may or may not matter. super(_MultiProcessBroker._Worker, self) == dump_render_tree_thread.Worker.__init__() That code does: super(dump_render_tree_thread.Worker, self) == multiprocessing.Process.__init() So it works fine, and, in my admittedly somewhat shaky understanding of MI in Python, is how this code has to be written for drt_thread.Worker to know how to call the correct next method in the MRO. From threads on the Google internal python-users@ list, Guide mentioned that the post you linked to is dated and somewhat misleading. I asked for a recommendation on the best way to do what I'm trying to do here, but have yet to get a reply. There are two possible other options that I can think of. 1) have MultiProcessBroker._Worker init both subclasses (and not use super). drt_thread.Worker only initialize itself and not call anything else (making it more like a mixin or a trait). 2) use delegation or aggregation instead of inheritance in the first place. This is the way I originally did this, but it was clunky and the naming was awkward. That said, it's also a bit more testable, depending on how it's coded, because I could make drt_thread.Worker a parameter that gets passed in, and which would remove the dependency on dump_render_tree_thread. What do you think?

Created attachment 75682 [details] update w/ new interfaces, aggregation code

Please review again. This code uses the new aggregation-style interfaces I've added in 50557. There's a bunch of code duplication here that would go away if we decided to allow multiple inheritance. However, if we allow it here and didn't allow it in the first patches for 50375 (the ones that didn't use aggregation), we should probably document why it was okay in one place and not okay in another somewhere.

Since when do we not allow multiple inheritance? We use it all over the tool/commands/ directory. :)

(In reply to comment #10) > Since when do we not allow multiple inheritance? We use it all over the tool/commands/ directory. :) MI is fine for mixins, but that's not what Dirk's talking about here.

(In reply to comment #11) > (In reply to comment #10) > > Since when do we not allow multiple inheritance? We use it all over the tool/commands/ directory. :) > > MI is fine for mixins, but that's not what Dirk's talking about here. Yeah, I meant not allow as in "R- its use in this patch", not banning multiple inheritance generally. However, given that, for example, your comment in #4 about super not being the thing to use was wrong AFAICT, and that it took me a bunch of research to figure out what the "right" way to do MI in this setting was prior to that, banning some uses doesn't seem like a completely bad idea.

marking as WONTFIX. will split up the patches differently.