983990 - Convert nsPluginStreamListenerPeer::mDataForwardToRequest to nsDataHashtable<nsUint32HashKey, uint32_t>

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Andrew McCreight [:mccr8]]

The table currently uses 32-bit keys, but it looks like the comments like "XXX support 64-bit offsets" can be addressed by using 64-bit keys and dropping the conversions.

Comment 1

[Andrew McCreight [:mccr8]]

Thanks, let me know if you have any questions!

Comment 2

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug983990.diff

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginStreamListenerPeer.cpp#849

do we need this now?

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8391893 [details] [diff] [review]
bug983990.diff

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

Looking over the code some more now, I think I gave bad advice about switching things to 64-bit.  Sorry about that.  I think that for now it makes sense to  convert the hash table to nsDataHashtable<nsUint32HashKey, uint32_t>, using absoluteOffset instead of absoluteOffset64.  Converting this code properly over to 64-bit will require some wider-ranging code changes, so it makes sense to do that in a separate bug (if that is something that is even still desired).  Of course, changing the hash table should make that easier to do.  When you update your patch for that, I can take a look at it.

::: dom/plugins/base/nsPluginStreamListenerPeer.cpp
@@ +823,2 @@
>        int32_t amtForwardToPlugin =
> +      NS_PTR_TO_INT32(mDataForwardToRequest->Get(absoluteOffset64));

You should be able to get rid of NS_PTR_TO_INT32.  I think this was needed because nsHashtable was storing void* as its data, but with nsDataHashtable you'll get the actual int32_t out of it that you need.

Comment 4

[Andrew McCreight [:mccr8]]

Other than those comments, your patch looks good.

Comment 5

[Anuj Agarwal [:anujagarwal464]]

Attachment: nsDataHashtable<nsUint32HashKey, uint32_t>

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8391920 [details] [diff] [review]
nsDataHashtable<nsUint32HashKey, uint32_t>

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

This looks good to me, thanks. I'll forward this to John for a review.

Comment 7

[Anuj Agarwal [:anujagarwal464]]

(In reply to Andrew McCreight [:mccr8] from comment #3)

> You should be able to get rid of NS_PTR_TO_INT32.  I think this was needed
> because nsHashtable was storing void* as its data, but with nsDataHashtable
> you'll get the actual int32_t out of it that you need.

I will get uint32_t from nsDataHashtable. I think i should type cast it to int32_t before assigning.

Comment 8

[John Schoenick [:johns]]

Comment on attachment 8391920 [details] [diff] [review]
nsDataHashtable<nsUint32HashKey, uint32_t>

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

lgtm

::: dom/plugins/base/nsPluginStreamListenerPeer.cpp
@@ +889,5 @@
>      int64_t absoluteOffset64 = 0;
>      brr->GetStartRange(&absoluteOffset64);
>      // XXX support 64-bit offsets
>      int32_t absoluteOffset = (int32_t)int64_t(absoluteOffset64);
> +        

Whitespace! Not that this file isn't covered in it.

Comment 9

[Andrew McCreight [:mccr8]]

I can fix the whitespace when I land it.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Remove all trailing whitespace from nsPluginStreamListenerPeer.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8392407 [details] [diff] [review]
Remove all trailing whitespace from nsPluginStreamListenerPeer.

rs=bsmedberg over IRC

Comment 12

[Andrew McCreight [:mccr8]]

Thanks!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56dc258068bc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8fdfcc4878db

Comment 13

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=659d8f58b430

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56dc258068bc
https://hg.mozilla.org/mozilla-central/rev/8fdfcc4878db