Merge lp:~smspillaz/compiz-core/compiz-core.fix_969101 into lp:compiz-core

[Sam Spilsbury]

== Problem ==

See LP#969101 - DecorWindow::computeShadowRegion is called like 20000 times per 800 moves, which is bad and not necessary considering what it is used for. Also the implementation kinda sucked

== Solution ==

Refactor shadow clipping into a clip group and clippable interfaces. Clip groups represent a single "layer" for shadowing purposes where all the shadows are clipped against the union of the input rects of the windows belonging to that layer. Only update items in the clip group when a relevant one has changed.

MatchedDecorClipGroup etc provide a "guarded" entrance so that only relevant windows are let in.

== Tests ==

Unit tests included.

[Alan Griffiths (alan-griffiths)]

The killer: tests are not added to "make test"

Also, pure virtual functions should be private (not protected).

And there's no point to making destructors pure => "virtual ~DecorClippableInterface () {}"

Comment only: I (and almost everyone except Herb) don't see the point of NVPI.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Tests OK. No obvious regressions.

However it does terrify me to be changing such a vast (and important) region of code so close to release. I can't retain confidence in the release if we merge this before precise, so I'll leave the decision to someone else.

review: Abstain

[Daniel van Vugt (vanvugt)]

Approved, on the condition that it doesn't get merged till 0.9.8.

[Sam Spilsbury (smspillaz)]

1) Its fully tested
2) It vastly improves performance
3) The vast majority of the diff size is in the tests
4) It improves code design
5) Its blocking another very important piece of code from landing

I think those are plenty good reasons to merge this /before/ precise

[Mikkel Kamstrup Erlandsen (kamstrup)]

Took it for a test spin here and it works very well. Tried all sorts of weird window combinations with maximaized, semi-maximized, etc. All worked.

This branch seems to have as a side effect also fixing the bug where lingering shadow outlines would be briefly left behind when scrubbing panel app/indicator menus very fast. Only tested this on fast (sandybridge) hardware though, and not on my slower netbook. Is this correct or am I dreaming things up?

[Alan Griffiths (alan-griffiths)]

I agree with Sam that the "after" code is cleaner and that the majority of the diff is low risk (adding tests & build script).

I'd quibble with making destructors pure, and NVPI but it seems to run fine.

Cleaner, unit tested code... what's not to like?

review: Approve

[Sam Spilsbury (smspillaz)]

> Took it for a test spin here and it works very well. Tried all sorts of weird
> window combinations with maximaized, semi-maximized, etc. All worked.
>
> This branch seems to have as a side effect also fixing the bug where lingering
> shadow outlines would be briefly left behind when scrubbing panel
> app/indicator menus very fast. Only tested this on fast (sandybridge) hardware
> though, and not on my slower netbook. Is this correct or am I dreaming things
> up?

That is a bug in the animation plugin. Not sure what to do about it really.

[Daniel van Vugt (vanvugt)]

Confirmed this performance improvement seems to unblock the issue with:
lp:~smspillaz/compiz-core/compiz-core.work_923683

[Daniel van Vugt (vanvugt)]

review: Approve

=== modified file 'plugins/decor/CMakeLists.txt'
--- plugins/decor/CMakeLists.txt	2011-01-25 00:31:51 +0000
+++ plugins/decor/CMakeLists.txt	2012-03-31 03:35:24 +0000
@@ -3,7 +3,11 @@
 include (CompizPlugin)
 include (CompizCommon)
 
-compiz_plugin(decor PLUGINDEPS composite opengl LIBRARIES decoration)
+include_directories (${CMAKE_CURRENT_SOURCE_DIR}/src/clip-groups/include/)
+
+compiz_plugin(decor PLUGINDEPS composite opengl LIBRARIES decoration compiz_decor_clip_groups)
+
+add_subdirectory (src/clip-groups)
 
 if (COMPIZ_BUILD_WITH_RPATH AND NOT COMPIZ_DISABLE_PLUGIN_DECOR)
 

=== added directory 'plugins/decor/src/clip-groups'
=== added file 'plugins/decor/src/clip-groups/CMakeLists.txt'
--- plugins/decor/src/clip-groups/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ plugins/decor/src/clip-groups/CMakeLists.txt	2012-03-31 03:35:24 +0000
@@ -0,0 +1,62 @@
+pkg_check_modules (
+  GLIBMM
+  REQUIRED
+  glibmm-2.4 glib-2.0
+)
+
+INCLUDE_DIRECTORIES (  
+  ${CMAKE_CURRENT_SOURCE_DIR}/include
+  ${CMAKE_CURRENT_SOURCE_DIR}/src
+
+  ${compiz_SOURCE_DIR}/src/point/include
+  ${compiz_SOURCE_DIR}/src/rect/include
+  ${compiz_SOURCE_DIR}/src/window/geometry/include
+  ${compiz_SOURCE_DIR}/src/window/geometry-saver/include
+  ${compiz_SOURCE_DIR}/src/window/extents/include
+  ${compiz_SOURCE_DIR}/include
+    
+  ${Boost_INCLUDE_DIRS}
+  
+  ${GLIBMM_INCLUDE_DIRS}
+)
+
+LINK_DIRECTORIES (${GLIBMM_LIBRARY_DIRS}) 
+
+SET ( 
+  PUBLIC_HEADERS 
+)
+
+SET ( 
+  PRIVATE_HEADERS
+  ${CMAKE_CURRENT_SOURCE_DIR}/include/clip-groups.h
+)
+
+SET( 
+  SRCS 
+  ${CMAKE_CURRENT_SOURCE_DIR}/src/clip-groups.cpp
+)
+
+ADD_LIBRARY( 
+  compiz_decor_clip_groups STATIC
+  
+  ${SRCS}
+  
+  ${PUBLIC_HEADERS}
+  ${PRIVATE_HEADERS}
+)
+
+if (COMPIZ_BUILD_TESTING)
+ADD_SUBDIRECTORY( ${CMAKE_CURRENT_SOURCE_DIR}/tests )
+endif (COMPIZ_BUILD_TESTING)
+
+SET_TARGET_PROPERTIES(
+  compiz_decor_clip_groups PROPERTIES
+  PUBLIC_HEADER "${PUBLIC_HEADERS}"
+)
+
+TARGET_LINK_LIBRARIES(
+  compiz_decor_clip_groups
+
+  compiz_core
+  ${GLIBMM_LIBRARIES}
+)

