Update tests to handle asynchronous plugin initialization

RESOLVED FIXED in Firefox 38

Status

()

Core
Plug-ins
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

This is patch 15 from bug 998863. Address Georg's review comments in https://bugzilla.mozilla.org/show_bug.cgi?id=998863#c49 before landing.
Blocks: 1116806
No longer blocks: 998863
Duplicate of this bug: 1121158
Created attachment 8549819 [details] [diff] [review]
Test updates
Attachment #8549819 - Flags: review?(gfritzsche)
Comment on attachment 8549819 [details] [diff] [review]
Test updates

Review of attachment 8549819 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed and checking with a layout peer whether the layout/ changes are ok for them.

I think there are more test locations that cover plugins - see e.g. bug 899080 and bug 926830.
I assume they didn't show up on try - but maybe they will result in intermittent failures?

Also, this means all plugin tests will cover forced sync instantiation, right?
Can we feasibly have test coverage as well for the case where we don't force sync?

::: dom/plugins/test/mochitest/test_bug854082.html
@@ +33,1 @@
>      setterCalled = true;

This is redundant now.

::: dom/plugins/test/reftest/border-padding-3.html
@@ +3,5 @@
> +<head>
> +<script src="utils.js">
> +</script>
> +</head>
> +<body style="margin:0" onLoad="forceLoadPlugin('plugin', function(){})">

What is that empty function arg good for? It shouldn't do anything.

::: dom/plugins/test/reftest/shrink-1.html
@@ +16,5 @@
>  
>  document.addEventListener("MozReftestInvalidate", doShrink, false);
>    </script>
>  </head>
> +<body onLoad="forceLoadPlugin('plugin', function(){})">

Redundant function(){}?

::: dom/plugins/test/reftest/update-1.html
@@ +50,5 @@
>  // MozReftestInvalidate is delivered after initial painting has settled.
>  document.addEventListener("MozReftestInvalidate", start, false);
>    </script>
>  </head>
> +<body onLoad="forceLoadPlugin('plugin', function(){})">

Redundant function(){}?

::: layout/reftests/bugs/541406-1.html
@@ +6,5 @@
> +</head>
> +<body onload="forceLoadPlugin('p', function() {
> +  var p = document.getElementById('p');
> +  p.focus();
> +  document.documentElement.removeAttribute('class');

forceLoadPlugin() already does removeAttribute('class')

::: layout/reftests/bugs/580160-1-ref.html
@@ +3,5 @@
> +<head>
> +<script src="forceloadplugin.js">
> +</script>
> +</head>
> +<body onLoad="forceLoadPlugin('p', function(){})">

Redundant function(){}?
Attachment #8549819 - Flags: review?(gfritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Comment on attachment 8549819 [details] [diff] [review]
> Test updates
> 
> Review of attachment 8549819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed and checking with a layout peer whether the
> layout/ changes are ok for them.
> 
> I think there are more test locations that cover plugins - see e.g. bug
> 899080 and bug 926830.
> I assume they didn't show up on try - but maybe they will result in
> intermittent failures?
> 
> Also, this means all plugin tests will cover forced sync instantiation,
> right?
> Can we feasibly have test coverage as well for the case where we don't force
> sync?
> 

The main issue in this bug is reftests: the snapshot of the plugin can't be taken until we know that the plugin has finished initializing. In other test cases the timing of when the plugin actually paints is not as important. Most mochitests may be left as-is (including non-forced sync).


> ::: dom/plugins/test/mochitest/test_bug854082.html
> @@ +33,1 @@
> >      setterCalled = true;
> 
> This is redundant now.
> 

Right. I'll fix that.


> ::: dom/plugins/test/reftest/border-padding-3.html
> @@ +3,5 @@
> > +<head>
> > +<script src="utils.js">
> > +</script>
> > +</head>
> > +<body style="margin:0" onLoad="forceLoadPlugin('plugin', function(){})">
> 
> What is that empty function arg good for? It shouldn't do anything.

If you look at the implementation of forceLoadPlugin, if there is a function arg, it executes that instead of removing the class attribute. This is for special cases where the test already used that attribute and needs to execute other code instead. The empty function is essentially saying, "don't execute anything after the plugin initializes."

