[Bug 37664] Chromium: Add --chromium option to new-run-webkit-websocketserver

bugzill...@webkit.org

unread,

May 14, 2010, 5:23:59 PM5/14/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=37664

Eric Seidel <er...@webkit.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #55520|review? |review-
Flag| |

--- Comment #34 from Eric Seidel <er...@webkit.org> 2010-05-14 14:23:56 PST ---
(From update of attachment 55520)
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:52
+ chromium.ChromiumPort.setup_environ_for_server(self)
This is wrong. I know it doesn't do anyting yet, but I Think you meant env = chromium.ChromiumPort.setup_environ_for_server, no?

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:54
+ env = os.environ
This line wouldn't be needed if the above was fixed.

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:55
+ env['PATH'] = '%s;%s' % (
Not a big deal at all, but I think the conciseness was to standardize on " over '. I don't really care which we use so long as we try to pick one. :)

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:62
+ if (sys.platform == 'win32' and self._options and
We need to move away from this hasattr pattern and towards one where the options are closer to the code at hand. A genric "options" element is a bad design. Code which uses certain optiosn should always require those to be passed.

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:68
+ subprocess.Popen(setup_mount).wait()
Why not Executive.run_command()?

WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:51
+ class WebKitOptions(object):
The ports themselves should expose what options they require. But we'll fix that globally at some later date.

WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:139
+ if __name__ == '__main__':
I don't think these are generally used in our unittest setup.

Certainly looks better than the code that was there before. r- for the env = chromium.ChromiumPort.setup_environ_for_server() issue, all the rest are nits.

--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

bugzill...@webkit.org

unread,

May 17, 2010, 1:10:15 AM5/17/10

to webkit-bo...@googlegroups.com

bugzill...@webkit.org

unread,

May 17, 2010, 1:10:23 AM5/17/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=37664

Fumitoshi Ukai <uk...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------

Attachment #56210| |review?
Flag| |

--- Comment #35 from Fumitoshi Ukai <uk...@chromium.org> 2010-05-16 22:10:21 PST ---
Created an attachment (id=56210)
--> (https://bugs.webkit.org/attachment.cgi?id=56210)
Patch

bugzill...@webkit.org

unread,

May 17, 2010, 2:59:55 AM5/17/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=37664

Eric Seidel <er...@webkit.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #56210|review? |review+
Flag| |

--- Comment #36 from Eric Seidel <er...@webkit.org> 2010-05-16 23:59:52 PST ---
(From update of attachment 56210)
I'm uncertain if we should be returning a copy of os.environ or not.
426 return os.environ

Your change does not change behavior though, so it's fine.

bugzill...@webkit.org

unread,

May 17, 2010, 3:07:13 AM5/17/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=37664

Fumitoshi Ukai <uk...@chromium.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|REOPENED |RESOLVED
Resolution| |FIXED

--- Comment #37 from Fumitoshi Ukai <uk...@chromium.org> 2010-05-17 00:07:10 PST ---
Committed r59595: <http://trac.webkit.org/changeset/59595>

Reply all

Reply to author

Forward