Bug 91008 – Web Inspector: Override the DeviceOrientation

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Konrad Piascik]

Allow web inspector to override the values recieved by deviceOrientation events.

[Konrad Piascik]

Created attachment 151944 [details]
Patch

[Rob Buis]

Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

Looks good, some stuff to fix.

> Source/WebCore/inspector/InspectorInstrumentation.h:262
> +    static DeviceOrientation* overridenDeviceOrientation(Page*, DeviceOrientation*);

Have you considered wrapping the whole function in #if ENABLE(INSPECTOR)?

> Source/WebCore/inspector/InspectorInstrumentation.h:1369
> +inline DeviceOrientation* InspectorInstrumentation::overridenDeviceOrientation(Page* page, DeviceOrientation* deviceOrientation)

You probably mean overrideDeviceOrientation, or overriddenDeviceOrientation?

> LayoutTests/ChangeLog:10
> +        * inspector/device-orientation-success.html: Added.

No expected result?

> LayoutTests/inspector/device-orientation-success.html:26
> +        function callbackComplete()

Indenting!

[Konrad Piascik]

Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

>> Source/WebCore/inspector/InspectorInstrumentation.h:262
>> +    static DeviceOrientation* overridenDeviceOrientation(Page*, DeviceOrientation*);
> 
> Have you considered wrapping the whole function in #if ENABLE(INSPECTOR)?

This is not the convention of this class.  The Impl functions are wrapped in this enable but the others are not.

>> Source/WebCore/inspector/InspectorInstrumentation.h:1369
>> +inline DeviceOrientation* InspectorInstrumentation::overridenDeviceOrientation(Page* page, DeviceOrientation* deviceOrientation)
> 
> You probably mean overrideDeviceOrientation, or overriddenDeviceOrientation?

will fix.

>> LayoutTests/ChangeLog:10
>> +        * inspector/device-orientation-success.html: Added.
> 
> No expected result?

nope I don't have a way to generate them.

>> LayoutTests/inspector/device-orientation-success.html:26
>> +        function callbackComplete()
> 
> Indenting!

fixing.

[Pavel Feldman]

Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

Looks good overall, couple of comments inline.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
> +    return deviceOrientation;

Who owns this object?

>>> Source/WebCore/inspector/InspectorInstrumentation.h:1369
>>> +inline DeviceOrientation* InspectorInstrumentation::overridenDeviceOrientation(Page* page, DeviceOrientation* deviceOrientation)
>> 
>> You probably mean overrideDeviceOrientation, or overriddenDeviceOrientation?
> 
> will fix.

I'd prefer overrideDeviceOrientation

> Source/WebCore/inspector/front-end/SettingsScreen.js:693
> +    _createDeviceOrientationOverrideControl: function()

A screenshot please

> Source/WebCore/inspector/front-end/SettingsScreen.js:695
> +        const deviceOrientationSetting = WebInspector.settings.deviceOrientationOverride.get();

var

> Source/WebCore/inspector/front-end/SettingsScreen.js:727
> +            this._deviceOrientationSectionElement.addStyleClass("hidden");

Please don't hide control - disable them instead.

> Source/WebCore/inspector/front-end/SettingsScreen.js:728
> +            PageAgent.clearDeviceOrientationData();

Should be done via UA

>>> LayoutTests/ChangeLog:10
>>> +        * inspector/device-orientation-success.html: Added.
>> 
>> No expected result?
> 
> nope I don't have a way to generate them.

What do you mean you can't generate them?

[Konrad Piascik]

Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
>> +    return deviceOrientation;
> 
> Who owns this object?

This is passed in from DeviceOrientationController::didChangeDeviceOrientation()

>> Source/WebCore/inspector/front-end/SettingsScreen.js:693
>> +    _createDeviceOrientationOverrideControl: function()
> 
> A screenshot please

will attach

>> Source/WebCore/inspector/front-end/SettingsScreen.js:695
>> +        const deviceOrientationSetting = WebInspector.settings.deviceOrientationOverride.get();
> 
> var

ok

>> Source/WebCore/inspector/front-end/SettingsScreen.js:727
>> +            this._deviceOrientationSectionElement.addStyleClass("hidden");
> 
> Please don't hide control - disable them instead.