=== added directory 'plugins/decor/src/clip-groups/include'
=== added file 'plugins/decor/src/clip-groups/include/clip-groups.h'
--- plugins/decor/src/clip-groups/include/clip-groups.h	1970-01-01 00:00:00 +0000
+++ plugins/decor/src/clip-groups/include/clip-groups.h	2012-03-31 03:35:24 +0000
@@ -0,0 +1,100 @@
+/*
+ * Copyright (C) 2001 Havoc Pennington
+ * Copyright (C) 2002, 2003 Red Hat, Inc.
+ * Copyright (C) 2003 Rob Adams
+ * Copyright (C) 2005 Novell, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
+ * 02111-1307, USA.
+ */
+
+#ifndef _COMPIZ_DECOR_CLIP_GROUPS_H
+#define _COMPIZ_DECOR_CLIP_GROUPS_H
+
+#include <vector>
+#include <core/rect.h>
+#include <core/region.h>
+#include <core/match.h>
+
+namespace compiz
+{
+    namespace decor
+    {
+	class DecorClipGroupInterface;
+
+	class DecorClippableInterface
+	{
+	    public:
+
+		virtual ~DecorClippableInterface () = 0;
+		void updateShadow (const CompRegion &r) { doUpdateShadow (r); }
+		void setOwner (DecorClipGroupInterface *i) { doSetOwner (i); }
+		bool matches (const CompMatch &m) { return doMatches (m); }
+		const CompRegion & outputRegion () { return getOutputRegion (); }
+		const CompRegion & inputRegion () { return getInputRegion (); }
+		void updateGroupShadows () { doUpdateGroupShadows (); }
+
+	    private:
+
+		virtual void doUpdateShadow (const CompRegion &) = 0;
+		virtual void doSetOwner (DecorClipGroupInterface *i) = 0;
+		virtual bool doMatches (const CompMatch &m) = 0;
+		virtual const CompRegion & getOutputRegion () = 0;
+		virtual const CompRegion & getInputRegion () = 0;
+		virtual void doUpdateGroupShadows () = 0;
+	};
+
+	class DecorClipGroupInterface
+	{
+	    public:
+
+		virtual ~DecorClipGroupInterface () = 0;
+
+		bool pushClippable (DecorClippableInterface *dc) { return doPushClippable (dc); }
+		bool popClippable (DecorClippableInterface *dc) { return doPopClippable (dc); }
+		void regenerateClipRegion () { doRegenerateClipRegion (); }
+		const CompRegion & clipRegion () { return getClipRegion (); }
+		void updateAllShadows () { return doUpdateAllShadows (); }
+
+	    private:
+
+		virtual bool doPushClippable (DecorClippableInterface *dc) = 0;
+		virtual bool doPopClippable (DecorClippableInterface *dc) = 0;
+		virtual void doRegenerateClipRegion () = 0;
+		virtual const CompRegion & getClipRegion () = 0;
+		virtual void doUpdateAllShadows () = 0;
+	};
+
+	namespace impl
+	{
+	    class GenericDecorClipGroup :
+		public DecorClipGroupInterface
+	    {
+		private:
+
+		    bool doPushClippable (DecorClippableInterface *dc);
+		    bool doPopClippable (DecorClippableInterface *dc);
+		    void doRegenerateClipRegion ();
+		    const CompRegion & getClipRegion ();
+		    void doUpdateAllShadows ();
+
+		    std::vector <DecorClippableInterface *> mClippables;
+		    CompRegion                              mRegion;
+	    };
+	}
+    }
+}
+
+#endif

