Uses a finalizer to delete zlib filter resources

Uses a finalizer to delete zlib filter resources

Also other misc. cleanup.

R=johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/202e077815e78e25482f3a0c82589aeb4065c441

[zra, 2016-03-08 22:37:31]

zra@google.com changed reviewers:
+ johnmccutchan@google.com

[zra, 2016-03-08 22:37:32]

Changes related to my comments on the zlib Mojo CL. Will transfer these changes
over there, next.

[zra, 2016-03-10 15:46:56]

ping

[Cutch, 2016-03-10 17:10:00]

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:43: ASSERT(dictionary != NULL);
Are we sure we want to move from throwing an exception to ASSERT (which won't do
anything in a released SDK)?

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:234: Dart_Handle
Filter::SetFilterPointerNativeField(Dart_Handle filter,
Consider renaming this method as it now does a lot more than set the native
field.

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:263: delete[] dictionary_;
delete is null safe. Here and elsewhere.

[zra, 2016-03-10 18:48:09]

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:43: ASSERT(dictionary != NULL);
On 2016/03/10 17:09:59, Cutch wrote:
> Are we sure we want to move from throwing an exception to ASSERT (which won't
do
> anything in a released SDK)?

I don't think we have consistent practices around this question. Using DartUtils
to throw an exception is probably wrong though. Changed to return NULL here, and
to directly call Dart_PropagateError(Dart_NewApiError(...)) at call sites, which
is also not great, but doesn't rely on DartUtils checking for error handles.

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:234: Dart_Handle
Filter::SetFilterPointerNativeField(Dart_Handle filter,
On 2016/03/10 17:09:59, Cutch wrote:
> Consider renaming this method as it now does a lot more than set the native
> field.

Done.

https://codereview.chromium.org/1777643003/diff/40001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:263: delete[] dictionary_;
On 2016/03/10 17:09:59, Cutch wrote:
> delete is null safe. Here and elsewhere.

Done.

[Cutch, 2016-03-10 18:59:14]

lgtm

[zra, 2016-03-10 23:42:03]

Description was changed from

==========
Uses a finalizer to delete zlib filter resources

Also other misc. cleanup.
==========

to

==========
Uses a finalizer to delete zlib filter resources

Also other misc. cleanup.

R=johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/202e077815e78e25482f3a0c82589aeb4065c441
==========

[zra, 2016-03-10 23:42:04]

Committed patchset #4 (id:60001) manually as
202e077815e78e25482f3a0c82589aeb4065c441 (presubmit successful).

[siva, 2016-03-11 12:48:19]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-03-11 12:48:19]

https://codereview.chromium.org/1777643003/diff/60001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/1777643003/diff/60001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:91: }
Why not change the signature of CopyDictionary to also return the dictionary
length which it is computed inside of it.
static Dart_Handle CopyDictionary(Dart_Handle dictionary_obj,
                                  uint8_t** dictionary,
                                  intptr_t* dictionary_length);

That would avoid the need for the subsequent call to Dart_Listlength.

https://codereview.chromium.org/1777643003/diff/60001/runtime/bin/filter.cc#n...
runtime/bin/filter.cc:130: bool raw = DartUtils::GetBooleanValue(raw_obj);
Maybe just:
bool raw;
Dart_GetNativeBooleanArgument(args, 7, &raw);

The same should hold for the int64_t values:
Dart_GetNativeIntegerArgument(args, 3, &window_bits);