will disable instead

[Pavel Feldman]

(In reply to comment #5)
> (From update of attachment 151944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review
> 
> >> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
> >> +    return deviceOrientation;
> > 
> > Who owns this object?
> 
> This is passed in from DeviceOrientationController::didChangeDeviceOrientation()
> 

My point is that it is not clear whether this object is ref counted from the method signature and if it is not, this is likely a memory problem.

[Konrad Piascik]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 151944 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review
> > 
> > >> Source/WebCore/inspector/InspectorInstrumentation.cpp:1165
> > >> +    return deviceOrientation;
> > > 
> > > Who owns this object?
> > 
> > This is passed in from DeviceOrientationController::didChangeDeviceOrientation()
> > 
> 
> My point is that it is not clear whether this object is ref counted from the method signature and if it is not, this is likely a memory problem.

Yes it is refCounted.

[Konrad Piascik]

Comment on attachment 151944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151944&action=review

>>>> LayoutTests/ChangeLog:10
>>>> +        * inspector/device-orientation-success.html: Added.
>>> 
>>> No expected result?
>> 
>> nope I don't have a way to generate them.
> 
> What do you mean you can't generate them?

I'm having difficulty setting up chromium on linux and blackberry doesn't currently support running inspector tests.  Qt doesn't support deviceOrientation and neither does mac build.  I can try setting up chrome on mac and see if that works.

[Konrad Piascik]

Created attachment 152026 [details]
Screenshot

[Konrad Piascik]

Created attachment 152044 [details]
Patch

[Pavel Feldman]

Comment on attachment 152044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152044&action=review

> Source/WebCore/inspector/Inspector.json:383
> +                "name": "setDeviceOrientationData",

setDeviceOrientationOverride

Not all backends will support such capability. You should add canOverrideDeviceOrientation capability (see the device metrics example).

> Source/WebCore/inspector/Inspector.json:389
> +                ]

This method should be marked as hidden.

> Source/WebCore/inspector/Inspector.json:393
> +                "description": "clears the overridden DeviceOrientation."

clearDeviceOrientationOverride, also should be hidden.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1202
> +DeviceOrientationData* InspectorInstrumentation::overrideDeviceOrientationImpl(InstrumentingAgents* instrumentingAgents, DeviceOrientationData* deviceOrientationData)

Could you declare orientation data as PassRefPtr?

> Source/WebCore/inspector/InspectorInstrumentation.h:1413
> +inline DeviceOrientationData* InspectorInstrumentation::overrideDeviceOrientation(Page* page, DeviceOrientationData* deviceOrientationData)

PassRefPtr ?

> Source/WebCore/inspector/front-end/SettingsScreen.js:855
> +            element.addEventListener("blur", this._applyDeviceOrientationUserInput.bind(this), false);

You want to align input value to the right.

[Konrad Piascik]

Created attachment 154071 [details]
Patch

[Konrad Piascik]

Created attachment 154113 [details]
Patch

[Konrad Piascik]

Created attachment 154129 [details]
Patch

[Early Warning System Bot]

Comment on attachment 154129 [details]
Patch

Attachment 154129 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13338361

[WebKit Review Bot]

Comment on attachment 154129 [details]
Patch

Attachment 154129 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13331460

[Konrad Piascik]

Created attachment 154132 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154132 [details]
Patch

Attachment 154132 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13335415

New failing tests:
fast/forms/range/slider-onchange-event.html
fast/forms/range/slider-mouse-events.html
fast/forms/range/slider-delete-while-dragging-thumb.html

[WebKit Review Bot]

Created attachment 154137 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Konrad Piascik]

