Closed Bug 474505 Opened 15 years ago Closed 12 years ago

Replace uses of nsVoidPtrHashkey with nsPtrHashKey<T>

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: roc, Assigned: hessamms)

References

()

Details

(Whiteboard: [good first bug] [mentor=ehsan])

Attachments

(1 file, 6 obsolete files)

We can stop using nsVoidPtrHashkey and start using the typed version.
Whiteboard: [good first bug]
When this is complete, we should just remove nsVoidPtrHashKey.
Yeah, but we shouldn't be using it since it gives us less typechecking and requires more type casts.
Assignee: nobody → ambient.sounds
Status: NEW → ASSIGNED
I think it's safe to say that this is not being worked on at present.
Assignee: ambient.sounds → nobody
Whiteboard: [good first bug] → [good first bug] [mentor=jdm]
I'm stealing the mentorship from jdm on this one (hope you don't mind Josh!).
Whiteboard: [good first bug] [mentor=jdm] → [good first bug] [mentor=ehsan]
I'm working on this . I'll try to make a patch for all files , Of course because this is my first contribute to bugzilla I might I can not fix all the files correctly. However I will try.
Assignee: nobody → hessamms
Attached patch first patch (obsolete) — Splinter Review
Comment on attachment 562820 [details] [diff] [review]
first patch

Thanks very much Hessam, this is looking really good. I'm not sure if |const void| is necessary in all of those changes rather than just |void|, but I'm sure Ehsan will know. It's great to see a bunch of these turning into real types instead of void pointers, too :)

Do you know what the deal is with those huge blocks of changes in nsDocument that don't seem to make any actual changes? Did you make any modifications in there? If not, can you just revert that file and make the other changes again?

>-  array->AppendElement(static_cast<nsSVGRenderingObserver*>(
>-          const_cast<void*>(aEntry->GetKey())));
>+  aEntry->GetKey();

This change is incorrect, since you lose the array->AppendElement.

Other than that, this patch looks promising! Good work!
Attached patch first patch (FIXED) (obsolete) — Splinter Review
Attachment #562820 - Attachment is obsolete: true
Thank you for reviewing my patch .
I have corrected the NsDocument file and array->AppendElement according to your comment. about converting |const void| to |void| I'm waiting for ehsan comment .
Comment on attachment 563143 [details] [diff] [review]
first patch (FIXED)

Presumably Ehsan does not have the clearance to review every single one of these changes, but I'll let him deal with that.
Attachment #563143 - Flags: feedback?(ehsan)
Comment on attachment 563143 [details] [diff] [review]
first patch (FIXED)

Review of attachment 563143 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good overall!  Thanks a lot for working on this.  I've made some suggestions on places where we could possibly use real types instead of void.  If you can fix those please, we can submit this for the final review round!  :-)

::: accessible/src/base/nsAccCache.h
@@ +65,5 @@
>   * Clear the cache and shutdown the accessibles.
>   */
>  template <class T>
>  static void
> +ClearCache(nsRefPtrHashtable<nsPtrHashKey<const void>, T> & aCache)

I think the only hashtable passed to this is nsAccessibleHashtable (defined in nsAccessible.h).  Can you make this a non-template function which takes an nsAccessibleHashtable& parameter?  It would definitely make this easier to read.

@@ +93,5 @@
>   * Traverse the accessible cache for cycle collector.
>   */
>  template <class T>
>  static void
> +CycleCollectorTraverseCache(nsRefPtrHashtable<nsPtrHashKey<const void>, T> & aCache,

Same here.

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +732,5 @@
>  
>    TurnOffNewTabSwitchingForJawsAndWE();
>  }
>  
> +nsRefPtrHashtable<nsPtrHashKey<void>, nsDocAccessible> nsAccessNodeWrap::sHWNDCache;

We should be able to use nsPtrHashKey<HWND> here, right?  And then we should be able to get rid of the void* cast further down in nsAccessNodeWrap::WindowProc when calling GetWeak on the hashtable.

