Closed Bug 1115437 Opened 8 years ago Closed 8 years ago

Update tests to handle asynchronous plugin initialization

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox38 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
firefox38 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(2 files, 1 obsolete file)

Part 1: Plugin reftest updates
19.77 KB, patch
bugzilla
: review+
Details | Diff | Splinter Review
Part 2: Layout reftest updates
5.83 KB, patch
roc
: review+
Details | Diff | Splinter Review 
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: asyncplugininit
No longer blocks: 998863
Attached patch Test updates (obsolete) — — Splinter Review 

        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)

    
    

    
  


  
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+

    
    

    
  


  
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)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/afa28e98419f
https://hg.mozilla.org/mozilla-central/rev/850264d2c1c4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox38:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.