(In reply to comment #18)
> (From update of attachment 154132 [details])
> Attachment 154132 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13335415
> 
> New failing tests:
> fast/forms/range/slider-onchange-event.html
> fast/forms/range/slider-mouse-events.html
> fast/forms/range/slider-delete-while-dragging-thumb.html

These failures are not related to any code that I've touched and I don't think they're caused by my changes.

[Pavel Feldman]

Comment on attachment 154132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154132&action=review

Overall looks good.

> Source/WebCore/inspector/Inspector.json:355
> +                "name": "setDeviceOrientationData",

setDeviceOrientationOverride

> Source/WebCore/inspector/Inspector.json:365
> +                "name": "clearDeviceOrientationData",

clearDeviceOrientationOverride

> Source/WebCore/inspector/front-end/SettingsScreen.js:352
> +    if (Capabilities.canOverrideDeviceOrientation)

Lets put it behind experiment and check for WebInspector.experimentsSettings.deviceLocationAndOrientation.isEnabled() (same applies for device location change).

> Source/WebCore/inspector/front-end/SettingsScreen.js:661
> +        const p = document.createElement("p");

It looks very similar to the device location / metrics. Could we extract this checkbox construction into a method?

> Source/WebCore/inspector/front-end/SettingsScreen.js:737
> +            var element = parentElement.createChild("input");

Please use CSS for styling.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:180
> +        return (this._alpha || this._alpha == 0) && (this._beta || this._beta == 0) && (this._gamma || this._gamma == 0) ? this._alpha + "," + this._beta + "," + this._gamma : "";

Why not JSON.parse / JSON.stringify ?

> Source/WebCore/inspector/front-end/UserAgentSupport.js:206
> +        return /^[-]?[0-9]*[.]?[0-9]*$/.test(value);

nit: It is better to prohibit non-digit input in the text field.

> LayoutTests/inspector/device-orientation-success.html:32
> +    InspectorTest.evaluateInPage("var handler = function(evt) {console.log('alpha: ' + evt.alpha + 'beta: ' + evt.beta + 'gamma: ' + evt.gamma);}, false);", setupHandler);

This function can be declared in the inline script above.

[Konrad Piascik]

Created attachment 154771 [details]
Patch

[Konrad Piascik]

Comment on attachment 154132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154132&action=review

>> Source/WebCore/inspector/front-end/UserAgentSupport.js:206
>> +        return /^[-]?[0-9]*[.]?[0-9]*$/.test(value);
> 
> nit: It is better to prohibit non-digit input in the text field.

How do you want to do that?  Is there an example in the code already?

[Konrad Piascik]

Created attachment 154923 [details]
Patch

[Build Bot]

Comment on attachment 154923 [details]
Patch

Attachment 154923 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13374395

[WebKit Review Bot]

Comment on attachment 154923 [details]
Patch

Attachment 154923 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13362717

New failing tests:
inspector/device-orientation-success.html

[WebKit Review Bot]

Created attachment 154949 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Konrad Piascik]

(In reply to comment #26)
> (From update of attachment 154923 [details])
> Attachment 154923 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13362717
> 
> New failing tests:
> inspector/device-orientation-success.html

Error in my test re-submitting.

[Konrad Piascik]

Created attachment 154961 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154961 [details]
Patch

Attachment 154961 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13367723

New failing tests:
inspector/device-orientation-success.html

[WebKit Review Bot]

Created attachment 154977 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pavel Feldman]

Comment on attachment 154961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154961&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:1040
> +        return;

Please report an error here.

> Source/WebCore/inspector/front-end/SettingsScreen.js:512
> +    _createInput: function(parentElement, id, defaultText, eventListener)

Please annotate new methods with closure.

> Source/WebCore/inspector/front-end/SettingsScreen.js:517
> +        element.style.width = "80px";

Please use CSS for this one.

> Source/WebCore/inspector/front-end/SettingsScreen.js:762
> +        const p = document.createElement("p");

Please do not use "const" for variables.
Also, sounds like _createCheckbox. Or even better, you could create a "section" object with the checkbox that would automatically enable / disable a list of given controls.

> Source/WebCore/inspector/front-end/SettingsScreen.js:783
> +        var controlsDisabled = !this._deviceOrientationOverrideCheckboxElement.checked;

I.e. automate this code once for all.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:240
> +    this._alpha = alpha;

You want to make them public so that JSON.stringify created a nice string.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:270
> +WebInspector.UserAgentSupport.DeviceOrientation.parseUserInput = function(alphaString, betaString, gammaString)

