Closed Bug 635643 Opened 13 years ago Closed 13 years ago

remove nsIRegion

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: tnikkel, Assigned: marco)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=tnikkel@gmail.com][inbound])

Attachments

(1 file, 7 obsolete files)

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.
Whiteboard: [mentor=tnikkel@gmail.com]
Sorry, I'm stupid. I've uploaded the patch to the wrong bug!
Attachment #546929 - Attachment is obsolete: true
Attachment #546929 - Flags: review?(josh)
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?
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.
Did you try doing hg rm on the files instead of just deleting them from the file system?
Attached patch Patch revised (obsolete) — Splinter Review
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.
Attachment #547370 - Attachment is obsolete: true
Attachment #547370 - Flags: review?(tnikkel)
Attachment #547582 - Flags: review?(tnikkel)
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?
Attached patch Patch revised v2 (obsolete) — Splinter Review
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.
Attachment #547582 - Attachment is obsolete: true
Attachment #547582 - Flags: review?(tnikkel)
Attachment #547655 - Flags: review?(tnikkel)
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.
Attached patch Patch revised v3 (obsolete) — Splinter Review
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?
Attachment #547655 - Attachment is obsolete: true
Attachment #547655 - Flags: review?(tnikkel)
Attachment #547722 - Flags: review?(tnikkel)
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 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.
Assignee: nobody → mar.castelluccio
Attached patch Patch revised v4 (obsolete) — Splinter Review
Removed useless definitions and the variable mRectSet.
Modified the GetRegion function to copy the region, instead of returning a pointer to it.
Attachment #547722 - Attachment is obsolete: true
Attachment #547722 - Flags: review?(tnikkel)
Attachment #547921 - Flags: review?(tnikkel)
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.
Attached patch Patch revised v5 (obsolete) — Splinter Review
Attachment #547921 - Attachment is obsolete: true
Attachment #547921 - Flags: review?(tnikkel)
Attachment #548064 - Flags: review?(tnikkel)
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!
Attached patch Patch revised v6Splinter Review
I've made the last changes.
Attachment #548064 - Attachment is obsolete: true
Attachment #548064 - Flags: review?(tnikkel)
Attachment #548130 - Flags: review?(tnikkel)
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.
Attachment #548130 - Flags: review?(tnikkel) → review+
Landed on inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/22b20e5dcdce
Whiteboard: [mentor=tnikkel@gmail.com] → [mentor=tnikkel@gmail.com][inbound]
Blocks: 41701
http://hg.mozilla.org/mozilla-central/rev/22b20e5dcdce
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.