::: accessible/src/msaa/nsAccessNodeWrap.h
@@ +169,5 @@
>  
>    static LRESULT CALLBACK WindowProc(HWND hWnd, UINT Msg,
>                                       WPARAM WParam, LPARAM lParam);
>  
> +  static nsRefPtrHashtable<nsPtrHashKey<void>, nsDocAccessible> sHWNDCache;

Same here, we should be able to use HWND.

::: content/base/src/nsDocument.h
@@ +1126,5 @@
>    PRUint8 mXMLDeclarationBits;
>  
>    PRUint8 mDefaultElementType;
>  
> +  nsInterfaceHashtable<nsPtrHashKey<nsIContent>, nsPIBoxObject> *mBoxObjectTable;

This looks very good, it's the correct type that we want to use as the hashtable key.

::: content/xbl/src/nsBindingManager.cpp
@@ +1331,5 @@
>  
>    return NS_OK;
>  }
>  
> +typedef nsTHashtable<nsPtrHashKey<const void> > RuleProcessorSet;

I think we can just use nsPtrHashKey<nsIStyleRuleProcessor>, and get rid of the related casts down the file.

The same goes for the other two places in this file which you've edited.

::: dom/base/nsGlobalWindow.h
@@ +964,5 @@
>    PRBool mCleanedUp, mCallCleanUpAfterModalDialogCloses;
>  
>    nsCOMPtr<nsIDOMOfflineResourceList> mApplicationCache;
>  
> +  nsDataHashtable<nsPtrHashKey<nsXBLPrototypeHandler>, void*> mCachedXBLPrototypeHandlers;

This looks very good!

::: dom/ipc/TabChild.cpp
@@ +373,5 @@
>      win.forget(aReturn);
>      return NS_OK;
>  }
>  
> +static nsInterfaceHashtable<nsPtrHashKey<void>, nsIDialogParamBlock> gActiveDialogs;

You should be able to use nsPtrHashKey<ContentDialogChild>.

::: dom/plugins/ipc/PluginInstanceParent.h
@@ +333,5 @@
>      const NPNetscapeFuncs* mNPNIface;
>      NPWindowType mWindowType;
>      int mQuirks;
>  
> +    nsDataHashtable<nsPtrHashKey<NPObject>, PluginScriptableObjectParent*> mScriptableObjects;

Looks great!

::: dom/plugins/ipc/PluginModuleParent.h
@@ +315,5 @@
>      bool mShutdown;
>      bool mClearSiteDataSupported;
>      bool mGetSitesWithDataSupported;
>      const NPNetscapeFuncs* mNPNIface;
> +    nsDataHashtable<nsPtrHashKey<void>, PluginIdentifierParent*> mIdentifiers;

You should be able to use nsPtrHashKey<NPIdentifier>.

::: dom/src/storage/nsDOMStorage.h
@@ +83,5 @@
>  using mozilla::dom::StorageParent;
>  
>  class DOMStorageImpl;
>  
> +class nsDOMStorageEntry : public nsPtrHashKey<const void>

Hmm, we could probably get rid of this class altogether and just use nsPtrHashKey<DOMStorageImpl> instead of it, but I'm not sure if that's going to work, and we don't need to handle that in this bug.  This looks good enough for now.

::: dom/src/threads/nsDOMThreadService.h
@@ +187,5 @@
>    // mCreationsInProgress.
>    mozilla::ReentrantMonitor mReentrantMonitor;
>  
>    // A map from nsDOMWorkerThread to nsDOMWorkerRunnable.
> +  nsRefPtrHashtable<nsPtrHashKey<nsDOMWorker>, nsDOMWorkerRunnable> mWorkersInProgress;

These look great!

::: dom/workers/RuntimeService.h
@@ +94,5 @@
>    // Protected by mMutex.
>    nsTArray<IdleThreadInfo> mIdleThreadArray;
>  
>    // *Not* protected by mMutex.
> +  nsClassHashtable<nsPtrHashKey<nsPIDOMWindow>, nsTArray<WorkerPrivate*> > mWindowMap;

