Skip to content

Conversation

@codebytere
Copy link
Member

Resolves #12774.

Removes instances of Value::GetAs<TYPE> from atom/ and brightray/.

/cc @alexeykuzmin

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • commit messages or PR title follow semantic commit guidelines

@codebytere codebytere requested a review from a team June 25, 2018 17:54
@alexeykuzmin
Copy link
Contributor

Seems like a compiler on Windows doesn't like the change in atom/browser/net/url_request_async_asar_job.cc.

->GetString("path", &file_path);
} else if (options->is_string()) {
options->GetAsString(&file_path);
file_path = options->GetString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be changed to:

std::string file_path;
if (options->is_dict()) {
  auto path_value = options->FindKeyOfType("path", base::Value::Type::STRING);
  if (path_value)
    file_path = path_value->GetString();
} else if (options->is_string()) {
  file_path = options->GetString();
}

if (file_path.empty()) {
  NotifyStartError(net::URLRequestStatus(net::URLRequestStatus::FAILED,
                                         net::ERR_NOT_IMPLEMENTED));
} else {
  asar::URLRequestAsarJob::Initialize(
      base::CreateSequencedTaskRunnerWithTraits(
          {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}),
#if defined(OS_WIN)
      base::FilePath(base::UTF8ToWide(file_path)));
#else
      base::FilePath(file_path));
#endif
  asar::URLRequestAsarJob::Start();
}

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@codebytere codebytere merged commit 003a92e into master Jun 27, 2018
@codebytere codebytere deleted the remove-getas branch June 27, 2018 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants