Bug 131871 – Enable snapshot tests on mac wk2

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Roger Fong]

I'm currently working on enabling these tests, modifying each test to use the new API and adjusting the expected results.
I've run into an issue with the autoplay tests however.

When I run the test via layout-tests the primary plugin is not found.
However, when I just load the test in the browser, the primary plugin is found as expected...

[Roger Fong]

If i set the maximum number of snapshot attempts to 1 the test passes.

[Roger Fong]

After some extensive logging I believe that the following is happening:

The plugin is initialized before the primary plugin has been determined.
If the maximum number of snapshot retries is 0 then we immediately set display state to DisplayingSnapshot.

Now we hit HTMLPlugInImageElement::createElementRenderer
and hit
    if (displayState() == DisplayingSnapshot) {
        auto renderSnapshottedPlugIn = createRenderer<RenderSnapshottedPlugIn>(*this, std::move(style));
        renderSnapshottedPlugIn->updateSnapshot(m_snapshotImage);
        return std::move(renderSnapshottedPlugIn);
    }

We then get into determining the primary plugin, which is where (if a candidate plugin is found) we will restart the plugin appropriately.

However, the hit test fails, presumably due to the code that we hit earlier.
Commenting out the code block above causes us to fallback to creating a RenderImage renderer, which seems to fair better with hit testing for some reason.

Perhaps we should not create the element renderer until the plugin has been initialized, since we should know the initial state of the plugin anyways before deciding what type of renderer to create.

Or we shouldn't set the display state to DisplayingSnapshot at all until we know for sure the plugin is meant to be snapshotted (defer the setting of the display state until the plugin has been initialized)

[Roger Fong]

The dimension of the plugin element in question comes out to 139 by 18, the testing spot is 300, 300.

The actual width and height is 600 by 600.

[Roger Fong]

After diving down into the rabbit hole that is layout code, I've become thoroughly convinced that we should never be snapshotting a plugin before we determine whether or not it is the primary one.

Snapshotting a plugin causes all sorts of problems to arise. In this particular case it seems like we run into RenderFlexibleBox layout code which sets the logical override dimensions of the pluginImageElement.
However, these dimensions are not the dimensions of the original plugin element. 

I am not savvy enough with layout code to know what's going on here but it seems fundamentally wrong to try to hittest with something that isn't what we originally intended to hit test against in the first place.

[Roger Fong]

<rdar://problem/14395065>

[Roger Fong]

Created attachment 230467 [details]
patch

[Roger Fong]

500 ms didn't seem to be enough for the layout tests to reliably go thru the hit testing and snapshotting process, I gave each a full second to do so and haven't managed to get any failing results yet.

[Simon Fraser (smfr)]

Comment on attachment 230467 [details]
patch

one second per test seems way too long.

[Darin Adler]

Comment on attachment 230467 [details]
patch

All these tests say window.internals, but they could just say internals.

It’s really bad that these tests run so slowly and that they are inherently subject to race conditions. It’s not good to have multiple tests that require waiting 1 second no matter how fast the computer is or how heavily loaded it is.

[Roger Fong]

(In reply to comment #8)
> (From update of attachment 230467 [details])
> one second per test seems way too long.

I at first thought this was because of the timer delays, but looking at them now the delay for primary snapshot detection is a not so whopping 3ms.

I also noticed that quicktime only takes 500 ms. The quicktime plugin is also quite simple, just an orange background.

the lines.swf test file we use is a bit more complicated, so maybe it'll be better if we use a simpler plugin

[Roger Fong]

time1: 340397
time2: 341137

clock() call between subframeLoaderWillInitializePlugin and subframeLoaderDidInitializePlugin.

Looks like I could get away with a little less than 1000 ms, but that doesn't really solve the issue does it.

So basically it seems like the plugin takes a while to load fully.
Dean says there's not much that we can do about that.

I'm going to enable these tests to see how they fare on the bots.

[Roger Fong]

(In reply to comment #11)
> time1: 340397
> time2: 341137

guess those are ticks huh.

[Roger Fong]

http://trac.webkit.org/changeset/168048

Keeping this bug open to observe how the bots do.

[Roger Fong]

Didn't notice some earlier mistakes I made.
Fixes are here:

http://trac.webkit.org/changeset/169173

Now the tests should actually run and we can see how flakey they are.

[Alexey Proskuryakov]

These tests are failing every time on WebKit2.

  plugins/snapshotting/set-plugin-size-to-tiny.html [ Failure ]
  plugins/snapshotting/simple.html [ Failure ]
  plugins/snapshotting/snapshot-plugin-not-quite-blocked-by-image.html [ Failure ]

[Alexey Proskuryakov]

These tests are still broken. Is there any reason why it is ok to make everyone's work harder for so long?

EWS takes a really long time to finish, and even lies sometimes because of this.

[Roger Fong]

(In reply to comment #16)
> These tests are still broken. Is there any reason why it is ok to make everyone's work harder for so long?
> 
> EWS takes a really long time to finish, and even lies sometimes because of this.

Skipped https://bugs.webkit.org/show_bug.cgi?id=133227 for now.