This one is also great!

::: gfx/thebes/GLContext.h
@@ +1072,5 @@
>      // while preserving current ClearColor etc. values.
>      // Useful for resizing offscreen buffers.
>      void ClearSafely();
>  
> +    nsDataHashtable<nsPtrHashKey<void>, void*> mUserData;

This one looks good.  We're actually using void* pointers to key the hashtable!

::: js/src/xpconnect/src/xpcprivate.h
@@ +602,5 @@
>      PLDHashTable             mJSRoots;
>  #endif
>      nsAutoPtr<XPCCallContext> mCycleCollectionContext;
>  
> +    typedef nsBaseHashtable<nsPtrHashKey<const void>, nsISupports*, nsISupports*> ScopeSet;

Looks good.  We _could_ go with void instead of const void, but I don't care much!

::: layout/base/nsPresContext.h
@@ +1057,5 @@
>    // a specific language, however (e.g, if it is inferred from the
>    // charset rather than explicitly specified as a lang attribute).
>    nsIAtom*              mLanguage;      // [STRONG]
>  
> +  nsRefPtrHashtable<nsPtrHashKey<nsIFrame>, nsImageLoader>

This is very good!

::: layout/svg/base/src/nsSVGEffects.h
@@ +268,5 @@
>     */
>    void RemoveAll();
>  
>  private:
> +  nsTHashtable<nsPtrHashKey<nsSVGRenderingObserver> > mObservers;

Awesome!

::: layout/svg/base/src/nsSVGOuterSVGFrame.h
@@ +167,5 @@
>  
>    // A hash-set containing our nsSVGForeignObjectFrame descendants. Note we use
>    // a hash-set to avoid the O(N^2) behavior we'd get tearing down an SVG frame
>    // subtree if we were to use a list (see bug 381285 comment 20).
> +  nsTHashtable<nsPtrHashKey<nsSVGForeignObjectFrame> > mForeignObjectHash;

Super!

::: toolkit/components/remote/nsGTKRemoteService.h
@@ +79,5 @@
>  
>    virtual void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
>                                                PRUint32 aTimestamp);
>  
> +  nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;

This is very good!

::: xpcom/base/nsCycleCollector.cpp
@@ +732,5 @@
>  };
>  
>  // XXX Would be nice to have an nsHashSet<KeyType> API that has
>  // Add/Remove/Has rather than PutEntry/RemoveEntry/GetEntry.
> +typedef nsTHashtable<nsPtrHashKey<const void> > PointerSet;

As said before, we _could_ go with void here too, but I don't care much!

::: xpcom/threads/nsThreadManager.h
@@ +87,5 @@
>    }
>    
>    static nsThreadManager sInstance;
>  
> +  nsRefPtrHashtable<nsPtrHashKey<PRThread>, nsThread> mThreadsByPRThread;

Awesome!
Attachment #563143 - Flags: feedback?(ehsan) → feedback+
Attached patch patch v3 (obsolete) — Splinter Review
Thanks for reviewing my patch Ehsan. I fixed all the issues mentioned above .
Attachment #563143 - Attachment is obsolete: true
Comment on attachment 563992 [details] [diff] [review]
patch v3

I think this is ready for review now!
Attachment #563992 - Flags: review?(benjamin)
I've submitted this patch to the try server: http://tbpl.mozilla.org/?tree=Try&rev=f9042a973e06
Try run for f9042a973e06 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f9042a973e06
Results (out of 79 total builds):
    exception: 2
    success: 66
    warnings: 2
    failure: 9
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-f9042a973e06
Hesam, seems like there are two compilation problems with the patch.

The first one happens on Linux.  See this log for example: <https://tbpl.mozilla.org/php/getParsedLog.php?id=6658104&tree=Try&full=1>, which seems to have been caused because you haven't handled this case: <http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceParent.cpp#1423>.