I agree that they syntax is kind of clumsy and non-intuitive. Perhaps modifying forceLoadPlugin to distinguish between undefined and null would be better?

> 
> ::: dom/plugins/test/reftest/shrink-1.html
> @@ +16,5 @@
> >  
> >  document.addEventListener("MozReftestInvalidate", doShrink, false);
> >    </script>
> >  </head>
> > +<body onLoad="forceLoadPlugin('plugin', function(){})">
> 
> Redundant function(){}?
> 
> ::: dom/plugins/test/reftest/update-1.html
> @@ +50,5 @@
> >  // MozReftestInvalidate is delivered after initial painting has settled.
> >  document.addEventListener("MozReftestInvalidate", start, false);
> >    </script>
> >  </head>
> > +<body onLoad="forceLoadPlugin('plugin', function(){})">
> 
> Redundant function(){}?
> 
> ::: layout/reftests/bugs/541406-1.html
> @@ +6,5 @@
> > +</head>
> > +<body onload="forceLoadPlugin('p', function() {
> > +  var p = document.getElementById('p');
> > +  p.focus();
> > +  document.documentElement.removeAttribute('class');
> 
> forceLoadPlugin() already does removeAttribute('class')
> 
> ::: layout/reftests/bugs/580160-1-ref.html
> @@ +3,5 @@
> > +<head>
> > +<script src="forceloadplugin.js">
> > +</script>
> > +</head>
> > +<body onLoad="forceLoadPlugin('p', function(){})">
> 
> Redundant function(){}?

My above explanation should cover these.
Flags: needinfo?(gfritzsche)
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #4)
> > I think there are more test locations that cover plugins - see e.g. bug
> > 899080 and bug 926830.
> > I assume they didn't show up on try - but maybe they will result in
> > intermittent failures?
> > 
> > Also, this means all plugin tests will cover forced sync instantiation,
> > right?
> > Can we feasibly have test coverage as well for the case where we don't force
> > sync?
> > 
> 
> The main issue in this bug is reftests: the snapshot of the plugin can't be
> taken until we know that the plugin has finished initializing. In other test
> cases the timing of when the plugin actually paints is not as important.
> Most mochitests may be left as-is (including non-forced sync).

Ah, good then.

> > ::: dom/plugins/test/reftest/border-padding-3.html
> > @@ +3,5 @@
> > > +<head>
> > > +<script src="utils.js">
> > > +</script>
> > > +</head>
> > > +<body style="margin:0" onLoad="forceLoadPlugin('plugin', function(){})">
> > 
> > What is that empty function arg good for? It shouldn't do anything.
> 
> If you look at the implementation of forceLoadPlugin, if there is a function
> arg, it executes that instead of removing the class attribute. This is for
> special cases where the test already used that attribute and needs to
> execute other code instead. The empty function is essentially saying, "don't
> execute anything after the plugin initializes."
> 
> I agree that they syntax is kind of clumsy and non-intuitive. Perhaps
> modifying forceLoadPlugin to distinguish between undefined and null would be
> better?

Ok, that is not clear from looking at the uses. I know, it's only tests, but can't we just use a bool arg for "remove class attribute" or something like that?
Flags: needinfo?(gfritzsche)
Created attachment 8560074 [details] [diff] [review]
Part 1: Plugin reftest updates

Modified to use a boolean in forceLoadPlugin as Georg suggested. I've separated this from the layout reftests which will go into part 2. In part 2 we will still need to pass a function but I've cleaned that up to accept a boolean in the general case.

Carrying forward r+.
Attachment #8549819 - Attachment is obsolete: true
Attachment #8560074 - Flags: review+
Created attachment 8560079 [details] [diff] [review]
Part 2: Layout reftest updates

When async plugin initialization is turned on, we need to force the plugin to finish initializing before we allow reftest to take its snapshot. These test updates make the layout reftests compatible.
Attachment #8560079 - Flags: review?(roc)
Attachment #8560079 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/afa28e98419f
https://hg.mozilla.org/integration/mozilla-inbound/rev/850264d2c1c4
https://hg.mozilla.org/mozilla-central/rev/afa28e98419f
https://hg.mozilla.org/mozilla-central/rev/850264d2c1c4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.