=== added directory 'plugins/decor/src/clip-groups/src'
=== added file 'plugins/decor/src/clip-groups/src/clip-groups.cpp'
--- plugins/decor/src/clip-groups/src/clip-groups.cpp	1970-01-01 00:00:00 +0000
+++ plugins/decor/src/clip-groups/src/clip-groups.cpp	2012-03-31 03:35:24 +0000
@@ -0,0 +1,76 @@
+#include "clip-groups.h"
+#include <boost/foreach.hpp>
+#include <algorithm>
+
+#ifndef foreach
+#define foreach BOOST_FOREACH
+#endif
+
+using namespace compiz::decor;
+using namespace compiz::decor::impl;
+
+DecorClippableInterface::~DecorClippableInterface () {}
+DecorClipGroupInterface::~DecorClipGroupInterface () {}
+
+bool
+GenericDecorClipGroup::doPushClippable (DecorClippableInterface *dc)
+{
+    std::vector <DecorClippableInterface *>::iterator it = std::find (mClippables.begin (),
+								      mClippables.end (),
+								      dc);
+
+    if (it == mClippables.end ())
+    {
+	mClippables.push_back (dc);
+	regenerateClipRegion ();
+	dc->setOwner (this);
+
+	return true;
+    }
+
+    return false;
+}
+
+bool
+GenericDecorClipGroup::doPopClippable (DecorClippableInterface *dc)
+{
+    std::vector <DecorClippableInterface *>::iterator it = std::find (mClippables.begin (),
+								      mClippables.end (),
+								      dc);
+
+    if (it != mClippables.end ())
+    {
+	dc->setOwner (NULL);
+	dc->updateShadow (emptyRegion);
+	mClippables.erase (it);
+	regenerateClipRegion ();
+
+	return true;
+    }
+
+    return false;
+}
+
+void
+GenericDecorClipGroup::doRegenerateClipRegion ()
+{
+    mRegion -= infiniteRegion;
+
+    foreach (DecorClippableInterface *clippable, mClippables)
+    {
+	mRegion += clippable->inputRegion ();
+    }
+}
+
+const CompRegion &
+GenericDecorClipGroup::getClipRegion ()
+{
+    return mRegion;
+}
+
+void
+GenericDecorClipGroup::doUpdateAllShadows ()
+{
+    foreach (DecorClippableInterface *clippable, mClippables)
+	clippable->updateShadow (mRegion);
+}

=== added directory 'plugins/decor/src/clip-groups/tests'
=== added file 'plugins/decor/src/clip-groups/tests/CMakeLists.txt'
--- plugins/decor/src/clip-groups/tests/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ plugins/decor/src/clip-groups/tests/CMakeLists.txt	2012-03-31 03:35:24 +0000
@@ -0,0 +1,14 @@
+include_directories(${CMAKE_CURRENT_SOURCE_DIR})
+
+add_executable (compiz_test_decor_clip_groups
+                ${CMAKE_CURRENT_SOURCE_DIR}/clip-groups/src/test-decor-clip-groups.cpp)
+
+target_link_libraries (compiz_test_decor_clip_groups
+		       compiz_decor_clip_groups
+                       ${GTEST_BOTH_LIBRARIES}
+		       ${GMOCK_LIBRARY}
+		       ${GMOCK_MAIN_LIBRARY}
+		       ${CMAKE_THREAD_LIBS_INIT} # Link in pthread. 
+                       )
+
+gtest_add_tests (compiz_test_decor_clip_groups "" ${CMAKE_CURRENT_SOURCE_DIR}/clip-groups/src/test-decor-clip-groups.cpp)

