984123 - Convert nsControllerCommandGroup::mGroupsHash and nsGroupsEnumerator::mHashtable to modern hash tables

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Andrew McCreight [:mccr8]]

I started looking at this, but the groups enumerator thing makes it a bit weird.

Comment 1

[Anuj Agarwal [:anujagarwal464]]

Can I work on this?

Comment 2

[Andrew McCreight [:mccr8]]

Sure.  Thanks.

Comment 3

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug984123-part1.diff incomplete

Andrew, does this look OK to you? 
I will update mHashTable in next patch.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8413187 [details] [diff] [review]
bug984123-part1.diff incomplete

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

It would be good to keep the removal of mGroupsHash in a separate patch from the removal of mHashtable, to make it easier to review.  When you do that, each patch should compile.  So don't remove the #include "nsHashtable.h" until the second patch.

This looks pretty good to me.  You mostly just need to fix up ClearGroupsHash() like I describe below.

::: embedding/components/commandhandler/src/nsCommandGroup.cpp
@@ -215,5 @@
>  
>  void
>  nsControllerCommandGroup::ClearGroupsHash()
>  {
> -    mGroupsHash.Reset(ClearEnumerator, (void *)this);

I think you still need to call ClearEnumerator, because nsTArray will not automatically call delete on the char* it contains.  You will still end up wanting to delete the "delete commandList" from this method.  Instead of whatever this Reset() method is, you'll want to pass it to Enumerate, and fix that up so it works.  You can just pass in |nullptr| for the closure argument, instead of this, because I don't think ClearEnumerator uses it.

Then, once ClearEnumerator has been run, you can call Clear() on mGroupsHash.

@@ +226,5 @@
>  /* void addCommandToGroup (in DOMString aCommand, in DOMString aGroup); */
>  NS_IMETHODIMP
>  nsControllerCommandGroup::AddCommandToGroup(const char * aCommand, const char *aGroup)
>  {
> +  nsDependentCString groupKey(aGroup);  

While you are here, you should remove this trailing space.

@@ +308,5 @@
>  /* nsISimpleEnumerator getEnumeratorForGroup (in DOMString aGroup); */
>  NS_IMETHODIMP
>  nsControllerCommandGroup::GetEnumeratorForGroup(const char * aGroup, nsISimpleEnumerator **_retval)
>  {
> +  nsDependentCString groupKey(aGroup);  

While you are already modifying this line, remove the trailing whitespace, please.

::: embedding/components/commandhandler/src/nsCommandGroup.h
@@ +37,5 @@
>  
>  protected:
>  
> +  nsClassHashtable<nsCStringHashKey, nsTArray<char*> > mGroupsHash;		// hash keyed on command group.
> +                                    									// Entries are nsVoidArrays of pointers to char16_t*

You can delete this comment about Entries because it is very wrong, and was wrong even before your patch. :)

Comment 5

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug984123.diff with warnings

Because of this
http://dxr.mozilla.org/mozilla-central/source/embedding/components/commandhandler/src/nsCommandGroup.cpp#302
converted both nsControllerCommandGroup::mGroupsHash and nsGroupsEnumerator::mHashtable in single patch.

Patch complies successfully but getting following warnings

>0:08.81 Warning: C4018 in c:\projects\mozilla\mozilla-build-prac\embedding\comp
>onents\commandhandler\src\nsCommandGroup.cpp: '<' : signed/unsigned mismatch
>0:08.81 c:\projects\mozilla\mozilla-build-prac\embedding\components\commandhand
>ler\src\nsCommandGroup.cpp(72) : warning C4018: '<' : signed/unsigned mismatch
>0:08.81 Warning: C4018 in c:\projects\mozilla\mozilla-build-prac\embedding\comp
>onents\commandhandler\src\nsCommandGroup.cpp: '>=' : signed/unsigned mismatch
>0:08.81 c:\projects\mozilla\mozilla-build-prac\embedding\components\commandhand
>ler\src\nsCommandGroup.cpp(90) : warning C4018: '>=' : signed/unsigned mismatch

Comment 6

[Andrew McCreight [:mccr8]]

> Patch complies successfully but getting following warnings
Hmm.  The problem there is that nsHashtable uses a signed int for its Count(), whereas the newer hashtable wrappers use an unsigned int.  Maybe the way to fix that is just to change the call to Count() in the two places with the warning to static_cast<int32_t>(mHashTable.Count()).  I think it is unlikely that mHashtable will get so big that will overflow.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8413241 [details] [diff] [review]
bug984123.diff with warnings

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

This looks good, I just have a few minor things to fix up.

> converted both nsControllerCommandGroup::mGroupsHash and nsGroupsEnumerator::mHashtable in single patch.

Sounds reasonable.

::: embedding/components/commandhandler/src/nsCommandGroup.cpp
@@ +18,5 @@
>  class nsGroupsEnumerator : public nsISimpleEnumerator
>  {
>  public:
> +
> +  nsGroupsEnumerator(nsClassHashtable<nsCStringHashKey, nsTArray<char*> >& inHashTable);

Please define a typedef for "nsClassHashtable<nsCStringHashKey, nsTArray<char*> >" in the header, maybe called GroupsHashtable, and then use it in the header and this file.  That way if the type ends up changing later, it can be easily updated.  Plus it is easier to read, because you can tell at a glance that it is the same sort of hash table, and not some slight variation.

@@ -28,5 @@
>  protected:
>  
> -  static bool HashEnum(nsHashKey *aKey, void *aData, void* aClosure);
> -
> -  nsresult      Initialize();

This file had some weird spacing!

@@ +99,5 @@
>    return CallQueryInterface(supportsString, _retval);
>  }
>  
>  /* static */
>  /* return false to stop */

You can delete the comment "return false to stop" because it isn't true any more, and this wasn't returning false anyways. :)

@@ +106,2 @@
>  {
> +  nsGroupsEnumerator* groupsEnum = reinterpret_cast<nsGroupsEnumerator *>(aClosure);

While you are modifying this anyways, please change this to static_cast instead of reinterpret_cast.  In Firefox code, that is the standard way to case these void* closure arguments to their real type.

@@ +109,2 @@
>    
> +  groupsEnum->mGroupNames[groupsEnum->mIndex] = (char*)stringKey.get();

It would be nice to avoid extra string creation.  Can |(char*)stringKey.get();| just be |aKey.get()|?

@@ +134,5 @@
>  
>  class nsNamedGroupEnumerator : public nsISimpleEnumerator
>  {
>  public:
> +  

nit: Please remove the trailing whitespace.

@@ +184,5 @@
>    mIndex ++;
>    if (mIndex >= int32_t(mGroupArray->Length()))
>      return NS_ERROR_FAILURE;
>      
> +  char16_t *thisGroupName = (char16_t*)mGroupArray->ElementAt(mIndex);

It seems bad that we're casting a char* to a char16_t*, but just leave it alone.  If that is actually a problem, it can be dealt with elsewhere.

@@ +214,5 @@
>  
>  void
>  nsControllerCommandGroup::ClearGroupsHash()
>  {
> +  // TODO: Call nsControllerCommandGroup::ClearEnumerator

You can delete this TODO now it looks like. :)

@@ +215,5 @@
>  void
>  nsControllerCommandGroup::ClearGroupsHash()
>  {
> +  // TODO: Call nsControllerCommandGroup::ClearEnumerator
> +  mGroupsHash.EnumerateRead(ClearEnumerator, this);

ClearEnumerator doesn't use its closure argument, so please just pass in |nullptr| for the second argument to EnumerateRead, instead of |this|.

@@ +227,5 @@
>  /* void addCommandToGroup (in DOMString aCommand, in DOMString aGroup); */
>  NS_IMETHODIMP
>  nsControllerCommandGroup::AddCommandToGroup(const char * aCommand, const char *aGroup)
>  {
> +  nsDependentCString groupKey(aGroup);  

While you are modifying this line anyways, please delete the whitespace at the end of the line.

@@ +309,5 @@
>  /* nsISimpleEnumerator getEnumeratorForGroup (in DOMString aGroup); */
>  NS_IMETHODIMP
>  nsControllerCommandGroup::GetEnumeratorForGroup(const char * aGroup, nsISimpleEnumerator **_retval)
>  {
> +  nsDependentCString groupKey(aGroup);  

While you are modifying this line anyways, please remove the trailing whitespace.

::: embedding/components/commandhandler/src/nsCommandGroup.h
@@ +36,2 @@
>  
>  protected:

while you are here, you can just delete this second |protected:|.

@@ +40,4 @@
>  
>  protected:
>  
> +  nsClassHashtable<nsCStringHashKey, nsTArray<char*> > mGroupsHash;

The "hash keyed on command group." and "This could be made more space-efficient, maybe with atoms" part of it could still be in here somewhere, as I think they are still relevant.

Comment 8

[Anuj Agarwal [:anujagarwal464]]

>It would be nice to avoid extra string creation.  Can |(char*)stringKey.get();| just be |aKey.get()|?

'get' is not a member of 'nsACString_internal'

If I remember correctly, we did same thing here-
http://dxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp?from=nsWebBrowserPersist.cpp#2456

Comment 9

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug984123.diff

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8413398 [details] [diff] [review]
bug984123.diff

This looks good to me.  Let's see how a try run does.
  https://tbpl.mozilla.org/?tree=Try&rev=bf6b0cc40d90

> If I remember correctly, we did same thing here
Ah, ok, I'd forgotten.

Comment 11

[Andrew McCreight [:mccr8]]

The try run looked good.  Feel free to ask :ehsan for review when you are ready.

Comment 12

[(no longer active)]

Comment on attachment 8413398 [details] [diff] [review]
bug984123.diff

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

::: embedding/components/commandhandler/src/nsCommandGroup.cpp
@@ +13,5 @@
>  #include "nsCommandGroup.h"
>  #include "nsIControllerCommand.h"
>  #include "nsCRT.h"
>  
> +typedef nsClassHashtable<nsCStringHashKey, nsTArray<char*> > GroupsHashtable;

Nit: please move this typedef to the header, address my nit about the space there, and use it in the header as well.  You probably want to make the typedef a public member of the nsCommandGroup class.

@@ +103,4 @@
>  {
> +  nsGroupsEnumerator *groupsEnum = static_cast<nsGroupsEnumerator*>(aClosure);
> +  nsAutoCString stringKey = nsAutoCString(aKey);
> +  groupsEnum->mGroupNames[groupsEnum->mIndex] = (char*)stringKey.get();

This is wrong.  You're making mGroupNames point to something on your stack, which would be invalid once you return from this function.  (This is a problem that exists in the previous version of the code as well, of course.)

@@ +231,3 @@
>    }
>    // add the command to the list. Note that we're not checking for duplicates here
> +  char *commandString = NS_strdup(aCommand); // we store allocated char16_t* in the array

Why not make the values in the hashtable nsTArray<nsCString>'s or some such in order to avoid manual memory allocation here?

::: embedding/components/commandhandler/src/nsCommandGroup.h
@@ +32,3 @@
>  
>  protected:
> +  nsClassHashtable<nsCStringHashKey, nsTArray<char*> > mGroupsHash; // hash keyed on command group.

Nit: no space between the > >.

Comment 13

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug984123.diff -updated

Comment 14

[(no longer active)]

Comment on attachment 8415350 [details] [diff] [review]
bug984123.diff -updated

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

r=me with the GroupsHashtable comment addressed.

::: embedding/components/commandhandler/src/nsCommandGroup.cpp
@@ +228,3 @@
>    }
>    // add the command to the list. Note that we're not checking for duplicates here
> +  char *commandString = NS_strdup(aCommand); // we store allocated char16_t* in the array

I see that you chose to not address my comment about this!  I feel bad about holding your patch for this as this is pre-existing code...

::: embedding/components/commandhandler/src/nsCommandGroup.h
@@ +16,5 @@
>  
>  #define NS_CONTROLLER_COMMAND_GROUP_CONTRACTID \
>   "@mozilla.org/embedcomp/controller-command-group;1"
>  
> +typedef nsClassHashtable<nsCStringHashKey, nsTArray<char*>> GroupsHashtable;

Please move this to be a member of nsControllerCommandGroup to prevent polluting the global namespace.

Comment 15

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug984123.diff - part1 r=ehsan

>I see that you chose to not address my comment about this!

Sorry, I just missed your comment. I will upload a separate patch for that.

Comment 16

[Andrew McCreight [:mccr8]]

Let me know if you need any help, Anuj.

Comment 17

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug914031-1.diff r=ehsan

Comment 18

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug914031-2.diff

Comment 19

[Anuj Agarwal [:anujagarwal464]]

@Andrew: Sorry for delay.

In nsCommandGroup.cpp nsNamedGroupEnumerator::GetNext
getting error on supportsString->SetData(thisGroupName);

> cannot convert from 'nsCString' to 'const nsAString_internal'

Comment 20

[Andrew McCreight [:mccr8]]

Comment on attachment 8420527 [details] [diff] [review]
bug914031-2.diff

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

This looks reasonable to me, but I'm not very familiar with string stuff, which is what this patch is mostly dealing with.

::: embedding/components/commandhandler/src/nsCommandGroup.cpp
@@ +178,5 @@
>    if (mIndex >= int32_t(mGroupArray->Length()))
>      return NS_ERROR_FAILURE;
>      
> +  nsCString thisGroupName = mGroupArray->ElementAt(mIndex);
> +  NS_ASSERTION((char*)thisGroupName.get(), "Bad Element in mGroupArray");

I think you don't need to do this NS_ASSERTION for nsCString, as I think it can't hold a null string, just an empty one.

@@ +230,4 @@
>      mGroupsHash.Put(groupKey, commandList);
>    }
>    // add the command to the list. Note that we're not checking for duplicates here
> +  nsCString commandString = nsCString(aCommand); // we store allocated char16_t* in the array

This looks odd.  Can you just directly do AppendElement(aCommand) below?  I'm not really sure what the right way to convert it is, if that doesn't work.
You can also get rid of that comment at the end of the line, I think.

@@ +230,5 @@
>      mGroupsHash.Put(groupKey, commandList);
>    }
>    // add the command to the list. Note that we're not checking for duplicates here
> +  nsCString commandString = nsCString(aCommand); // we store allocated char16_t* in the array
> +  //if (!commandString) return NS_ERROR_OUT_OF_MEMORY;

Delete this commented out line.

@@ +253,5 @@
>    uint32_t numEntries = commandList->Length();
>    for (uint32_t i = 0; i < numEntries; i++)
>    {
> +    nsCString commandString = commandList->ElementAt(i);
> +    if (nsCString(aCommand) != commandString)

Does this work, or do you need to call Equals?  I'm not sure.

@@ +278,5 @@
>    uint32_t numEntries = commandList->Length();
>    for (uint32_t i = 0; i < numEntries; i++)
>    {
> +    nsCString commandString = commandList->ElementAt(i);
> +    if (nsCString(aCommand) != commandString)

Same thing as above, for Equals.

@@ +309,1 @@
>    if (!theGroupEnum) return NS_ERROR_OUT_OF_MEMORY;

You can delete this line, as new is infallible.

@@ +315,5 @@
>  #pragma mark -
>  #endif
>  
>  PLDHashOperator
> +nsControllerCommandGroup::ClearEnumerator(const nsACString &aKey, nsTArray<nsCString> *aData, void *closure)

nsTArray calls the destructor of all of its elements, so you can delete ClearEnumerator, and remove the place that calls it.  As you can see, this method isn't actually doing anything any more. :)

::: embedding/components/commandhandler/src/nsCommandGroup.h
@@ +40,3 @@
>  
>  protected:
> +	nsClassHashtable<nsCStringHashKey, nsTArray<nsCString>> mGroupsHash; // hash keyed on command group.

Just make the type GroupsHashtable.

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to Anuj Agarwal [:anujagarwal464] from comment #19)
> @Andrew: Sorry for delay.
It is no problem, I just wanted to make sure you aren't stuck.

> In nsCommandGroup.cpp nsNamedGroupEnumerator::GetNext
> getting error on supportsString->SetData(thisGroupName);
> 
> > cannot convert from 'nsCString' to 'const nsAString_internal'

Hmm.  That's odd.  I don't know how you are supposed to do that conversion.

Comment 22

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug914031-2.diff

Comment 23

[(no longer active)]

Comment on attachment 8420564 [details] [diff] [review]
bug914031-2.diff

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

::: embedding/components/commandhandler/src/nsCommandGroup.cpp
@@ +177,5 @@
>    mIndex++;
>    if (mIndex >= int32_t(mGroupArray->Length()))
>      return NS_ERROR_FAILURE;
>      
> +  nsCString thisGroupName = mGroupArray->ElementAt(mIndex);

const nsCString& please.

@@ +182,3 @@
>    
>    nsresult rv;
> +  nsCOMPtr<nsISupportsCString> supportsString = do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID, &rv);

Hmm, so nsGroupEnumerator returns an nsISupportsCString now and nsNamedGroupEnumerator returns an nsISupportsString.  That makes little sense to me so changing that here should be OK but there is a chance that this breaks a consumer, so please make sure you test this on try.

@@ +248,5 @@
>    uint32_t numEntries = commandList->Length();
>    for (uint32_t i = 0; i < numEntries; i++)
>    {
> +    nsCString commandString = commandList->ElementAt(i);
> +    if (nsCString(aCommand) != commandString)

Nit: nsDependentCString.

@@ +272,5 @@
>    uint32_t numEntries = commandList->Length();
>    for (uint32_t i = 0; i < numEntries; i++)
>    {
> +    nsCString commandString = commandList->ElementAt(i);
> +    if (nsCString(aCommand) != commandString)

Ditto.

::: embedding/components/commandhandler/src/nsCommandGroup.h
@@ +35,3 @@
>  
>  protected:
> + GroupsHashtable mGroupsHash; // hash keyed on command group.

Nit: indentation.

Comment 24

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug914031-2.diff

I don't have access to Try.

Comment 25

[Andrew McCreight [:mccr8]]

here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=f9901df45a2d

Comment 26

[Andrew McCreight [:mccr8]]

L64 debug try run was green.

Comment 27

[Andrew McCreight [:mccr8]]

Anuj, please also remove the include of nsHashtable.h from part 1, as that should not be needed any more.

Comment 28

[(no longer active)]

Comment on attachment 8420659 [details] [diff] [review]
bug914031-2.diff

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

::: embedding/components/commandhandler/src/nsCommandGroup.h
@@ +6,5 @@
>  #ifndef nsCommandGroup_h__
>  #define nsCommandGroup_h__
>  
>  #include "nsIController.h"
>  #include "nsHashtable.h"

Like Andrew mentioned, please remove this.

Comment 29

[Anuj Agarwal [:anujagarwal464]]

Attachment: Part-1 r=ehsan

Comment 30

[Anuj Agarwal [:anujagarwal464]]

Attachment: Part-2 r=ehsan

@Andrew: Thanks!

Comment 31

[Andrew McCreight [:mccr8]]

I'll land this today or tomorrow.

Comment 32

[Andrew McCreight [:mccr8]]

Thanks for the patches!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5eda757f4ad8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22994a1e0af4

Comment 33

[Andrew McCreight [:mccr8]]

I backed out and re-landed the second part because I forgot to set the author correctly on the patch.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/11695e267488
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e1adb8f51fa9

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5eda757f4ad8
https://hg.mozilla.org/mozilla-central/rev/e1adb8f51fa9