The second one happens on Windows.  See this log for example: <https://tbpl.mozilla.org/php/getParsedLog.php?id=6658489&tree=Try&full=1>.  This one seems to be my fault.  :-)

I recommended you to do this:

-nsRefPtrHashtable<nsVoidPtrHashKey, nsDocAccessible> nsAccessNodeWrap::sHWNDCache;
+nsRefPtrHashtable<nsPtrHashKey<HWND>, nsDocAccessible> nsAccessNodeWrap::sHWNDCache;

HWND on Windows is defined as a pointer to a structure, something similar to this:

struct HWND__ {
  int dummy;
};
typedef HWND__* HWND;

Using nsPtrHashKey<HWND> makes the KeyType HWND* (or, HWND__**), not HWND.  That was my mistake.  I think you should go back to using nsPtrHashKey<void> (I'm very sorry about the wrong comment.)

Can you please submit a new patch addressing these two issues?

Thanks!
Comment on attachment 563992 [details] [diff] [review]
patch v3

Clearing review request because of tryserver build bustage. I'll be happy to review a revised version!
Attachment #563992 - Flags: review?(benjamin)
Attached patch patch v4 (obsolete) — Splinter Review
It's a little bit late, Sorry. :)
I have updated the patch so that it works with the latest updated source code, at least on my machine, Please submit this patch to the try server again. thanks.
Attachment #563992 - Attachment is obsolete: true
Whiteboard: [good first bug] [mentor=ehsan] → [good first bug] [mentor=ehsan][autoland-try]
Whiteboard: [good first bug] [mentor=ehsan][autoland-try] → [good first bug] [mentor=ehsan][autoland-in-queue]
Autoland Patchset:
	Patches: 564299, 605933
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Whiteboard: [good first bug] [mentor=ehsan][autoland-in-queue] → [good first bug] [mentor=ehsan]
Attachment #564299 - Attachment is obsolete: true
Comment on attachment 605933 [details] [diff] [review]
patch v4

Marking f+ to satisfy the autoland bot...
Attachment #605933 - Flags: feedback+
Whiteboard: [good first bug] [mentor=ehsan] → [good first bug] [mentor=ehsan][autoland-try]
Whiteboard: [good first bug] [mentor=ehsan][autoland-try] → [good first bug] [mentor=ehsan][autoland-in-queue]
Autoland Patchset:
	Patches: 605933
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Whiteboard: [good first bug] [mentor=ehsan][autoland-in-queue] → [good first bug] [mentor=ehsan]
Try run for b332b544c583 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b332b544c583
Results (out of 113 total builds):
    success: 87
    warnings: 17
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-b332b544c583
(In reply to Mozilla RelEng Bot from comment #25)
>     https://tbpl.mozilla.org/?tree=Try&rev=b332b544c583

Looking at the build failures here, it seems like there is a usage of nsVoidPtrHashKey in dom/plugins/ipc/PluginInstanceParent.h/cpp (see the mScriptableObjects member.)

Can you please fix that as well, Hessam?  Thanks!
Attached patch patch v5 (obsolete) — Splinter Review
PluginInstanceParent and nsAccessNodeWrap fixed.
Attachment #605933 - Attachment is obsolete: true
Try run for d83deb02e6ee is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d83deb02e6ee
Results (out of 169 total builds):
    exception: 1
    success: 134
    warnings: 31
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-d83deb02e6ee
Attached patch patch v6Splinter Review
Attachment #606340 - Attachment is obsolete: true
Try run for 8faf0e760f9d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8faf0e760f9d
Results (out of 216 total builds):
    exception: 3
    success: 172
    warnings: 27
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-8faf0e760f9d
Comment on attachment 606541 [details] [diff] [review]
patch v6

This version of the patch compiles fine on all platforms.
Attachment #606541 - Flags: review?(benjamin)
Attachment #606541 - Flags: review?(benjamin) → review+
Thanks a lot for your patch, Hessam!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6cb28e6c61c
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/a6cb28e6c61c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: