Bug 63532

Created attachment 98915 [details] Patch

Comment on attachment 98915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98915&action=review Going forward, this really needs a WebKit2 implementation, too. > Tools/DumpRenderTree/LayoutTestController.cpp:2139 > + if (argumentCount == 1) > + controller->setHixie76WebSocketProtocolEnabled(JSValueToBoolean(context, arguments[0])); A function with this name could enable it when called with no arguments, too. That could save someone a few minutes of confusion some day.

Comment on attachment 98915 [details] Patch There's already a general LayoutTestController method for setting preferences. We don't need a one-off for each one.

(In reply to comment #3) > (From update of attachment 98915 [details]) > There's already a general LayoutTestController method for setting preferences. We don't need a one-off for each one. Sure, I will use LayoutTestController::overridePreference(). With this function, almost all the code of the above patch has become unnecessary, but we still need to reset the preference value in the beginning of each test run. The patch will become reasonably small, so I think I can do Chromium part in the same patch as well as mac/win.

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

Created attachment 99060 [details] Patch v2

Comment on attachment 99060 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=99060&action=review > Tools/DumpRenderTree/chromium/WebPreferences.cpp:113 > + hixie76WebSocketProtocolEnabled = true; Is the default value true? What's your testing plan? Will we have tests for the current protocol and tests for the new protocol? What tests should call overridePreference()?

Comment on attachment 99060 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=99060&action=review >> Tools/DumpRenderTree/chromium/WebPreferences.cpp:113 >> + hixie76WebSocketProtocolEnabled = true; > > Is the default value true? > > What's your testing plan? Will we have tests for the current protocol and tests for the new protocol? What tests should call overridePreference()? At least for now, I want to make the default value true to keep existing tests as-is (note that Hixie76 is an old protocol, so this means "we use the old protocol by default.") I'm going to update WebSocket tests in the following way: 1) Move existing tests (using the old hixie76 protocol) under a new directory (say, websocket/tests/hixie76). 2) Put tests for the new protocol. Each test should have a line "overridePreference("WebKitHixie76WebSocketProtocolEnabled", 0)" in the beginning. We can copy most tests from hixie76 tests, but some tests will need manual modifications, hence they will diverge from hixie76 tests. 3) Make changes... 4) When we are sure about flipping default to the new protocol, put "overridePreference("WebKitHixie76WebSocketProtocolEnabled", 1)" to every hixie76 tests, and flip the default value. 5) When we are sure about dropping support for the old protocol, remove tests for the old protocol, Settings item, "overridePreference()" calls, etc. So my answers to your last two questions are: - Meanwhile, we will have to maintain two sets of tests: one for the old protocol, and one for the new protocol. I expect this period will not last long. - Meanwhile, only new tests should call overridePreference() (because the old protocol is default). This will change as the default changes.

Comment on attachment 99060 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=99060&action=review >>> Tools/DumpRenderTree/chromium/WebPreferences.cpp:113 >>> + hixie76WebSocketProtocolEnabled = true; >> >> Is the default value true? >> >> What's your testing plan? Will we have tests for the current protocol and tests for the new protocol? What tests should call overridePreference()? > > At least for now, I want to make the default value true to keep existing tests as-is (note that Hixie76 is an old protocol, so this means "we use the old protocol by default.") > > I'm going to update WebSocket tests in the following way: > 1) Move existing tests (using the old hixie76 protocol) under a new directory (say, websocket/tests/hixie76). > 2) Put tests for the new protocol. Each test should have a line "overridePreference("WebKitHixie76WebSocketProtocolEnabled", 0)" in the beginning. We can copy most tests from hixie76 tests, but some tests will need manual modifications, hence they will diverge from hixie76 tests. > 3) Make changes... > 4) When we are sure about flipping default to the new protocol, put "overridePreference("WebKitHixie76WebSocketProtocolEnabled", 1)" to every hixie76 tests, and flip the default value. > 5) When we are sure about dropping support for the old protocol, remove tests for the old protocol, Settings item, "overridePreference()" calls, etc. > > So my answers to your last two questions are: > - Meanwhile, we will have to maintain two sets of tests: one for the old protocol, and one for the new protocol. I expect this period will not last long. > - Meanwhile, only new tests should call overridePreference() (because the old protocol is default). This will change as the default changes. ok, I understand. The initial value of Settings:m_useHixie76WebSocketProtocol() is also true. Resetting to true is reasonable.

Comment on attachment 99060 [details] Patch v2 Clearing flags on attachment: 99060 Committed r90085: <http://trac.webkit.org/changeset/90085>

All reviewed patches have been landed. Closing bug.