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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

(Depends on 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

9 years ago
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

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

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

9 years ago
Posted 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.

Comment 9

9 years ago
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.
Assignee

Updated

9 years ago
Blocks: 619056
Comment on attachment 497623 [details] [diff] [review]
Possible fix

See comments
Attachment #497623 - Flags: review?(roc) → review-
Assignee

Updated

9 years ago
Duplicate of this bug: 621229
Assignee

Comment 15

9 years ago
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)
Assignee

Comment 16

9 years ago
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?
Assignee

Comment 17

9 years ago
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?
Assignee

Comment 18

9 years ago
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
Assignee

Comment 20

9 years ago
> 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
Assignee

Comment 21

9 years ago
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)
Assignee

Comment 22

9 years ago
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+
Assignee

Comment 23

9 years ago
http://hg.mozilla.org/mozilla-central/rev/b9029c71a63a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 625651

Updated

8 years ago
Depends on: 647139

Updated

8 years ago
Depends on: 648277

Updated

8 years ago
Depends on: 685768

Updated

7 years ago
Depends on: 732892
You need to log in before you can comment on or make changes to this bug.