Closed Bug 619176 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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?
Attached patch Possible fix (obsolete) — Splinter Review
Attachment #497623 - Flags: review?(roc)
(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.
Blocks: 619056
Comment on attachment 497623 [details] [diff] [review]
Possible fix

See comments
Attachment #497623 - Flags: review?(roc) → review-
Duplicate of this bug: 621229
We don't need visibility update in BuildDisplayList, it is enough to have nsObjectFrame::ComputeWidgetGeometry function.
Assignee: nobody → romaxa
Attachment #497623 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #502466 - Flags: review?(roc)
Comment on attachment 502466 [details] [diff] [review]
Suggesting to remove UpdateWindowVisibility from BuildDisplayList

Pending Mozilla try build:
http://hg.mozilla.org/try/rev/2e024a5cb13f
Attachment #502466 - Flags: approval2.0?
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...
Attachment #502466 - Flags: review+
Attachment #502466 - Flags: approval2.0?
This should keep visibility in good shape and don't break tests
Attachment #502466 - Attachment is obsolete: true
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.
Blocks: 622829
> 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
Attachment #502519 - Attachment is obsolete: true
Attachment #502754 - Flags: review?(roc)
Comment on attachment 502754 [details] [diff] [review]
Call Visibility update only for reftests.

Blocking blocker
Attachment #502754 - Flags: approval2.0?
Attachment #502754 - Flags: review?(roc)
Attachment #502754 - Flags: review+
Attachment #502754 - Flags: approval2.0?
Attachment #502754 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/b9029c71a63a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 625651
Depends on: 647139
Depends on: 648277
Depends on: 685768
Depends on: 732892
You need to log in before you can comment on or make changes to this bug.