Closed Bug 370379 Opened 13 years ago Closed 13 years ago

Create views less frequently

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sharparrow1, Assigned: sharparrow1)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
Mostly an idea at this point; need to do more cleanup before this is finished.  Seems to work well; a surprisingly small number of changes are required.

This might affect scrolling performance in some cases; not really sure, though.  Also might have broken fixed backgrounds in certain cases.  Need to look more carefully

Probably should clean up nsBoxFrame::CreateViewForFrame as well.

Besides cleanup, layerX/layoutY and html:image's x and y probably need to be changed not to look for views.
Depends on: 370492
Attached patch Patch v1 (obsolete) — Splinter Review
(I'm going to get rid of the ChildrenMustHaveWidgets change in nsFrame.cpp. I doubt anyone actually uses non-boxes without views as children of decks, but it's still correct to check.)

There's a few different things going on in this patch; I'll separate the patches if you'd prefer, although it is a little inconvenient.

First off, there are some header changes which are trivial.

Secondly, there's some changes stemming off of making nsContainerFrame::FrameNeedsView only return true for frames which return true for NeedsView (scrollframes and object frames).  This means views are not automatically created for positioned frames and frames with opacity.  The changes needed to make this work are in nsAbsoluteContainingBlock.cpp and nsStyleStruct.cpp.  This is a pretty major change, but it seems to work fine (at least with the patch to bug 370492.)

Third, this patch gets rid of SetViewContentTransparency and SetViewOpacity.  The values are no longer used (due to display lists?), so it's very straightforward to just remove them.

There are also a few other small changes.
Attachment #255086 - Attachment is obsolete: true
I'm not quite ready to ask for review on this (I think it'll be better to separate it out), but thoughts?
This is good. I haven't gotten around to cleaning this stuff up after frame display lists made things obsolete.

I think some of it overlaps with the compositor patch I've been working on, but that's OK since that patch is far from done and getting this kind of cleanup landed can only help.
Attached patch Patch part 1 v2 (obsolete) — Splinter Review
I'm not completely sure about the nsDisplayList.cpp change, although I think it's correct.  (It's needed to keep fixed backgrounds on the canvas from breaking.)
Attachment #255319 - Attachment is obsolete: true
Attachment #256193 - Flags: review?(roc)
-  if ((display->IsBlockLevel() || display->IsFloating()) &&
-      (display->mOverflowX == NS_STYLE_OVERFLOW_CLIP)) {

Why is it safe to remove this? Child views still need to be clipped.

-        else if (NS_STYLE_VISIBILITY_HIDDEN == vis->mVisible) {
-          // If it has a widget, hide the view because the widget can't deal with it
-          if (view->HasWidget()) {
-            viewIsVisible = PR_FALSE;
-          }

Why is it safe to remove this view visibility code?

> I'm not completely sure about the nsDisplayList.cpp change, although I think
> it's correct.

I don't see any nsDisplayList.cpp change in this patch...
(In reply to comment #5)
> -  if ((display->IsBlockLevel() || display->IsFloating()) &&
> -      (display->mOverflowX == NS_STYLE_OVERFLOW_CLIP)) {
> 
> Why is it safe to remove this? Child views still need to be clipped.

Actually, view clipping doesn't really clip anything; the only uses as far as I can tell are to restrict invalidate regions (for the scrolling functions and for UpdateView).  And invalidating more than you should obviously can't break.  I've also done some testing, and there don't appear to be any regressions with -moz-hidden-unscrollable.  (There are some issues with -moz-hidden-unscrollable clipping and scroll views, but this patch doesn't affect that.)

> 
> -        else if (NS_STYLE_VISIBILITY_HIDDEN == vis->mVisible) {
> -          // If it has a widget, hide the view because the widget can't deal
> with it
> -          if (view->HasWidget()) {
> -            viewIsVisible = PR_FALSE;
> -          }
> 
> Why is it safe to remove this view visibility code?

It's impossible for viewIsVisible to be false.  We just created the view, so it can't have a widget yet, and the only callers of this function call it for elements.

> 
> > I'm not completely sure about the nsDisplayList.cpp change, although I think
> > it's correct.
> 
> I don't see any nsDisplayList.cpp change in this patch...
> 

Oops, I guess I posted the wrong version. I'll post the right one in a moment.
Attached patch Patch Part 1 v3Splinter Review
Attachment #256193 - Attachment is obsolete: true
Attachment #256212 - Flags: review?(roc)
Attachment #256193 - Flags: review?(roc)
I don't want to start invalidating more than we were, because it could affect performance negatively, but actually I think InvaliateInternal implementations in frames already do all the required clipping except in weird situations where frames are clipped by other frames that aren't in their containing block chain (i.e., frames are clipped by non-ancestor frames) and that's rare. So this should be OK. (And of course we should be able to remove nsView::GetClippedRect, but that can be done separately).
Comment on attachment 256212 [details] [diff] [review]
Patch Part 1 v3

Your change to nsDisplayBackground is correct because IsVaryingRelativeToFrame should return true whenever the display item's rendering changes when aFrame moves relative to its parent (as indicated by the comment in nsDisplayList.h for nsDisplayItem::IsVaryingRelativeToFrame).  The name of the method is a bit misleading though. In this case, if mFrame == aAncestorFrame then mFrame is moving relative to its parent and therefore relative to the viewport so we must return true. If that makes sense, you might want to mention that in a comment.

You need to change the IID on nsIViewManager.h and nsIView.h.
Attachment #256212 - Flags: superreview+
Attachment #256212 - Flags: review?(roc)
Attachment #256212 - Flags: review+
Resolving fixed; I'll file separate bugs for the other changes.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
some pull-down menu, <select><option>, does not work after this checkin.
(In reply to comment #12)
> some pull-down menu, <select><option>, does not work after this checkin.
> 

Could you be a little bit more spefidic?  I'm not having any issues.
I cannot specify it, but one is in edit section of movable type (free Blog tool).
one more sample.

http://roter-baum.de/spenden
Depends on: 371620
Depends on: 372039
Depends on: 372057
Depends on: 373474
No longer depends on: 375400
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.