http://hg.mozilla.org/mozilla-central/rev/6ff3fcbb7845#l9.17 with this line we always set Plugin in "Visible" state... I think this is wrong... We should check rectangle and don't make plugins visible if rectangle out of the visible area...

Hrm, we build layers for frames which aren't on the screen? Then yes, we should check the rectangle.

Why do we need that call at all ? isn't it enough Update Widget Configuration call and compute visibility?

No: WidgetConfiguration happens after the paint sequence (BuildLayer) is complete, and so the visibility state is incorrect in some cases.

what is the right way to get visible state in BuildLayer? should I intersect "r" in nsObjectFrame::BuildLayer with some aManager->GetRoot->EffectiveVisibleRegion?

(In reply to comment #1) > Hrm, we build layers for frames which aren't on the screen? Then yes, we should > check the rectangle. We should not be building layers for frames which aren't on the screen.

So, I don't understand what the problem is here. If the plugin is scrolled offscreen, or even covered by opaque content, we should not be calling BuildLayer on it.

(In reply to comment #3) > No: WidgetConfiguration happens after the paint sequence (BuildLayer) is > complete, and so the visibility state is incorrect in some cases. That can happen, but it's only temporary, so I think it would be OK to report "visible" for slightly longer than necessary. So I think we could actually just remove the UpdateWindowVisibility call from BuildLayer.

Note that the current widget-configuration code doesn't ever make an invisible->visible switch, only visible->invisible. We currently rely on BuildLayer to set up the necessary pieces for a visible plugin. It would require some work to do the layer setup in WidgetConfiguration, because we don't know the layer-manager type, which is necessary to pass down to the plugin so that it creates the correct surface type.

(In reply to comment #9) > Note that the current widget-configuration code doesn't ever make an > invisible->visible switch, only visible->invisible. nsObjectFrame::ComputeWidgetGeometry: if (mInstanceOwner) { // UpdateWindowVisibility will notify the plugin of position changes // by updating the NPWindow and calling NPP_SetWindow/AsyncSetWindow. mInstanceOwner->UpdateWindowVisibility(!aRegion.IsEmpty()); } What am I missing?

Hrm. It may be that I changed this mid-development... or that when we first receive computewidgetgeometry that we don't have an instanceowner yet. But I remember that it was necessary to add the call to BuildLayer to pass some tests...

(In reply to comment #11) > Hrm. It may be that I changed this mid-development... or that when we first > receive computewidgetgeometry that we don't have an instanceowner yet. If that's what it is, we should stash the visibility state in the nsObjectFrame and set it when the instance owner is up and running.

Comment on attachment 497623 [details] [diff] [review] Possible fix See comments

We don't need visibility update in BuildDisplayList, it is enough to have nsObjectFrame::ComputeWidgetGeometry function.

Comment on attachment 502466 [details] [diff] [review] Suggesting to remove UpdateWindowVisibility from BuildDisplayList Pending Mozilla try build: http://hg.mozilla.org/try/rev/2e024a5cb13f

Comment on attachment 502466 [details] [diff] [review] Suggesting to remove UpdateWindowVisibility from BuildDisplayList hmm bad, this patch failed with this output: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1294668812.1294672815.8712.gz bunch of tests are failed with this message: 541406-1-ref.html | timed out waiting for pending paint count to reach zero (waiting for MozPaintWaitFinished) sounds like plugins are expecting UpdateWindowVisibility->SetWindow call here...

This should keep visibility in good shape and don't break tests

The latest patch seems to freeze the browser when I test scrolling for bug 622829. But the patch before it seems fine and fixes the performance problem of that bug.

> But the patch before it seems fine and fixes the performance problem of Ok, then I need to make sure that we don't call SetWindow all the time, and pass all tests with first patch

This seems what we want... it call Force Visibility true for reftests, and don't break real pages rendering and performance

Comment on attachment 502754 [details] [diff] [review] Call Visibility update only for reftests. Blocking blocker