Last Comment Bug 635643 - remove nsIRegion
: remove nsIRegion
Status: RESOLVED FIXED
[mentor=tnikkel@gmail.com][inbound]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Marco Castelluccio [:marco]
:
Mentors:
Depends on:
Blocks: deCOM 41701
  Show dependency treegraph
 
Reported: 2011-02-20 16:42 PST by Timothy Nikkel (:tnikkel)
Modified: 2011-07-26 04:03 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removes the function windowsUtils20 and so nsIDOMWindowUtils_MOZILLA_2_0_BRANCH (1.13 KB, patch)
2011-07-19 15:27 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Removed nsIRegion and made nsIScriptableRegion a wrapper directly around nsIntRegion. (17.71 KB, patch)
2011-07-21 04:50 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch revised (37.19 KB, patch)
2011-07-21 18:09 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch revised v2 (36.71 KB, patch)
2011-07-22 03:45 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch revised v3 (37.36 KB, patch)
2011-07-22 10:03 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch revised v4 (36.68 KB, patch)
2011-07-23 05:26 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch revised v5 (41.02 KB, patch)
2011-07-24 17:54 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch revised v6 (41.04 KB, patch)
2011-07-25 02:34 PDT, Marco Castelluccio [:marco]
tnikkel: review+
Details | Diff | Splinter Review

Description Timothy Nikkel (:tnikkel) 2011-02-20 16:42:49 PST
nsIRegion is basically just a wrapper around nsRegion and is basically unused now. It is implemented by nsThebesRegion and the only user is nsIScriptableRegion which is just a wrapper around nsIRegion. We should remove the middle man and just make nsIScriptableRegion a wrapper around ns(Int)Region.
Comment 1 Marco Castelluccio [:marco] 2011-07-19 15:27:02 PDT
Created attachment 546929 [details] [diff] [review]
Removes the function windowsUtils20 and so nsIDOMWindowUtils_MOZILLA_2_0_BRANCH
Comment 2 Marco Castelluccio [:marco] 2011-07-19 15:28:06 PDT
Sorry, I'm stupid. I've uploaded the patch to the wrong bug!
Comment 3 Marco Castelluccio [:marco] 2011-07-21 04:50:28 PDT
Created attachment 547370 [details] [diff] [review]
Removed nsIRegion and made nsIScriptableRegion a wrapper directly around nsIntRegion.
Comment 4 :Ms2ger 2011-07-21 10:12:40 PDT
Comment on attachment 547370 [details] [diff] [review]
Removed nsIRegion and made nsIScriptableRegion a wrapper directly around nsIntRegion.

>--- a/gfx/src/nsScriptableRegion.cpp	Tue Jul 19 14:24:01 2011 -0400
>+++ b/gfx/src/nsScriptableRegion.cpp	Thu Jul 21 13:48:17 2011 +0200
>-nsScriptableRegion::nsScriptableRegion(nsIRegion* region) : mRegion(nsnull), mRectSet(nsnull)
>+nsScriptableRegion::nsScriptableRegion()
> {
>-	mRegion = region;
>-	NS_IF_ADDREF(mRegion);
>+	mRectSet = NULL;
>+	//mRegion = region;
>+	//NS_IF_ADDREF(mRegion);

Please don't keep commented out code around.

> NS_IMETHODIMP nsScriptableRegion::SetToRegion(nsIScriptableRegion *aRegion)
> {
>+  const nsScriptableRegion* pRegion = static_cast<const nsScriptableRegion*>(aRegion);
>+  mRegion = pRegion->mRegion;

mRegion = static_cast<nsScriptableRegion*>(aRegion)->mRegion;

will do, no? I think it would be better to add a [notxpcom] readonly attribute region to nsIScriptableRegion to avoid all these casts.

> NS_IMETHODIMP nsScriptableRegion::UnionRect(PRInt32 aX, PRInt32 aY, PRInt32 aWidth, PRInt32 aHeight)
> {
>+  mRegion.Or(mRegion, nsIntRect (aX, aY, aWidth, aHeight));

No space between nsIntRect and (.

> NS_IMETHODIMP nsScriptableRegion::GetBoundingBox(PRInt32 *aX, PRInt32 *aY, PRInt32 *aWidth, PRInt32 *aHeight)
> {
>+  nsIntRect BoundRect;
>+  BoundRect = mRegion.GetBounds();

Variables start with a lower-case letter, and merge these two lines.

>-NS_IMETHODIMP nsScriptableRegion::GetRegion(nsIRegion** outRgn)
>+NS_IMETHODIMP nsScriptableRegion::GetRegion(nsIntRegion* outRgn)
> {
>-  *outRgn = mRegion;
>-  NS_IF_ADDREF(*outRgn);
>+  outRgn = &mRegion;

Doesn't this need to copy?

>--- a/gfx/src/nsThebesGfxFactory.cpp	Tue Jul 19 14:24:01 2011 -0400
>+++ b/gfx/src/nsThebesGfxFactory.cpp	Thu Jul 21 13:48:17 2011 +0200
>@@ -78,23 +76,18 @@ nsScriptableRegionConstructor(nsISupport
>-  nsCOMPtr <nsIRegion> rgn = new nsThebesRegion();
>-  nsCOMPtr<nsIScriptableRegion> scriptableRgn;
>-  if (rgn != nsnull)
>-  {
>-    scriptableRgn = new nsScriptableRegion(rgn);
>-    inst = scriptableRgn;
>-  }
>+  nsCOMPtr<nsIScriptableRegion> scriptableRgn = new nsScriptableRegion();
>+  inst = scriptableRgn;
>   if (!inst)
>   {
>     rv = NS_ERROR_OUT_OF_MEMORY;
>     return rv;
>   }

This can go, new is infallible. However, this function can be further simplified to

if (!aResult) ...
if (aOuter) ...

nsCOMPtr<nsIScriptableRegion> scriptableRgn = new nsScriptableRegion();
return scriptableRgn->QueryInterface(aIID, aResult);

>--- a/widget/src/xpwidgets/nsBaseDragService.cpp	Tue Jul 19 14:24:01 2011 -0400
>+++ b/widget/src/xpwidgets/nsBaseDragService.cpp	Thu Jul 21 13:48:17 2011 +0200
>+  nsIntRegion *clipRegion = NULL;
>+  if (aRegion)
>+    aRegion->GetRegion(clipRegion);
> 
>   nsIntPoint pnt(aScreenDragRect->x, aScreenDragRect->y);
>   nsRefPtr<gfxASurface> surface =
>-    presShell->RenderNode(dragNode, aRegion ? &clipRegion : nsnull,
>+    presShell->RenderNode(dragNode, aRegion ? clipRegion : nsnull,

This can just pass clipRegion now, no?
Comment 5 Marco Castelluccio [:marco] 2011-07-21 10:35:37 PDT
Thanks for the review Ms2ger, I'll do these modifications tomorrow.
I've also deleted three files (nsThebesRegion.cpp, nsThebesRegion.h, nsIRegion.h), but this modification seems to lack (I don't know why) from the patch I've uploaded.
Comment 6 Timothy Nikkel (:tnikkel) 2011-07-21 10:39:07 PDT
Did you try doing hg rm on the files instead of just deleting them from the file system?
Comment 7 Marco Castelluccio [:marco] 2011-07-21 18:09:52 PDT
Created attachment 547582 [details] [diff] [review]
Patch revised

I've deleted the three files with hg rm and followed Ms2ger's advices.
For nsIScriptableRegion I haven't added [notxpcom], but used the static cast, because I haven't found much about it, indeed the only reference I've found about notxpcom says to preferably avoid its use.
Comment 8 Timothy Nikkel (:tnikkel) 2011-07-21 21:38:02 PDT
Comment on attachment 547582 [details] [diff] [review]
Patch revised

>diff -r 5d1799e16fe9 view/src/nsViewManager.cpp
>@@ -161,29 +161,29 @@ nsViewManager::~nsViewManager()
>-nsViewManager::CreateRegion(nsIRegion* *result)
>+nsViewManager::CreateRegion(nsIScriptableRegion* *result)

nsViewManager::CreateRegion seems to be unused. Let's just get rid of it and any gunk it requires.

>@@ -629,17 +629,17 @@ nsViewManager::UpdateWidgetArea(nsView *
> static PRBool
> ShouldIgnoreInvalidation(nsViewManager* aVM)
> {
>   while (aVM) {
>     nsIViewObserver* vo = aVM->GetViewObserver();
>-    if (!vo || vo->ShouldIgnoreInvalidation()) {
>+    if (vo && vo->ShouldIgnoreInvalidation()) {
>       return PR_TRUE;
>     }
>     nsView* view = aVM->GetRootViewImpl()->GetParent();
>     aVM = view ? view->GetViewManager() : nsnull;
>   }
>   return PR_FALSE;
> }

This change seems unrelated. Was there a reason for it?
Comment 9 Marco Castelluccio [:marco] 2011-07-22 03:45:02 PDT
Created attachment 547655 [details] [diff] [review]
Patch revised v2

Deleted nsViewManager::CreateRegion as it's unused.
The unrelated change was due to a modification in mozilla-central while I was working on the code.
Comment 10 Timothy Nikkel (:tnikkel) 2011-07-22 09:33:13 PDT
Comment on attachment 547655 [details] [diff] [review]
Patch revised v2

This then makes mRegionFactory unused in the view manager, lets get rid of it too. You can do this view system cleanup in another patch if you'd like.
Comment 11 Marco Castelluccio [:marco] 2011-07-22 10:03:15 PDT
Created attachment 547722 [details] [diff] [review]
Patch revised v3

I've deleted the useless variable. When I find variables and functions never used in the code, can I delete them without problems, or should I ask?
Comment 12 Timothy Nikkel (:tnikkel) 2011-07-22 10:07:54 PDT
Thanks.

You can delete them and then ask for review. :) What some people do is have a patch that does cleanup like that first and then the main patch on top of it. It depends on what the reviewer wants, how big the cleanup is, and how well it fits into the main patch.
Comment 13 Timothy Nikkel (:tnikkel) 2011-07-22 13:12:02 PDT
Comment on attachment 547722 [details] [diff] [review]
Patch revised v3

>diff -r 5d1799e16fe9 gfx/src/nsIScriptableRegion.idl
> [scriptable, uuid(4d179656-a5bd-42a6-a937-c81f820dcf2f)]
> interface nsIScriptableRegion : nsISupports

I think we'll need to bump the uuid here since we are changing the interface.

>diff -r 5d1799e16fe9 gfx/src/nsScriptableRegion.h
> #include "nsIScriptableRegion.h"
> #include "gfxCore.h"
>-#include "nsIRegion.h"
>+#include "nscore.h"
>+#include "nsISupports.h"
>+#include "nsRect.h"
>+#include "nsRegion.h"
> 
>-class nsIRegion;
>+class nsIntRegion;
>+
>+enum nsRegionComplexity
>+{
>+  eRegionComplexity_empty = 0,
>+  eRegionComplexity_rect = 1,
>+  eRegionComplexity_complex = 2
>+};
>+
>+typedef struct
>+{
>+  PRInt32   x;
>+  PRInt32   y;
>+  PRUint32  width;
>+  PRUint32  height;
>+} nsRegionRect;
>+
>+typedef struct
>+{
>+  PRUint32      mNumRects;    //number of actual rects in the mRects array
>+  PRUint32      mRectsLen;    //length, in rects, of the mRects array
>+  PRUint32      mArea;        //area of the covered portion of the region
>+  nsRegionRect  mRects[1];
>+} nsRegionRectSet;

> private:
>-	nsIRegion* mRegion;
>+	nsIntRegion mRegion;
> 	nsRegionRectSet *mRectSet;
> };

Do we need mRectSet at all? Let's get rid of it. And then do we need all the definitions above?

>diff -r 5d1799e16fe9 widget/src/xpwidgets/nsBaseDragService.cpp
>@@ -513,29 +512,24 @@ nsBaseDragService::DrawDrag(nsIDOMNode* 
>-  nsIntRegion clipRegion;
>+  nsIntRegion *clipRegion = NULL;

I think we are still using nsnull instead of NULL.

>   if (aRegion) {
>-    nsCOMPtr<nsIRegion> clipIRegion;
>-    aRegion->GetRegion(getter_AddRefs(clipIRegion));
>-    if (clipIRegion) {
>-      clipRegion = clipIRegion->GetUnderlyingRegion();
>-    }
>+    aRegion->GetRegion(&clipRegion);
>   }
> 
>   nsIntPoint pnt(aScreenDragRect->x, aScreenDragRect->y);
>   nsRefPtr<gfxASurface> surface =
>-    presShell->RenderNode(dragNode, aRegion ? &clipRegion : nsnull,
>-                          pnt, aScreenDragRect);
>+    presShell->RenderNode(dragNode, clipRegion, pnt, aScreenDragRect);

RenderNode actually changes the region it is passed so we need to make a copy of the region. This is the only user of the GetRegion function so I think we should just change it to return an nsIntRegion instead of a pointer to one.
Comment 14 Marco Castelluccio [:marco] 2011-07-23 05:26:10 PDT
Created attachment 547921 [details] [diff] [review]
Patch revised v4

Removed useless definitions and the variable mRectSet.
Modified the GetRegion function to copy the region, instead of returning a pointer to it.
Comment 15 Timothy Nikkel (:tnikkel) 2011-07-23 16:35:59 PDT
Comment on attachment 547921 [details] [diff] [review]
Patch revised v4

>diff -r 5d1799e16fe9 gfx/src/nsIScriptableRegion.idl
>--- a/gfx/src/nsIScriptableRegion.idl	Thu Jul 21 11:14:25 2011 -0400
>+++ b/gfx/src/nsIScriptableRegion.idl	Sat Jul 23 14:21:27 2011 +0200
>@@ -35,20 +35,20 @@
> [scriptable, uuid(4d179656-a5bd-42a6-a937-c81f820dcf2f)]
> interface nsIScriptableRegion : nsISupports

Can you change the uuid here? You can use http://mozilla.pettay.fi/cgi-bin/mozuuid.pl to get a new one.

>diff -r 5d1799e16fe9 gfx/src/nsScriptableRegion.cpp
>--- a/gfx/src/nsScriptableRegion.cpp	Thu Jul 21 11:14:25 2011 -0400
>+++ b/gfx/src/nsScriptableRegion.cpp	Sat Jul 23 14:21:27 2011 +0200
>@@ -39,125 +39,120 @@
>-nsScriptableRegion::nsScriptableRegion(nsIRegion* region) : mRegion(nsnull), mRectSet(nsnull)
>+nsScriptableRegion::nsScriptableRegion()
> {
>-	mRegion = region;
>-	NS_IF_ADDREF(mRegion);
> }
> 
> nsScriptableRegion::~nsScriptableRegion()
> {
>-  if (mRegion) {
>-    mRegion->FreeRects(mRectSet);
>-    NS_RELEASE(mRegion);
>-  }
> }

So we don't need the destructor at all, lets not implement it in nsScriptableRegion. Can we get rid of the empty constructor too or is it needed for something?

> NS_IMETHODIMP nsScriptableRegion::Init()
> {
>-	return mRegion->Init();
>+  mRegion.SetEmpty();
>+  return NS_OK;
> }

ns(Int)Regions are set empty by default I think, so we don't need to do anything for Init. I don't think we can remove it because existing code calls it and we can't break it.

> NS_IMETHODIMP nsScriptableRegion::SetToRegion(nsIScriptableRegion *aRegion)
> {
>-	nsCOMPtr<nsIRegion> region(do_QueryInterface(aRegion));
>-	mRegion->SetTo(*region);
>-	return NS_OK;
>+  mRegion = static_cast<nsScriptableRegion*>(aRegion)->mRegion;
>+  return NS_OK;
> }

I don't think there is anything that implements nsIScriptableRegion that can QI to nsIRegion in the tree currently, and I'm doubtful anyone implements such a thing outside the tree, so the old code would just crash if it was used currently.

For this and the other methods that take a nsIScriptableRegion I think we should get the nsIntRegion using GetRegion and use that because static casting from nsIScriptableRegion to nsScriptableRegion is not safe if any other class implements nsIScriptableRegion.

> NS_IMETHODIMP nsScriptableRegion::ContainsRect(PRInt32 aX, PRInt32 aY, PRInt32 aWidth, PRInt32 aHeight, PRBool *containsRect)
> {
>-	*containsRect = mRegion->ContainsRect(aX, aY, aWidth, aHeight);
>-	return NS_OK;
>+  nsIntRegion TmpRegion;
>+  TmpRegion.And(mRegion, nsIntRect(aX, aY, aWidth, aHeight));
>+  *containsRect = (!TmpRegion.IsEmpty());
>+  return NS_OK;
> }

Isn't there a Contains function on nsIntRegion that we can use here?

I still need to look over GetRects.

>diff -r 5d1799e16fe9 gfx/src/nsScriptableRegion.h
>--- a/gfx/src/nsScriptableRegion.h	Thu Jul 21 11:14:25 2011 -0400
>+++ b/gfx/src/nsScriptableRegion.h	Sat Jul 23 14:21:27 2011 +0200
>@@ -34,28 +34,27 @@
>+#include "nscore.h"
>+#include "nsISupports.h"
>+#include "nsRect.h"
>+#include "nsRegion.h"
> 
>-class nsIRegion;
>+class nsIntRegion;

Do we still need all these header additions? We shouldn't need to declare nsIntRegion since we are including nsRegion.h.


There are a number of other files that include nsIRegion.h, can you remove them? Use mxr (http://mxr.mozilla.org/mozilla-central/) and do a search for nsIRegion to make sure you got them all.
Comment 16 Marco Castelluccio [:marco] 2011-07-24 17:54:24 PDT
Created attachment 548064 [details] [diff] [review]
Patch revised v5
Comment 17 Timothy Nikkel (:tnikkel) 2011-07-24 22:43:38 PDT
Comment on attachment 548064 [details] [diff] [review]
Patch revised v5

>diff -r 5d1799e16fe9 gfx/src/nsScriptableRegion.cpp
>--- a/gfx/src/nsScriptableRegion.cpp	Thu Jul 21 11:14:25 2011 -0400
>+++ b/gfx/src/nsScriptableRegion.cpp	Mon Jul 25 02:53:27 2011 +0200
>@@ -165,40 +161,42 @@ NS_IMETHODIMP nsScriptableRegion::GetRec
>-    // This will contain bogus data if values don't fit in 31 bit

Keep this comment I think.

>+  nsIntRegionRectIterator ri(mRegion);

Call it iter is I think what we usually do.

>+  const nsIntRect *pSrc;

Call it rect.

>+  while ((pSrc = ri.Next())) {
>+    JS_DefineElement(cx, destArray, n, INT_TO_JSVAL(pSrc->x), NULL, NULL, JSPROP_ENUMERATE);
>+	JS_DefineElement(cx, destArray, n+1, INT_TO_JSVAL(pSrc->y), NULL, NULL, JSPROP_ENUMERATE);
>+	JS_DefineElement(cx, destArray, n+2, INT_TO_JSVAL(pSrc->width), NULL, NULL, JSPROP_ENUMERATE);
>+	JS_DefineElement(cx, destArray, n+3, INT_TO_JSVAL(pSrc->height), NULL, NULL, JSPROP_ENUMERATE);
>+
>+	n += 4;
>   }

Some tabs slipped in here.

Other than that it looks great! Thank you!
Comment 18 Marco Castelluccio [:marco] 2011-07-25 02:34:34 PDT
Created attachment 548130 [details] [diff] [review]
Patch revised v6

I've made the last changes.
Comment 19 Timothy Nikkel (:tnikkel) 2011-07-25 15:21:52 PDT
Comment on attachment 548130 [details] [diff] [review]
Patch revised v6

Great, thanks!

Let me know if you need any pointers with anything in the Mozilla universe.
Comment 20 Timothy Nikkel (:tnikkel) 2011-07-25 15:22:30 PDT
Landed on inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/22b20e5dcdce
Comment 21 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-26 04:03:50 PDT
http://hg.mozilla.org/mozilla-central/rev/22b20e5dcdce

Note You need to log in before you can comment on or make changes to this bug.