Closed Bug 984124 Opened 6 years ago Closed 6 years ago

Convert nsWebBrowserPersist::mURIMap to a modern hashtable

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: mccr8, Assigned: anujagarwal464)

References

Details

Attachments

(2 files, 3 obsolete files)

No description provided.
Can I work on this bug?
Sure.  I started looking at it, but there was some string stuff I didn't quite understand.
Assignee: nobody → anujagarwal464
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 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.
Attachment #8394113 - Flags: feedback?(continuation) → feedback+
> 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.
>      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.
Attachment #8394113 - Attachment is obsolete: true
Attachment #8394257 - Flags: feedback?(continuation)
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.
Attachment #8394257 - Flags: feedback?(continuation) → feedback+
Attachment #8394257 - Attachment is obsolete: true
Attachment #8394286 - Flags: feedback?(continuation)
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.
Attachment #8394286 - Flags: feedback?(continuation) → feedback+
I think this is ready for review.  You can just fix the indentation along when you fix whatever other review comments you get.
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.
Attachment #8394286 - Flags: review?(ehsan)
>     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.
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
Attachment #8393560 - Attachment is obsolete: true
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.
Attachment #8394286 - Flags: review?(ehsan) → review?(benjamin)
Attachment #8394286 - Flags: review?(benjamin) → review+
updated indentation.
Attachment #8398885 - Flags: review?(continuation)
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
Attachment #8398885 - Flags: review?(continuation)
Thanks for the patch!  Sorry it took me so long to land.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b38595abf68
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
There are some more details here about the rules for mozilla-inbound, if you are interested:
  https://wiki.mozilla.org/Tree_Rules/Integration
https://hg.mozilla.org/mozilla-central/rev/7b38595abf68
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.