Closed
Bug 619176
Opened 14 years ago
Closed 14 years ago
Plugins get Visible state every time when scrolling (:BuildLayer always make them visible)
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: romaxa, Assigned: romaxa)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
1.07 KB,
patch
|
roc
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Hrm, we build layers for frames which aren't on the screen? Then yes, we should check the rectangle.
Assignee | ||
Comment 2•14 years ago
|
||
Why do we need that call at all ?
isn't it enough Update Widget Configuration call and compute visibility?
Comment 3•14 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•14 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•14 years ago
|
||
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•14 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?
Comment 11•14 years ago
|
||
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
Attachment #497623 -
Flags: review?(roc) → review-
Assignee | ||
Comment 15•14 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)
Attachment #502466 -
Flags: review?(roc) → review+
Assignee | ||
Comment 16•14 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•14 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•14 years ago
|
||
This should keep visibility in good shape and don't break tests
Attachment #502466 -
Attachment is obsolete: true
Comment 19•14 years ago
|
||
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.
Assignee | ||
Comment 20•14 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•14 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•14 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•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•