Skip to content

Commit

Permalink
Revert of [gin] Add Arguments::GetAll() (patchset Pissandshittium#1 i…
Browse files Browse the repository at this point in the history
…d:1 of https://codereview.chromium.org/2824883002/ )

Reason for revert:
Broke Win64 Compile: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/10538

Original issue's description:
> [gin] Add Arguments::GetAll()
>
> Add an Arguments::GetAll() function that returns all arguments as a
> std::vector<v8::Local<v8::Value>>. This is more clear, concise, and
> slightly more performant than the alternative of using
> Arguments::GetRemaining() since it doesn't require trying to convert
> and avoids unnecessary calls.
>
> Add a test for the new method.
>
> BUG=None
>
> Review-Url: https://codereview.chromium.org/2824883002
> Cr-Commit-Position: refs/heads/master@{#465253}
> Committed: https://chromium.googlesource.com/chromium/src/+/3853b0bd67adec1b98b222196b89aca2468b9fe2

TBR=jochen@chromium.org,rdevlin.cronin@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=None

Review-Url: https://codereview.chromium.org/2820113004
Cr-Commit-Position: refs/heads/master@{#465268}
  • Loading branch information
ojanvafai authored and Commit bot committed Apr 18, 2017
1 parent 064a811 commit b3945af
Show file tree
Hide file tree
Showing 4 changed files with 5 additions and 82 deletions.
6 changes: 5 additions & 1 deletion extensions/renderer/event_emitter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,11 @@ void EventEmitter::Dispatch(gin::Arguments* arguments) {
v8::HandleScope handle_scope(arguments->isolate());
v8::Local<v8::Context> context =
arguments->isolate()->GetCurrentContext();
std::vector<v8::Local<v8::Value>> v8_args = arguments->GetAll();
std::vector<v8::Local<v8::Value>> v8_args;
if (arguments->Length() > 0) {
// Converting to v8::Values should never fail.
CHECK(arguments->GetRemaining(&v8_args));
}
Fire(context, &v8_args, nullptr);
}

Expand Down
13 changes: 0 additions & 13 deletions gin/arguments.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,6 @@ v8::Local<v8::Value> Arguments::PeekNext() const {
return (*info_)[next_];
}

std::vector<v8::Local<v8::Value>> Arguments::GetAll() const {
std::vector<v8::Local<v8::Value>> result;
int length = info_->Length();
if (length == 0)
return result;

result.reserve(length);
for (int i = 0; i < length; ++i)
result.push_back((*info_)[i]);

return result;
}

v8::Local<v8::Context> Arguments::GetHolderCreationContext() {
return info_->Holder()->CreationContext();
}
Expand Down
5 changes: 0 additions & 5 deletions gin/arguments.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,6 @@ class GIN_EXPORT Arguments {
// dereferencing the handle.
v8::Local<v8::Value> PeekNext() const;

// Returns all arguments. Since this doesn't require any conversion, it
// cannot fail. This does not rely on or modify the current position in the
// array used by Get/PeekNext().
std::vector<v8::Local<v8::Value>> GetAll() const;

void ThrowError() const;
void ThrowTypeError(const std::string& message) const;

Expand Down
63 changes: 0 additions & 63 deletions gin/arguments_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,67 +64,4 @@ TEST_F(ArgumentsTest, TestArgumentsHolderCreationContext) {
}
}

TEST_F(ArgumentsTest, TestGetAll) {
v8::Isolate* isolate = instance_->isolate();
v8::HandleScope handle_scope(isolate);
v8::Local<v8::Context> context = context_.Get(instance_->isolate());

using V8List = std::vector<v8::Local<v8::Value>>;

V8List list1 = {
gin::ConvertToV8(isolate, 1), gin::StringToV8(isolate, "some string"),
gin::ConvertToV8(context, std::vector<double>({2.0, 3.0}))
.ToLocalChecked(),
};
bool called1 = false;

V8List list2 = {
gin::StringToV8(isolate, "some other string"),
gin::ConvertToV8(isolate, 42),
};
bool called2 = false;

V8List list3; // Empty list.
bool called3 = false;

auto check_arguments = [](V8List* expected, bool* called,
gin::Arguments* arguments) {
*called = true;
V8List actual = arguments->GetAll();
ASSERT_EQ(expected->size(), actual.size());
for (size_t i = 0; i < expected->size(); ++i)
EXPECT_EQ(expected->at(i), actual[i]) << i;
};

// Create an object that will compare GetHolderCreationContext() with
// |creation_context|.
v8::Local<v8::ObjectTemplate> object_template =
ObjectTemplateBuilder(isolate)
.SetMethod("check1", base::Bind(check_arguments, &list1, &called1))
.SetMethod("check2", base::Bind(check_arguments, &list2, &called2))
.SetMethod("check3", base::Bind(check_arguments, &list3, &called3))
.Build();

v8::Local<v8::Object> object =
object_template->NewInstance(context).ToLocalChecked();

auto do_check = [object, context](V8List& args, base::StringPiece key) {
v8::Local<v8::Value> val;
ASSERT_TRUE(
object->Get(context, gin::StringToSymbol(context->GetIsolate(), key))
.ToLocal(&val));
ASSERT_TRUE(val->IsFunction());
val.As<v8::Function>()
->Call(context, object, args.size(), args.data())
.ToLocalChecked();
};

do_check(list1, "check1");
EXPECT_TRUE(called1);
do_check(list2, "check2");
EXPECT_TRUE(called2);
do_check(list3, "check3");
EXPECT_TRUE(called3);
}

} // namespace gin

0 comments on commit b3945af

Please sign in to comment.