It is really close to WebInspector.UserAgentSupport.GeolocationPosition.parseUserInput. Could you do:
parseUserInputs(propertyNames, inputValues) where both are arrays and you would return JSON object? You would not need WebInspector.UserAgentSupport.DeviceOrientation and WebInspector.UserAgentSupport.GeolocationPosition then. Loads of boilerplate code does not make front-end code pretty.

> Source/WebCore/inspector/front-end/UserAgentSupport.js:272
> +    function isUserInputValid(value)

I'm sure I've already seen this method.

[Konrad Piascik]

Comment on attachment 154961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154961&action=review

I understand your desire for refactor and I agree, but I would prefer to land this then refactor all the code at once.  From what I see there are 3 changes that I could lump into a single refactor. 1) CSS for the inputs 2) checkbox for enabling the feature/disabling input 3) parse user input.  I can do all of this, but would rather it be a separate bug.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:1040
>> +        return;
> 
> Please report an error here.

will do

>> Source/WebCore/inspector/front-end/SettingsScreen.js:512
>> +    _createInput: function(parentElement, id, defaultText, eventListener)
> 
> Please annotate new methods with closure.

ok

>> Source/WebCore/inspector/front-end/SettingsScreen.js:762
>> +        const p = document.createElement("p");
> 
> Please do not use "const" for variables.
> Also, sounds like _createCheckbox. Or even better, you could create a "section" object with the checkbox that would automatically enable / disable a list of given controls.

will change const to var where appropriate.

>> Source/WebCore/inspector/front-end/UserAgentSupport.js:240
>> +    this._alpha = alpha;
> 
> You want to make them public so that JSON.stringify created a nice string.

they are public _ does not denote that a variable is private.

[Pavel Feldman]

> they are public _ does not denote that a variable is private.

But it makes JSON.stringify of it look ugly and that's the value that is stored in the settings. We should not save internal representations, we should make sure that persistence is nice. Declaring them as private is not that important here.

[Pavel Feldman]

> But it makes JSON.stringify of it look ugly and that's the value that is stored in the settings. We should not save internal representations, we should make sure that persistence is nice. Declaring them as private is not that important here.

... i.e. whoever edits the code after you feels free to rename _foo to _bar and he does not realize that it breaks the persistence compatibility.

[Konrad Piascik]

Created attachment 155378 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155378 [details]
Patch

Attachment 155378 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13385686

New failing tests:
inspector/device-orientation-success.html

[WebKit Review Bot]

Created attachment 155394 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Konrad Piascik]

Created attachment 155584 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 155584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155584&action=review

It would be awesome if this feature could play together with the "Device Metrics Emulation". Meaning, if the device gets rotated from portrait to landscape (or vice versa), this would also get reflected in the @media (orientation: ...) evaluation results.

> Source/WebCore/inspector/Inspector.json:388
> +                "name": "clearDeviceOrientationOverride",

I seem to have seen a similar discussion elsewhere (perhaps, on the Geolocation mocking patch), mentioning that we usually clear the override by providing out-of-domain values to a respective set...Override() method (e.g. a screen resolution of 0x0). What was the consensus?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1221
> +DeviceOrientationData* InspectorInstrumentation::overrideDeviceOrientationImpl(InstrumentingAgents* instrumentingAgents, DeviceOrientationData* deviceOrientation)

The usual way to override something by Instrumentation is to modify an in-out parameter (see InspectorInstrumentation::apply[ScreenWidth|ScreenHeight|UserAgent]Override() and the agent methods they invoke).

> Source/WebCore/inspector/InspectorPageAgent.cpp:1041
> +        *error = "Internal error: unable to override device orientaiton.";

Typo: "orientaITon" -> "orientation"

> Source/WebCore/inspector/InspectorPageAgent.cpp:1046
> +    clearDeviceOrientationOverride(&clearError);

No need to clear the current override first. Since m_deviceOrientation is a RefPtr, its contents will get deref'ed (and possibly destroyed) just fine when a new value is assigned to it.

> Source/WebCore/inspector/InspectorPageAgent.cpp:1068
> +    if (m_deviceOrientation)

The same idea about the apply...Override() approach holds here.

> Source/WebCore/inspector/front-end/SettingsScreen.js:845
> +

Too many blank lines