=== added directory 'plugins/decor/src/clip-groups/tests/clip-groups'
=== added directory 'plugins/decor/src/clip-groups/tests/clip-groups/src'
=== added file 'plugins/decor/src/clip-groups/tests/clip-groups/src/test-decor-clip-groups.cpp'
--- plugins/decor/src/clip-groups/tests/clip-groups/src/test-decor-clip-groups.cpp	1970-01-01 00:00:00 +0000
+++ plugins/decor/src/clip-groups/tests/clip-groups/src/test-decor-clip-groups.cpp	2012-03-31 03:35:24 +0000
@@ -0,0 +1,395 @@
+/*
+ * Copyright © 2012 Canonical Ltd.
+ *
+ * Permission to use, copy, modify, distribute, and sell this software
+ * and its documentation for any purpose is hereby granted without
+ * fee, provided that the above copyright notice appear in all copies
+ * and that both that copyright notice and this permission notice
+ * appear in supporting documentation, and that the name of
+ * Canonical Ltd. not be used in advertising or publicity pertaining to
+ * distribution of the software without specific, written prior permission.
+ * Canonical Ltd. makes no representations about the suitability of this
+ * software for any purpose. It is provided "as is" without express or
+ * implied warranty.
+ *
+ * CANONICAL, LTD. DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
+ * NO EVENT SHALL CANONICAL, LTD. BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+ * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ *
+ * Authored by: Sam Spilsbury <[e-mail]>
+ */
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+#include <iostream>
+#include "clip-groups.h"
+
+using ::testing::Invoke;
+using ::testing::_;
+using ::testing::StrictMock;
+
+using namespace compiz::decor;
+
+class CompDecorClipGroupsTest :
+    public ::testing::Test
+{
+};
+
+class FakeDecorClippable :
+    public DecorClippableInterface
+{
+    public:
+	FakeDecorClippable (const CompRegion &inputRegion,
+			    const CompRegion &outputRegion,
+			    DecorClippableInterface *parent,
+			    bool	     matches) :
+	    mInputRegion (inputRegion),
+	    mOutputRegion (outputRegion),
+	    mParent (parent),
+	    mOwner (NULL),
+	    mMatches (matches)
+	{
+	}
+
+	~FakeDecorClippable ()
+	{
+	    if (mOwner)
+		mOwner->popClippable (mParent);
+	}
+
+	const CompRegion &
+	getShadowRegion ()
+	{
+	    return mShadowRegion;
+	}
+
+    private:
+
+	void doUpdateShadow (const CompRegion &r)
+	{
+	    mShadowRegion = mOutputRegion.intersected (r - mInputRegion);
+	}
+
+	void doSetOwner (DecorClipGroupInterface *i)
+	{
+	    mOwner = i;
+	}
+
+	bool doMatches (const CompMatch &m)
+	{
+	    return mMatches;
+	}
+
+	const CompRegion & getOutputRegion () { return mOutputRegion; }
+	const CompRegion & getInputRegion () { return mInputRegion; }
+
+	void doUpdateGroupShadows ()
+	{
+	    if (mOwner)
+		mOwner->updateAllShadows ();
+	}
+
+	CompRegion mInputRegion;
+	CompRegion mOutputRegion;
+
+	CompRegion mShadowRegion;
+
+	DecorClippableInterface *mParent;
+	DecorClipGroupInterface *mOwner;
+
+	bool				       mMatches;
+};
+
+class MockDecorClippable :
+    public DecorClippableInterface
+{
+    public:
+	
+	MOCK_METHOD1 (doUpdateShadow, void (const CompRegion &));
+	MOCK_METHOD1 (doSetOwner, void (DecorClipGroupInterface *));
+	MOCK_METHOD1 (doMatches, bool (const CompMatch &));
+	MOCK_METHOD0 (getOutputRegion, const CompRegion & ());
+	MOCK_METHOD0 (getInputRegion, const CompRegion & ());
+	MOCK_METHOD0 (doUpdateGroupShadows, void ());
+
+	void Delegate (DecorClippableInterface &other)
+	{
+	    ON_CALL (*this, doUpdateShadow (_)).WillByDefault (Invoke (&other, &DecorClippableInterface::updateShadow));
+	    ON_CALL (*this, doSetOwner (_)).WillByDefault (Invoke (&other, &DecorClippableInterface::setOwner));
+	    ON_CALL (*this, doMatches (_)).WillByDefault (Invoke (&other, &DecorClippableInterface::matches));
+	    ON_CALL (*this, getOutputRegion ()).WillByDefault (Invoke (&other, &DecorClippableInterface::outputRegion));
+	    ON_CALL (*this, getInputRegion ()).WillByDefault (Invoke (&other, &DecorClippableInterface::inputRegion));
+	    ON_CALL (*this, doUpdateGroupShadows ()).WillByDefault (Invoke (&other, &DecorClippableInterface::updateGroupShadows));
+	}
+};
+
+void PrintTo(const CompRegion &reg, ::std::ostream *os)
+{
+    const CompRect &br = reg.boundingRect ();
+    *os << "Bounding Rect " << br.x () << " " << br.y () << " " << br.width () << " " << br.height () << std::endl;
+    const CompRect::vector &rv = reg.rects ();
+    for (CompRect::vector::const_iterator it = rv.begin ();
+	 it != rv.end ();
+	 it++)
+    {
+	const CompRect &r = *it;
+	*os << " - Rect : " << r.x () << " " << r.y () << " " << r.width () << " " << r.height () << std::endl;
+    }
+}
+
+TEST_F(CompDecorClipGroupsTest, TestPushClippable)
+{
+    impl::GenericDecorClipGroup cg;
+    StrictMock <MockDecorClippable> mdc;
+    FakeDecorClippable fdc (CompRegion (50, 50, 100, 100),
+			    CompRegion (0, 0, 100, 100),
+			    &mdc,
+			    true);
+
+    mdc.Delegate (fdc);
+
+    EXPECT_CALL (mdc, getInputRegion ());
+    EXPECT_CALL (mdc, doSetOwner (_)).Times (2);
+    EXPECT_CALL (mdc, doUpdateShadow (_));
+
+    cg.pushClippable (&mdc);
+}
+
+TEST_F(CompDecorClipGroupsTest, TestPushClippableUpdatesRegion)
+{
+    impl::GenericDecorClipGroup cg;
+    StrictMock <MockDecorClippable> mdc;
+    FakeDecorClippable fdc (CompRegion (50, 50, 100, 100),
+			    CompRegion (0, 0, 100, 100),
+			    &mdc,
+			    true);
+
+    mdc.Delegate (fdc);
+
+    EXPECT_CALL (mdc, getInputRegion ()).Times (2);
+    EXPECT_CALL (mdc, doSetOwner (_)).Times (2);
+    EXPECT_CALL (mdc, doUpdateShadow (_));
+
+    cg.pushClippable (&mdc);
+
+    EXPECT_EQ (cg.clipRegion (), mdc.inputRegion ());
+}
+
+MATCHER_P (CompRegionEq, other, "")
+{
+    return (arg ^ other).isEmpty ();
+}
+
+TEST_F(CompDecorClipGroupsTest, TestPush2ClippableUpdatesRegion)
+{
+    impl::GenericDecorClipGroup cg;
+    StrictMock <MockDecorClippable> mdc;
+    FakeDecorClippable fdc (CompRegion (50, 50, 100, 100),
+			    CompRegion (0, 0, 100, 100),
+			    &mdc,
+			    true);
+
+    mdc.Delegate (fdc);
+
+    StrictMock <MockDecorClippable> mdc2;
+    FakeDecorClippable fdc2 (CompRegion (250, 250, 100, 100),
+			     CompRegion (200, 200, 100, 100),
+			     &mdc2,
+			     true);
+
+    mdc2.Delegate (fdc2);
+
+    EXPECT_CALL (mdc, getInputRegion ()).Times (4);
+    EXPECT_CALL (mdc, doSetOwner (&cg));
+    EXPECT_CALL (mdc, doSetOwner (NULL));
+    EXPECT_CALL (mdc, doUpdateShadow (_)).Times (1);
+
+    EXPECT_CALL (mdc2, getInputRegion ()).Times (2);
+    EXPECT_CALL (mdc2, doSetOwner (&cg));
+    EXPECT_CALL (mdc2, doSetOwner (NULL));
+    EXPECT_CALL (mdc2, doUpdateShadow (_)).Times (1);
+
+    cg.pushClippable (&mdc);
+    cg.pushClippable (&mdc2);
+
+    CompRegion accumulated;
+
+    accumulated += mdc.inputRegion ();
+    accumulated += mdc2.inputRegion ();
+
+    EXPECT_THAT (cg.clipRegion (), CompRegionEq (accumulated));
+}
+
+TEST_F(CompDecorClipGroupsTest, TestPush3ClippableUpdatesRegion)
+{
+    impl::GenericDecorClipGroup cg;
+    StrictMock <MockDecorClippable> mdc;
+    FakeDecorClippable fdc (CompRegion (50, 50, 100, 100),
+			    CompRegion (0, 0, 100, 100),
+			    &mdc,
+			    true);
+
+    mdc.Delegate (fdc);
+
+    StrictMock <MockDecorClippable> mdc2;
+    FakeDecorClippable fdc2 (CompRegion (150, 150, 100, 100),
+			     CompRegion (100, 100, 100, 100),
+			     &mdc2,
+			     true);
+
+    mdc2.Delegate (fdc2);
+
+    StrictMock <MockDecorClippable> mdc3;
+    FakeDecorClippable fdc3 (CompRegion (250, 250, 100, 100),
+			     CompRegion (200, 200, 100, 100),
+			     &mdc3,
+			     true);
+
+    mdc3.Delegate (fdc3);
+
+    EXPECT_CALL (mdc, getInputRegion ()).Times (6);
+    EXPECT_CALL (mdc, doSetOwner (&cg));
+    EXPECT_CALL (mdc, doSetOwner (NULL));
+    EXPECT_CALL (mdc, doUpdateShadow (_)).Times (1);
+
+    EXPECT_CALL (mdc2, getInputRegion ()).Times (4);
+    EXPECT_CALL (mdc2, doSetOwner (&cg));
+    EXPECT_CALL (mdc2, doSetOwner (NULL));
+    EXPECT_CALL (mdc2, doUpdateShadow (_)).Times (1);
+
+    EXPECT_CALL (mdc3, getInputRegion ()).Times (2);
+    EXPECT_CALL (mdc3, doSetOwner (&cg));
+    EXPECT_CALL (mdc3, doSetOwner (NULL));
+    EXPECT_CALL (mdc3, doUpdateShadow (_)).Times (1);
+
+    cg.pushClippable (&mdc);
+    cg.pushClippable (&mdc2);
+    cg.pushClippable (&mdc3);
+
+    CompRegion accumulated;
+
+    accumulated += mdc.inputRegion ();
+    accumulated += mdc2.inputRegion ();
+    accumulated += mdc3.inputRegion ();
+
+    EXPECT_THAT (cg.clipRegion (), CompRegionEq (accumulated));
+}
+
+TEST_F(CompDecorClipGroupsTest, TestPush2ClippableUpdatesShadow)
+{
+    impl::GenericDecorClipGroup cg;
+    StrictMock <MockDecorClippable> mdc;
+    FakeDecorClippable fdc (CompRegion (50, 50, 25, 25),
+			    CompRegion (25, 25, 100, 100),
+			    &mdc,
+			    true);
+
+    mdc.Delegate (fdc);
+
+    StrictMock <MockDecorClippable> mdc2;
+    FakeDecorClippable fdc2 (CompRegion (75, 75, 50, 50),
+			     CompRegion (25, 25, 100, 100),
+			     &mdc2,
+			     true);
+
+    mdc2.Delegate (fdc2);
+
+    EXPECT_CALL (mdc, getInputRegion ()).Times (4);
+    EXPECT_CALL (mdc, doSetOwner (&cg));
+    EXPECT_CALL (mdc, doSetOwner (NULL));
+    EXPECT_CALL (mdc, doUpdateShadow (_)).Times (2);
+
+    EXPECT_CALL (mdc2, getInputRegion ()).Times (2);
+    EXPECT_CALL (mdc2, doSetOwner (&cg));
+    EXPECT_CALL (mdc2, doSetOwner (NULL));
+    EXPECT_CALL (mdc2, doUpdateShadow (_)).Times (2);
+
+    cg.pushClippable (&mdc);
+    cg.pushClippable (&mdc2);
+
+    cg.updateAllShadows ();
+
+    CompRegion accumulated;
+
+    accumulated += mdc.inputRegion ();
+    accumulated += mdc2.inputRegion ();
+
+    EXPECT_THAT (cg.clipRegion (), CompRegionEq (accumulated));
+
+    CompRegion fdcRegion (75, 75, 50, 50);
+
+    EXPECT_THAT (fdc.getShadowRegion (), CompRegionEq (fdcRegion));
+
+    CompRegion fdc2Region (50, 50, 25, 25);
+
+    EXPECT_THAT (fdc2.getShadowRegion (), CompRegionEq (fdc2Region));
+}
+
+TEST_F(CompDecorClipGroupsTest, TestPush3ClippableUpdatesRegionPop1)
+{
+    impl::GenericDecorClipGroup cg;
+    StrictMock <MockDecorClippable> mdc;
+    FakeDecorClippable fdc (CompRegion (50, 50, 100, 100),
+			    CompRegion (0, 0, 100, 100),
+			    &mdc,
+			    true);
+
+    mdc.Delegate (fdc);
+
+    StrictMock <MockDecorClippable> mdc2;
+    FakeDecorClippable fdc2 (CompRegion (150, 150, 100, 100),
+			     CompRegion (100, 100, 100, 100),
+			     &mdc2,
+			     true);
+
+    mdc2.Delegate (fdc2);
+
+    StrictMock <MockDecorClippable> mdc3;
+    FakeDecorClippable fdc3 (CompRegion (250, 250, 100, 100),
+			     CompRegion (200, 200, 100, 100),
+			     &mdc3,
+			     true);
+
+    mdc3.Delegate (fdc3);
+
+    EXPECT_CALL (mdc, getInputRegion ()).Times (7);
+    EXPECT_CALL (mdc, doSetOwner (&cg));
+    EXPECT_CALL (mdc, doSetOwner (NULL));
+    EXPECT_CALL (mdc, doUpdateShadow (_)).Times (1);
+
+    EXPECT_CALL (mdc2, getInputRegion ()).Times (5);
+    EXPECT_CALL (mdc2, doSetOwner (&cg));
+    EXPECT_CALL (mdc2, doSetOwner (NULL));
+    EXPECT_CALL (mdc2, doUpdateShadow (_)).Times (1);
+
+    EXPECT_CALL (mdc3, getInputRegion ()).Times (2);
+    EXPECT_CALL (mdc3, doSetOwner (&cg));
+    EXPECT_CALL (mdc3, doSetOwner (NULL));
+    EXPECT_CALL (mdc3, doUpdateShadow (_)).Times (1);
+
+    cg.pushClippable (&mdc);
+    cg.pushClippable (&mdc2);
+    cg.pushClippable (&mdc3);
+
+    CompRegion accumulated;
+
+    accumulated += mdc.inputRegion ();
+    accumulated += mdc2.inputRegion ();
+    accumulated += mdc3.inputRegion ();
+
+    EXPECT_THAT (cg.clipRegion (), CompRegionEq (accumulated));
+
+    cg.popClippable (&mdc3);
+
+    accumulated = CompRegion ();
+
+    accumulated += mdc.inputRegion ();
+    accumulated += mdc2.inputRegion ();
+
+    EXPECT_THAT (cg.clipRegion (), CompRegionEq (accumulated));
+}
+

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2012-03-22 17:00:51 +0000
+++ plugins/decor/src/decor.cpp	2012-03-31 03:35:24 +0000
@@ -42,6 +42,57 @@
 
 COMPIZ_PLUGIN_20090315 (decor, DecorPluginVTable)
 
