New issue
Advanced search Search tips

Issue 749865 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Improve image_writer_private code

Project Member Reported by lazyboy@chromium.org, Jul 27 2017

Issue description

While working on TaskScheduler migration, I've noticed that there are a few improvements can be done to the code:

base::Callback-s in OperationManager do not need to be RepeatingCallback.
APIFunction's (e.g. ImageWriterPrivateDestroyFunction) response pattern is repetitive, move it to a common base class, etc.

Keeping this for the record so that I don't forget.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 4 2017

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

commit 4e90ca7e9969066963079f84caca52aeb36e95b3
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Fri Aug 04 22:33:44 2017

Include ImageWriterUtilityClientTest on linux.

ImageWriterUtilityClient is non existent on chromeos but is
available on linux, and there is no reason to run its tests on linux.

We already run these tests on win and mac.

Bug:  749865 
Change-Id: I3936836606470ca66f9df1da03902f89f13f9a17
Reviewed-on: https://chromium-review.googlesource.com/602489
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492151}
[modify] https://crrev.com/4e90ca7e9969066963079f84caca52aeb36e95b3/chrome/test/BUILD.gn

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 14 2017

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

commit 0a33b76b02e40fbb0314a5ae3dbcc270085e72b0
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Thu Sep 14 17:21:24 2017

Use base::OnceCallback for image_writer/ callbacks.

These callbacks are only called once, so base::OnceCallback is
appropriate instead of base::Callback.

Bug:  749865 
Change-Id: Ia25ccc919afe95cf38268940b10b5f315715790d
Reviewed-on: https://chromium-review.googlesource.com/666545
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501978}
[modify] https://crrev.com/0a33b76b02e40fbb0314a5ae3dbcc270085e72b0/chrome/browser/extensions/api/image_writer_private/operation.h
[modify] https://crrev.com/0a33b76b02e40fbb0314a5ae3dbcc270085e72b0/chrome/browser/extensions/api/image_writer_private/operation_manager.cc
[modify] https://crrev.com/0a33b76b02e40fbb0314a5ae3dbcc270085e72b0/chrome/browser/extensions/api/image_writer_private/operation_manager.h

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 15 2017

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

commit 3a3e6a1e3f69295e161467a05832756c6f9fa359
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Fri Sep 15 00:42:41 2017

Refactor common response pattern of ImageWriterPrivate functions.

This is a tiny cleanup to avoid repeating pattern of ImageWriterPrivate
api functions: 4 out of 5 functions have the following pattern of
responding to ExtensionFunction:
if (!success) error_ = ...
SendResponse(success);

Extract them out to a base class (ImageWriterPrivateBaseFunction). Also,
explicitly use base::BindOnce where appropriate.

Bug:  749865 
Test: None, internal cleanup.
Change-Id: Ie99a1ad4b3d9477361550c9cd6cfe2938251ed54
Reviewed-on: https://chromium-review.googlesource.com/666989
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502111}
[modify] https://crrev.com/3a3e6a1e3f69295e161467a05832756c6f9fa359/chrome/browser/extensions/api/image_writer_private/image_writer_private_api.cc
[modify] https://crrev.com/3a3e6a1e3f69295e161467a05832756c6f9fa359/chrome/browser/extensions/api/image_writer_private/image_writer_private_api.h

Status: Fixed (was: Started)
I'll close this one as both of the promised improvements are now complete.

Sign in to add a comment