> Source/WebCore/inspector/front-end/SettingsScreen.js:850
> +        cellElement.appendChild(document.createTextNode("\u03B1: "));

I have no good ideas about this, but it would be nice to have some kind of a (graphical?) hint next to the fields, for the user to see which of the angles is alpha, beta, and gamma. If have something to say, please speak up! :) At the very least, we can implement it later.

> LayoutTests/inspector/device-orientation-success.html:25
> +        InspectorTest.evaluateInPage("window.removeEventListener('deviceorientation', handler, false);", callbackComplete);

You don't seem to need to remove the listener, as it will not impact the further test flow (and there will be a new window object for the next test, right?). Just invoke completeTest() at this point.

> LayoutTests/inspector/device-orientation-success.html:27
> +        function callbackComplete()

Bad indentation. This also needs a blank line above

[Pavel Feldman]

Comment on attachment 155584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155584&action=review

Alexander, I think it would make sense for me to complete this review. No need to jump into the process on the 10th patch revision.

>> Source/WebCore/inspector/Inspector.json:388
>> +                "name": "clearDeviceOrientationOverride",
> 
> I seem to have seen a similar discussion elsewhere (perhaps, on the Geolocation mocking patch), mentioning that we usually clear the override by providing out-of-domain values to a respective set...Override() method (e.g. a screen resolution of 0x0). What was the consensus?

The consensus was that we do explicit clear method.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1221
>> +DeviceOrientationData* InspectorInstrumentation::overrideDeviceOrientationImpl(InstrumentingAgents* instrumentingAgents, DeviceOrientationData* deviceOrientation)
> 
> The usual way to override something by Instrumentation is to modify an in-out parameter (see InspectorInstrumentation::apply[ScreenWidth|ScreenHeight|UserAgent]Override() and the agent methods they invoke).

I disagree, I like it this way. If you have multiple values to override, you do in-out parameters.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:1068
>> +    if (m_deviceOrientation)
> 
> The same idea about the apply...Override() approach holds here.

@apavlov, I think "apply" is superfluous

[Pavel Feldman]

Comment on attachment 155584 [details]
Patch

Restoring r? so that I saw it at the dashboard.

[Konrad Piascik]

View in context: https://bugs.webkit.org/attachment.cgi?id=155584&action=review

>> Source/WebCore/inspector/front-end/SettingsScreen.js:845
>> +
> 
> Too many blank lines

will remove.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:850
>> +        cellElement.appendChild(document.createTextNode("\u03B1: "));
> 
> I have no good ideas about this, but it would be nice to have some kind of a (graphical?) hint next to the fields, for the user to see which of the angles is alpha, beta, and gamma. If have something to say, please speak up! :) At the very least, we can implement it later.

I do have a really good idea of how to implement this.  Take a look at the Ripple emulator and how they do device orientation.  It's an open source project that I want to take code from and re-use in Web Inspector.  I was planning on doing this as a follow up patch.

[Konrad Piascik]

Created attachment 155795 [details]
Patch

Fixed type, removed blank line and updated test to use fewer callbacks.

[WebKit Review Bot]

Comment on attachment 155795 [details]
Patch

Clearing flags on attachment: 155795

Committed r124484: <http://trac.webkit.org/changeset/124484>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch has been failing on Mac:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=inspector%2Fdevice-orientation-success.html


--- /Volumes/Data/slave/lion-debug-tests-wk1/build/layout-test-results/inspector/device-orientation-success-expected.txt
+++ /Volumes/Data/slave/lion-debug-tests-wk1/build/layout-test-results/inspector/device-orientation-success-actual.txt
@@ -1,3 +1,1 @@
-CONSOLE MESSAGE: line 17: alpha: 20 beta: 30 gamma: 40
-CONSOLE MESSAGE: line 17: alpha: 1.1 beta: 2.2 gamma: 3.3

[Pavel Feldman]

@Konrad Piascik: are you following up on the test failure? We might need to roll this change out in case it fails tests consistently.

[Konrad Piascik]

(In reply to comment #48)
> @Konrad Piascik: are you following up on the test failure? We might need to roll this change out in case it fails tests consistently.

I can't investigate this until tuesday, but think that it should be skipped, if this is urgent.