+MatchedDecorClipGroup::MatchedDecorClipGroup (const CompMatch &match) :
+    mMatch (match)
+{
+}
+
+bool
+MatchedDecorClipGroup::doPushClippable (DecorClippableInterface *dc)
+{
+    if (dc->matches (mMatch))
+	return mClipGroupImpl.pushClippable (dc);
+
+    return false;
+}
+
+void
+DecorWindow::doUpdateShadow (const CompRegion &reg)
+{
+    shadowRegion = outputRegion () - (reg - inputRegion ());
+}
+
+void
+DecorWindow::doSetOwner (DecorClipGroupInterface *i)
+{
+    mClipGroup = i;
+}
+
+bool
+DecorWindow::doMatches (const CompMatch &m)
+{
+    return const_cast <CompMatch &> (m).evaluate (window) && !window->invisible ();
+}
+
+const CompRegion &
+DecorWindow::getOutputRegion ()
+{
+    return mOutputRegion;
+}
+
+const CompRegion &
+DecorWindow::getInputRegion ()
+{
+    return mInputRegion;
+}
+
+void
+DecorWindow::doUpdateGroupShadows ()
+{
+    if (mClipGroup)
+	mClipGroup->updateAllShadows ();
+}
+
 /* From core */
 
 /*
@@ -69,89 +120,6 @@
     return false;
 }
 
-/* 
- * DecorWindow::computeShadowRegion
- *
- * This function computes the current clip region for the
- * shadow that should be draw on glDraw. 
- *
- * Make shadows look nice, don't paint shadows on top of
- * things they don't make sense on top of, eg, menus
- * need shadows but they don't need to be painted when
- * another menu is adjacent and covering the shadow
- * region. Also panel shadows are nice, but not
- * when they obscure client window shadows
- *
- * We need to use the current clip region here
- * and take an intersection of that to ensure
- * that we don't unintentionally expand the clip
- * region that core already reduced by doing
- * occlusion detection
- */
-void
-DecorWindow::computeShadowRegion ()
-{
-    shadowRegion = CompRegion (window->outputRect ());
-
-    if (window->type () == CompWindowTypeDropdownMenuMask ||
-        window->type () == CompWindowTypePopupMenuMask)
-    {
-        /* Other transient menus should clip
-         * this menu's shadows, also the panel
-         * which is a transient parent should
-         * too */
-
-        CompWindowList::iterator it = std::find (screen->windows ().begin (),
-                                                 screen->windows ().end (),
-                                                 window);
-
-        for (it--; it != screen->windows ().end (); it--)
-        {
-            if (!(*it)->isViewable ())
-                continue;
-
-            if (!((*it)->type () == CompWindowTypeDropdownMenuMask ||
-                  (*it)->type () == CompWindowTypePopupMenuMask ||
-                  (*it)->type () == CompWindowTypeDockMask))
-		continue;
-
-            /* window needs to be a transient parent */
-            if (!isAncestorTo (window, (*it)))
-                continue;
-
-            CompRegion inter (shadowRegion);
-            inter &= (*it)->borderRect ();
-
-            if (!inter.isEmpty ())
-                shadowRegion -= inter;
-        }
-
-        /* If the region didn't change, then it is safe to
-         * say that that this window was probably the first
-         * menu in the "chain" of dropdown menus that comes
-         * from a menu-bar - in that case there isn't any
-         * window that the shadow would necessarily occlude
-         * here so clip the shadow to the top of the input
-         * rect.
-         *
-         * FIXME: We need a better way to detect exactly
-         * where the menubar is for the dropdown menu,
-         * that will look a lot better.
-         */
-        if (window->type () == CompWindowTypeDropdownMenuMask &&
-	    shadowRegion == CompRegionRef (window->outputRect ().region ()))
-        {
-            CompRect area (window->outputRect ().x1 (),
-                           window->outputRect ().y1 (),
-                           window->outputRect ().width (),
-			   window->inputRect ().y1 () -
-                           window->outputRect ().y1 ());
-
-	    shadowRegion -= area;
-        }
-    }
-}
-
 /*
  * DecorWindow::glDraw
  *
@@ -1508,7 +1476,7 @@
     if (dScreen->cmActive)
     {
 	cWindow->damageOutputExtents ();
-	computeShadowRegion ();
+	updateGroupShadows ();
     }
 
     /* Determine how much we moved the window for the old
@@ -2183,7 +2151,17 @@
 		break;
 	    }
 
+	    /* For non-switcher windows we need to update the decoration
+	     * anyways, since the window is unmapped. Also need to
+	     * update the shadow clip regions for panels and other windows */
+	    update (true);
+	    if (dScreen->mMenusClipGroup.pushClippable (this))
+		updateGroupShadows ();
+
+	    break;
+
 	case CompWindowNotifyUnmap:
