New issue
Advanced search Search tips

Issue 728667 link

Starred by 1 user
Status: Fixed
Owner:
sorin@chromium.org
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Fix TODO: avoid DeepCopy of component manifest

Project Member Reported by sorin@chromium.org, Jun 1 2017 
This is a mechanical change.

It is changing the interface of the CRXInstaller so that we can eliminate such code below:

  // TODO(ddorwin): Change parameter to std::unique_ptr<base::DictionaryValue>
  // so we can avoid this DeepCopy.
  current_manifest_.reset(manifest.DeepCopy());
  std::unique_ptr<base::DictionaryValue> manifest_copy(
      current_manifest_->DeepCopy());
  main_task_runner_->PostTask(
      FROM_HERE,
      base::Bind(&DefaultComponentInstaller::ComponentReady,
                 this, base::Passed(&manifest_copy)));
  return result;

The change makes the types in the ComponentInstallerTraits interface more consistent, as it is using "const base::DictionaryValue& manifest" everywhere.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 2 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0bf4bd3f8a3111af1a7194607248a3649a1b7baf

commit 0bf4bd3f8a3111af1a7194607248a3649a1b7baf
Author: Sorin Jianu <sorin@chromium.org>
Date: Fri Jun 02 01:24:21 2017

Remove DefaultComponentInstaller::manifest_ member.

This change simplifies the scope and lifetime of the manifest_.

Bug:  728667 
Change-Id: Ia234f9cf6e56dac45c5f358f6b3cd8bdc6652ba3
Reviewed-on: https://chromium-review.googlesource.com/522202
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476512}
[modify] https://crrev.com/0bf4bd3f8a3111af1a7194607248a3649a1b7baf/components/component_updater/default_component_installer.cc
[modify] https://crrev.com/0bf4bd3f8a3111af1a7194607248a3649a1b7baf/components/component_updater/default_component_installer.h
[modify] https://crrev.com/0bf4bd3f8a3111af1a7194607248a3649a1b7baf/components/component_updater/default_component_installer_unittest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/990ee140b49d7e2de7a21dbbbc522a5c540d7c02

commit 990ee140b49d7e2de7a21dbbbc522a5c540d7c02
Author: Sorin Jianu <sorin@chromium.org>
Date: Tue Jun 06 17:54:09 2017

Fix TODO: avoid DeepCopy of component manifest.

This is a mechanical change. It resolves the following TODO by
changing the parameter type of CrxInstaller::Install.

// TODO(ddorwin): Change parameter to std::unique_ptr<base::DictionaryValue>
// so we can avoid this DeepCopy.
current_manifest_.reset(manifest.DeepCopy());
  std::unique_ptr<base::DictionaryValue> manifest_copy(
      current_manifest_->DeepCopy());
  main_task_runner_->PostTask(
      FROM_HERE,
      base::Bind(&DefaultComponentInstaller::ComponentReady,
                 this, base::Passed(&manifest_copy)));

Bug:  728667 
Change-Id: I4504fcea186d99d2efa1fd71ba3335279decd8ae
Reviewed-on: https://chromium-review.googlesource.com/522913
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477335}
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/cros_component_installer.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/cros_component_installer.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/pepper_flash_component_installer.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/pnacl_component_installer.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/pnacl_component_installer.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/recovery_component_installer.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/ssl_error_assistant_component_installer.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/component_updater/supervised_user_whitelist_installer_unittest.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/net/crl_set_fetcher.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/net/crl_set_fetcher.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/component_updater/component_updater_service_unittest.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/component_updater/default_component_installer.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/component_updater/default_component_installer.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/component_updater/default_component_installer_unittest.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/update_client/component.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/update_client/test_installer.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/update_client/test_installer.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/update_client/update_client.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/update_client/update_client_unittest.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/components/update_client/utils.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/extensions/browser/updater/update_install_shim.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/extensions/browser/updater/update_install_shim.h
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/extensions/browser/updater/update_service_unittest.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/ios/chrome/browser/net/crl_set_fetcher.cc
[modify] https://crrev.com/990ee140b49d7e2de7a21dbbbc522a5c540d7c02/ios/chrome/browser/net/crl_set_fetcher.h

Comment 3 by sorin@chromium.org, Jun 6 2017

Status: Fixed (was: Started)
►

Sign in to add a comment