984124 - Convert nsWebBrowserPersist::mURIMap to a modern hashtable

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Comment 1

[Anuj Agarwal [:anujagarwal464]]

Can I work on this bug?

Comment 2

[Andrew McCreight [:mccr8]]

Sure.  I started looking at it, but there was some string stuff I didn't quite understand.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Convert nsWebBrowserPersist::mURIMap to nsClassHashtable, work in progress.

Comment 4

[Andrew McCreight [:mccr8]]

Here's the patch I was working on.  As you can see, I was converting it to an nsClassHashtable.  In an nsClassHashtable, the hashtable owns the data, so whenever things are removed from it, it calls delete on the data.  By contrast, an nsDataHashtable doesn't free whatever there is.

Comment 5

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8394113 [details] [diff] [review]
Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

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

It looks pretty good!  I just have a few minor comments.

::: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
@@ +1762,5 @@
>  }
>  
>  void nsWebBrowserPersist::Cleanup()
>  {
> +    mURIMap.EnumerateRead(EnumCleanupURIMap, this);

You don't need to call EnumCleanupURIMap any more, so you can delete this call, and that method. This is because a ClassHashtable automatically will call delete on all of the data in the table when you call Clear() on it.

@@ +2434,2 @@
>  {
> +    uint32_t *count = static_cast<uint32_t *>(aClosure);

nit: no space between uint32_t and the *

@@ +2456,3 @@
>      nsCOMPtr<nsIURI> uri;
>      rv = NS_NewURI(getter_AddRefs(uri),
> +                   nsDependentCString(spec.get(), spec.Length()),

Hmm.  It might make sense to just pass in aKey here instead of creating a new nsDependentCString.  I'm not really sure though, I don't know a lot about how our strings work.

@@ +2502,5 @@
>      return PL_DHASH_NEXT;
>  }
>  
> +PLDHashOperator
> +nsWebBrowserPersist::EnumCleanupURIMap(const nsACString &aKey, URIData *aData, void* closure)

As I said above, you can remove this method.

@@ +3343,4 @@
>      {
>          return NS_ERROR_FAILURE;
>      }
> +    URIData *data = (URIData *) mURIMap.Get(spec);

You can remove this cast.

@@ +3723,5 @@
>      nsresult rv = aURI->GetSpec(spec);
>      NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>  
>      // Create a sensibly named filename for the URI and store in the URI map
> +    //nsCStringKey key(spec.get());

Just remove this line rather than commenting it out. :)

@@ +3730,2 @@
>      {
> +        data = (URIData *) mURIMap.Get(spec);

You can remove this cast.

::: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.h
@@ +153,1 @@
>      void EndDownload(nsresult aResult = NS_OK);

You should be able to remove the #include of nsHashtable.h in this file.

@@ +159,5 @@
>  
>      // Hash table enumerators
> +    static PLDHashOperator EnumPersistURIs(
> +        const nsACString &aKey, URIData *aData, void* aClosure);
> +    static PLDHashOperator EnumCleanupURIMap(

As I said in other comments, you can remove EnumCleanupURIMap.

Comment 7

[Andrew McCreight [:mccr8]]

> nit: no space between uint32_t and the *
Sorry, I meant, no space in the static_cast<> thing. The space is good as it is in the type declaration for the variable.

Comment 8

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

>      nsCOMPtr<nsIURI> uri;
>      rv = NS_NewURI(getter_AddRefs(uri),
> +                   nsDependentCString(spec.get(), spec.Length()),


Was getting parameter conversion error while using aKey directly therefore I created a new nsDependentCString.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8394257 [details] [diff] [review]
Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

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

Looks good, except that I think the other way you were doing nsDependentCString was better.  I think you always want to declare a local variable for the local auto cstring.

Comment 10

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8394286 [details] [diff] [review]
Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

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

Looks good!

::: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
@@ +2448,3 @@
>      }
>  
> +   nsWebBrowserPersist *pthis = static_cast<nsWebBrowserPersist*>(aClosure);

nit: It looks like the indentation here is off.

Comment 12

[Andrew McCreight [:mccr8]]

I think this is ready for review.  You can just fix the indentation along when you fix whatever other review comments you get.

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8394286 [details] [diff] [review]
Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

Ehsan, one thing I'm not sure about here is the creation of the nsDependentCString stuff in the call to NS_NewURI in EnumPersistURIs, so if you could pay particular attention to that, that would be good.  Thanks.

Comment 14

[Anuj Agarwal [:anujagarwal464]]

>     mOutputMap.EnumerateRead(EnumCleanupOutputMap, this);
>     mOutputMap.Clear();
>     mUploadList.EnumerateRead(EnumCleanupUploadList, this);
>     mUploadList.Clear();

If ClassHashtable automatically will call delete on all of the data in the table when you call Clear() on it. Then I think there is no need for EnumCleanupOutputMap and EnumCleanupUploadList.

Comment 15

[Andrew McCreight [:mccr8]]

Those two methods don't actually delete anything, they call "Cancel" on the channel being used as a key, so I think they are still needed.  The channel might be held alive by something else, but we still want to cancel it.  Or something like that.

Comment 16

[(no longer active)]

Comment on attachment 8394286 [details] [diff] [review]
Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

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

FWIW everything in the patch except for this string weirdness looks good to me.

::: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
@@ +2448,3 @@
>      }
>  
> +   nsWebBrowserPersist *pthis = static_cast<nsWebBrowserPersist*>(aClosure);

Yep, you're missing one space here.

@@ +2455,3 @@
>      nsCOMPtr<nsIURI> uri;
>      rv = NS_NewURI(getter_AddRefs(uri),
> +                   nsDependentCString(key.get(), key.Length()),

I don't understand why this is needed.  aKey is a const nsACString&.  NS_NewURI also receives a const nsACString& <http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#148>.

Bouncing to Benjamin.

Comment 17

[Anuj Agarwal [:anujagarwal464]]

Attachment: Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

updated indentation.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8398885 [details] [diff] [review]
Bug984124 - Convert nsWebBrowserPersist::mURIMap to nsClassHashtable

You don't need a re-review for just fixing some whitespace. :)

I'll land this assuming the try run is green:
  https://tbpl.mozilla.org/?tree=Try&rev=121fe4eb3060

Comment 19

[Andrew McCreight [:mccr8]]

Thanks for the patch!  Sorry it took me so long to land.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b38595abf68

Comment 20

[Andrew McCreight [:mccr8]]

I just landed the patch on mozilla-inbound, so the bug needs to stay open.  Once the patch is merged to mozilla-central, the main repository, it will be closed by whoever lands it on mozilla-central.  That should happen within a day.

Comment 21

[Andrew McCreight [:mccr8]]

There are some more details here about the rules for mozilla-inbound, if you are interested:
  https://wiki.mozilla.org/Tree_Rules/Integration

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b38595abf68