Bug 63532 – DRT: Make Hixie76WebSocketProtocolEnabled preference flag configurable from LayoutTestController

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yuta Kitamura]

Add setHixie76WebSocketProtocolEnabled() method to LayoutTestController in order to switch WebSocket protocols in layout tests.

[Yuta Kitamura]

Created attachment 98915 [details]
Patch

[Alexey Proskuryakov]

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.

[Adam Barth]

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.

[Yuta Kitamura]

(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.

[Yuta Kitamura]

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

[Yuta Kitamura]

Created attachment 99060 [details]
Patch v2

[Kent Tamura]

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()?

[Yuta Kitamura]

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.

[Kent Tamura]

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.

[WebKit Review Bot]

Comment on attachment 99060 [details]
Patch v2

Clearing flags on attachment: 99060

Committed r90085: <http://trac.webkit.org/changeset/90085>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.