Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Description • 15 years ago

Mobile wants native widgets removed from decks and currently uses a #define to do that. Pre-compositor this used to work, but I'm not sure how. Now it doesn't. The problem is that the native widgets for the <browser>s in a deck aren't hidden when they're not in the deck's selected child. Rather than go back and try to figure out what changed and hack something in to fix the regression for mobile, I'd rather move forward and just remove widgets from deck children for everyone.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 1 • 15 years ago

View visibility is a mess unfortunately, but at least here I'm trying to tighten it up a bit --- see the comment in nsIView.h. Basically we assume that everything under a hidden view is going to be hidden, so we go ahead and hide every widget under a hidden view, and maintain that during dynamic changes.

	David Baron :dbaron:
	Comment 2 • 15 years ago

Comment on attachment 390786 [details] [diff] [review]
fix

>+NS_IMETHODIMP nsView::SetVisibility(nsViewVisibility aVisibility)
>+{
>+  mVis = aVisibility;
>+  NotifyEffectiveVisibilityChanged(IsEffectivelyVisible());
>   return NS_OK;
> }

Probably you should only call NotifyEffectiveVisibilityChanged if the result of IsEffectivelyVisible changes across the setting of mVis?

Other than that, this looks good, although it's made me realize that bug 465216 blocks one of the later phases of compositor (removing widgets at chrome/content boundaries).

r=dbaron

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 3 • 15 years ago

(In reply to comment #2)
> (From update of attachment 390786 [details] [diff] [review])
> >+NS_IMETHODIMP nsView::SetVisibility(nsViewVisibility aVisibility)
> >+{
> >+  mVis = aVisibility;
> >+  NotifyEffectiveVisibilityChanged(IsEffectivelyVisible());
> >   return NS_OK;
> > }
> 
> Probably you should only call NotifyEffectiveVisibilityChanged if the result of
> IsEffectivelyVisible changes across the setting of mVis?

I could, but nsViewManager::SetViewVisibility already bails early if mVis == aVisibility, and catching the extra case (where an ancestor is hidden, so this view remains effectively hidden) doesn't seem worth it.

Thanks.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 4 • 15 years ago

Checked in, but backed out because it caused a lot of test failures, e.g.:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1248929568.1248932030.27567.gz

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 5 • 15 years ago

This seems to have caused https://bugzilla.mozilla.org/show_bug.cgi?id=507380
as the backout of this bug fixed the scrollbars.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 15 years ago

But 507380 and a lot (if not all) of those test failures was due to a mistake in nsViewManager::GrabMouseEvents:

-  if (aView && static_cast<nsView*>(aView)->GetVisibility()
-               == nsViewVisibility_kHide) {
+  if (aView && static_cast<nsView*>(aView)->IsEffectivelyVisible()) {

That should of course be !IsEffectivelyVisible... Running try-server tests now.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 7 • 15 years ago

There was another problem where test_focus.xul was failing because we'd treat frames in hidden deck children as focusable. This turned out to be because with the previous version of the patch there is nothing to force deck children to have views, so there's no hidden view associated with hidden deck children, which is what nsFrame::IsFocusable uses to detect deck-hidden frames.

So in this version of the patch as well as the one-line fix in GrabMouseEvents, I'm overriding nsDeckFrame's SetInitialChildList/AppendFrames/InsertFrames to create views for the children. This seems cleaner than the trunk approach of calling a special method on the parent to detect when we need to create a view and widget.

	David Baron :dbaron:
	Comment 8 • 15 years ago

Comment on attachment 391856 [details] [diff] [review]
fix v2

>+CreateViewsForFrames(const nsFrameList& aFrames)

Maybe call this ForceCreateViewsForFrames or ForceViewsForFrames since it passes PR_TRUE to the aForce parameter of CreateViewForFrame?

>+  NS_IMETHOD  SetInitialChildList(nsIAtom*        aListName,
>+                                  nsFrameList&    aChildList);

Fix that weird double-space after NS_IMETHOD.

r=dbaron

	Dão Gottwald [:dao]
	Comment 10 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/79db77b024f7

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 11 • 15 years ago

Thanks Dao!

	Masatoshi Kimura [:emk]
	Comment 12 • 15 years ago

attachment 391961 [details] [diff] [review] looks to the same as attachment 391856 [details] [diff] [review].

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 13 • 15 years ago

Oops, somehow I attached the wrong patch. Oh well.

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 14 • 15 years ago

Sorry for tagging a 'fixed' bug, but don't have time to file a new one.
See discussion in this thread: http://forums.mozillazine.org/viewtopic.php?f=23&t=1397405

Seems this bug has broken the Options Panel...details in the thread.

	Rob Campbell [:rc] (:robcee)
	Comment 15 • 15 years ago

This bug has also broken Firebug (editing in HTML and DOM editors) as well as Twitterfox.

bug 509478

	John J. Barton
	Comment 16 • 15 years ago

Some thing is messed up here in bugzilla.

Bug 507380 claims that the fix here has been backed out. It looks like it was re-committed, but now comment 14, bug 509478, and bug 507380 seem to suggest that this fix is still causing problems.
	
Also the Depends-on list seems upside down. Several bugs on that list are really Blocks bugs: if this bug was correctly fixed they would be fixed.

I'm going to set this to reopened just to highlight it as needing attention; I guess we will need to wait for roc to return to sort it out.

	Dão Gottwald [:dao]
	Comment 17 • 15 years ago

(In reply to comment #16)
> Bug 507380 claims that the fix here has been backed out.

That's correct, see comment 4 and comment 5.

> It looks like it was re-committed,

Indeed, see comment 10.

> but now comment 14, bug 509478, and bug 507380 seem to suggest
> that this fix is still causing problems.

It is!

> Also the Depends-on list seems upside down.

No, it's not. Regressions caused by this bug block it.

	John J. Barton
	Comment 18 • 15 years ago

(In reply to comment #17)
...
> > Also the Depends-on list seems upside down.
> 
> No, it's not. Regressions caused by this bug block it.

Can you help me understand this? The bug is fixed, so blocking means what? It cannot make any more progress. By their definition the regressions say "that fix, it was not a good one, it should be redone" (as in "it was not really a fix after all).

If independent work is needed to solve the problem on the regression bugs, then they still don't block progress on this one, they are on their own. On the other hand if more work is needed on this patch, then it's not fixed and progress on it prevents progress on the regression reporting bug. 

(sorry for the meta comment).

	David Baron :dbaron:
	Comment 19 • 15 years ago

Do not use this bug as a forum for complaining about how we use dependencies.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 20 • 15 years ago

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

	Tracy Walker [:tracy]
	Comment 21 • 15 years ago

verified per re-checkin and assumed pass of try server testing.

	Timothy Nikkel (:tnikkel)
	Comment 22 • 14 years ago

There is code in nsFrameFrame.cpp that seems like it creates widgets for decks here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrameFrame.cpp#299

Is it still needed?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 23 • 14 years ago

No, that should be removed.