+	{
 
 	    /* When the switcher is unmapped, it has no frame window
 	     * so the frame window for it needs to unmapped manually */
@@ -2198,14 +2176,15 @@
 	     * anyways, since the window is unmapped. Also need to
 	     * update the shadow clip regions for panels and other windows */
 	    update (true);
-	    if (dScreen->cmActive)
-	    {
-		foreach (CompWindow *cw, DecorScreen::get (screen)->cScreen->getWindowPaintList ())
-		{
-		    DecorWindow::get (cw)->computeShadowRegion ();
-		}
-	    }
+
+	    /* Preserve the group shadow update ptr */
+	    DecorClipGroupInterface *clipGroup = mClipGroup;
+
+	    if (dScreen->mMenusClipGroup.popClippable (this))
+		if (clipGroup)
+		    clipGroup->updateAllShadows ();
 	    break;
+	}
 	case CompWindowNotifyUnreparent:
 	{
 	    /* Compiz detaches the frame window from
@@ -2397,18 +2376,6 @@
 		if (w)
 		    DecorWindow::get (w)->update (true);
 	    }
-	    /* On a transient change, we need to recompute shadow regions
-	     * for eg, menus */
-	    else if (event->xproperty.atom == XA_WM_TRANSIENT_FOR)
-	    {
-		if (cmActive)
-		{
-		    foreach (CompWindow *cw, cScreen->getWindowPaintList ())
-		    {
-			DecorWindow::get (cw)->computeShadowRegion ();
-		    }
-		}
-	    }
 	    else
 	    {
 		if (event->xproperty.window == screen->root ())
@@ -2736,14 +2703,11 @@
     }
     updateReg = true;
 
-    if (dScreen->cmActive)
-    {
-	foreach (CompWindow *cw,
-		 DecorScreen::get (screen)->cScreen->getWindowPaintList ())
-	{
-	    DecorWindow::get (cw)->computeShadowRegion ();
-	}
-    }
+    mInputRegion.translate (dx, dy);
+    mOutputRegion.translate (dx, dy);
+
+    if (dScreen->cmActive && mClipGroup)
+	updateGroupShadows ();
 
     window->moveNotify (dx, dy, immediate);
 }
