Closed
Bug 635643
Opened 13 years ago
Closed 13 years ago
remove nsIRegion
Categories
(Core :: Graphics, defect)
Core
Graphics
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)
41.04 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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.
Blocks: deCOM
Reporter | ||
Updated•13 years ago
|
Whiteboard: [mentor=tnikkel@gmail.com]
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #546929 -
Flags: review?(josh)
Assignee | ||
Comment 2•13 years ago
|
||
Sorry, I'm stupid. I've uploaded the patch to the wrong bug!
Updated•13 years ago
|
Attachment #546929 -
Attachment is obsolete: true
Attachment #546929 -
Flags: review?(josh)
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #547370 -
Flags: review?(tnikkel)
Comment 4•13 years ago
|
||
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?
Assignee | ||
Comment 5•13 years ago
|
||
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.
Reporter | ||
Comment 6•13 years ago
|
||
Did you try doing hg rm on the files instead of just deleting them from the file system?
Assignee | ||
Comment 7•13 years ago
|
||
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)
Reporter | ||
Comment 8•13 years ago
|
||
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?
Assignee | ||
Comment 9•13 years ago
|
||
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)
Reporter | ||
Comment 10•13 years ago
|
||
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.
Assignee | ||
Comment 11•13 years ago
|
||
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)
Reporter | ||
Comment 12•13 years ago
|
||
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.
Reporter | ||
Comment 13•13 years ago
|
||
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.
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → mar.castelluccio
Assignee | ||
Comment 14•13 years ago
|
||
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)
Reporter | ||
Comment 15•13 years ago
|
||
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.
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #547921 -
Attachment is obsolete: true
Attachment #547921 -
Flags: review?(tnikkel)
Attachment #548064 -
Flags: review?(tnikkel)
Reporter | ||
Comment 17•13 years ago
|
||
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!
Assignee | ||
Comment 18•13 years ago
|
||
I've made the last changes.
Attachment #548064 -
Attachment is obsolete: true
Attachment #548064 -
Flags: review?(tnikkel)
Attachment #548130 -
Flags: review?(tnikkel)
Reporter | ||
Comment 19•13 years ago
|
||
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+
Reporter | ||
Comment 20•13 years ago
|
||
Landed on inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/22b20e5dcdce
Whiteboard: [mentor=tnikkel@gmail.com] → [mentor=tnikkel@gmail.com][inbound]
Comment 21•13 years ago
|
||
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.
Description
•