Bug 63532 - DRT: Make Hixie76WebSocketProtocolEnabled preference flag configurable from LayoutTestController
Summary: DRT: Make Hixie76WebSocketProtocolEnabled preference flag configurable from L...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
: 63536 (view as bug list)
Depends on:
Blocks: 50099
  Show dependency treegraph
 
Reported: 2011-06-28 07:01 PDT by Yuta Kitamura
Modified: 2011-06-29 20:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.98 KB, patch)
2011-06-28 07:19 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (5.53 KB, patch)
2011-06-29 02:41 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-06-28 07:01:32 PDT
Add setHixie76WebSocketProtocolEnabled() method to LayoutTestController in order to switch WebSocket protocols in layout tests.
Comment 1 Yuta Kitamura 2011-06-28 07:19:34 PDT
Created attachment 98915 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-06-28 08:52:03 PDT
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 3 Adam Barth 2011-06-28 14:00:00 PDT
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.
Comment 4 Yuta Kitamura 2011-06-29 00:19:05 PDT
(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.
Comment 5 Yuta Kitamura 2011-06-29 01:04:24 PDT
*** Bug 63536 has been marked as a duplicate of this bug. ***
Comment 6 Yuta Kitamura 2011-06-29 02:41:28 PDT
Created attachment 99060 [details]
Patch v2
Comment 7 Kent Tamura 2011-06-29 02:50:58 PDT
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 8 Yuta Kitamura 2011-06-29 05:53:26 PDT
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 9 Kent Tamura 2011-06-29 18:22:59 PDT
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 10 WebKit Review Bot 2011-06-29 20:40:14 PDT
Comment on attachment 99060 [details]
Patch v2

Clearing flags on attachment: 99060

Committed r90085: <http://trac.webkit.org/changeset/90085>
Comment 11 WebKit Review Bot 2011-06-29 20:40:19 PDT
All reviewed patches have been landed.  Closing bug.