619176 - Plugins get Visible state every time when scrolling (:BuildLayer always make them visible)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Oleg Romashin (:romaxa)]

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...

Comment 1

[Benjamin Smedberg]

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

Comment 2

[Oleg Romashin (:romaxa)]

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

Comment 3

[Benjamin Smedberg]

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

Comment 4

[Oleg Romashin (:romaxa)]

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

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Possible fix

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 11

[Benjamin Smedberg]

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...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

See comments

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: Suggesting to remove UpdateWindowVisibility from BuildDisplayList

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

Comment 16

[Oleg Romashin (:romaxa)]

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 17

[Oleg Romashin (:romaxa)]

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...

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Just call setWindow without changes

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

Comment 19

[Timothy Nikkel (:tnikkel)]

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.

Comment 20

[Oleg Romashin (:romaxa)]

> 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

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: Call Visibility update only for reftests.

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

Comment 22

[Oleg Romashin (:romaxa)]

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

Blocking blocker

Comment 23

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/b9029c71a63a