@@ -2796,14 +2760,10 @@
     updateDecorationScale ();
     updateReg = true;
 
-    if (dScreen->cmActive)
-    {
-	foreach (CompWindow *cw,
-		 DecorScreen::get (screen)->cScreen->getWindowPaintList ())
-	{
-	    DecorWindow::get (cw)->computeShadowRegion ();
-	}
-    }
+    mInputRegion = CompRegion (window->inputRect ());
+    mOutputRegion = CompRegion (window->outputRect ());
+    if (dScreen->cmActive && mClipGroup)
+	updateGroupShadows ();
 
     window->resizeNotify (dx, dy, dwidth, dheight);
 }
@@ -2971,7 +2931,8 @@
 				   0,
 				   None,
 				   boost::shared_array <decor_quad_t> (NULL),
-				   0))
+				   0)),
+    mMenusClipGroup (CompMatch ("type=Dock | type=DropdownMenu | type=Menu | type=PopupMenu"))
 {
     supportingDmCheckAtom =
 	XInternAtom (s->dpy (), DECOR_SUPPORTING_DM_CHECK_ATOM_NAME, 0);
@@ -3045,7 +3006,10 @@
     unshading (false),
     shading (false),
     isSwitcher (false),
-    frameExtentsRequested (false)
+    frameExtentsRequested (false),
+    mClipGroup (NULL),
+    mOutputRegion (window->outputRect ()),
+    mInputRegion (window->inputRect ())
 {
     WindowInterface::setHandler (window);
 
@@ -3074,6 +3038,10 @@
     }
 
     window->resizeNotifySetEnabled (this, true);
+
+    if (!window->invisible ())
+	if (dScreen->mMenusClipGroup.pushClippable (this))
+	    updateGroupShadows ();
 }
 
 /* 
@@ -3090,6 +3058,9 @@
     if (wd)
 	WindowDecoration::destroy (wd);
 
+    if (mClipGroup)
+	mClipGroup->popClippable (this);
+
     decor.mList.clear ();
 }
 

=== modified file 'plugins/decor/src/decor.h'
--- plugins/decor/src/decor.h	2012-02-29 08:38:22 +0000
+++ plugins/decor/src/decor.h	2012-03-31 03:35:24 +0000
@@ -33,6 +33,8 @@
 #include <core/atoms.h>
 #include <core/windowextents.h>
 
+#include <clip-groups.h>
+
 #include "decor_options.h"
 
 #define DECOR_SCREEN(s) DecorScreen *ds = DecorScreen::get(s)
@@ -50,6 +52,27 @@
 #define DECOR_ACTIVE 1
 #define DECOR_NUM    2
 
+using namespace compiz::decor;
+
+class MatchedDecorClipGroup :
+    public DecorClipGroupInterface
+{
+    public:
+
+	MatchedDecorClipGroup (const CompMatch &match);
+
+    private:
+
+	bool doPushClippable (DecorClippableInterface *dc);
+	bool doPopClippable (DecorClippableInterface *dc) { return mClipGroupImpl.popClippable (dc); }
+	void doRegenerateClipRegion () { return mClipGroupImpl.regenerateClipRegion (); }
+	const CompRegion & getClipRegion () { return mClipGroupImpl.clipRegion (); }
+	void doUpdateAllShadows () { return mClipGroupImpl.updateAllShadows (); }
+
+	impl::GenericDecorClipGroup		mClipGroupImpl;
+	CompMatch                               mMatch;
+};
+
 class DecorTexture {
 
     public:
@@ -204,13 +227,16 @@
 	std::map<Window, DecorWindow *> frames;
 
 	CompTimer decoratorStart;
+
+	MatchedDecorClipGroup mMenusClipGroup;
 };
 
 class DecorWindow :
     public WindowInterface,
     public CompositeWindowInterface,
     public GLWindowInterface,
-    public PluginClassHandler<DecorWindow,CompWindow>
+    public PluginClassHandler<DecorWindow,CompWindow>,
+    public DecorClippableInterface
 {
     public:
 	DecorWindow (CompWindow *w);
@@ -260,6 +286,15 @@
 	static bool matchState (CompWindow *w, unsigned int decorState);
 	static bool matchActions (CompWindow *w, unsigned int decorActions);
 
+    private:
+
+	void doUpdateShadow (const CompRegion &);
+	void doSetOwner (DecorClipGroupInterface *i);
+	bool doMatches (const CompMatch &m);
+	const CompRegion & getOutputRegion ();
+	const CompRegion & getInputRegion ();
+	void doUpdateGroupShadows ();
+
     public:
 
 	CompWindow      *window;
@@ -296,6 +331,10 @@
 	bool	  isSwitcher;
 
 	bool      frameExtentsRequested;
+
+	DecorClipGroupInterface *mClipGroup;
+	CompRegion		mOutputRegion;
+	CompRegion              mInputRegion;
 };
 
 class DecorPluginVTable :