Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663603002:
Convert utility process ImageWriter IPC to mojo (Closed)
|
Created:
3 years, 10 months ago by Noel Gordon Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert utility process ImageWriter IPC to mojo
Add extensions::mojo::RemovableStorageWriter
Define a new mojo interface that is exposed by the utility process to the
browser. Update the extensions private api image writer caller to use the
new interface, rather than IPC, for image writing.
Add mojo RemovableStorageWriterClient interface
Define a mojo interface to send to utility process image writer, which it
can use to send Write and Verify operation progress and completion status
back to the caller.
Use content::UtilityProcessMojoClient
The code used content::UtilityProcessHost to control the utility process.
Replace that with the mojo utility client. The mojo channel that connects
the caller and the utility process can now be intentionally disconnected,
from either end, to implement the Cancel operation. Add a browsertest for
WriteCancel and VerifyCancel.
Enforce image_writer_utility_client state
The original code had state, but never checked the state assumption: that
a WRITE or VERIFY operation could be in progress - just never at the same
time. Add ResetRequest() between operations to reset state. Add DCHECK at
when any new operation is requested to DCHECK that the previous operation
if any has been completed.
Remove the utility process IPC whitelist
The image writer IPC (removed in this CL) were the last remaining entries
in the utility process IPC whitelist (these IPC ran elevated on WIN). The
whitelist is now unused. Remove all its code and associated files. Define
a new member utility_process_running_elevated_, and use it to prevent IPC
from being processed if the utility process is elevated.
Add image_writer_utility_client_browsertest
Again a utility process service with no test. Add a browsertest for Write
and Verify operations (invalid, valid, and cancel). Note: the browsertest
can't test the low-level USB driver code on any port since our bots don't
have USB sticks. To test that aspect, I manually tested with the ChromeOS
Recovery Tool [1] and was able to write and verify a ChromeOS image to my
USB stick on both Windows and Mac OSX.
ImageWriterHandler
Add a test mode to allow the class to be browser-tested (with the caveats
mentioned above). Add reference to bug 352442: file handles to the device
and image files must remain open between Write and Verify operations. Add
WriteVerify test fixture to specifically test that case.
[1] http://bit.ly/2jFtsZZ
BUG=680928
Review-Url: https://codereview.chromium.org/2663603002
Cr-Commit-Position: refs/heads/master@{#449296}
Committed: https://chromium.googlesource.com/chromium/src/+/6b03ae83bd948f7389a359acfd3ef8055c142c45
Patch Set 1 #Patch Set 2 : Rename test NoSource to NoImage. #Patch Set 3 : Whitelist gone: remove whitelist OWNERS file. #Patch Set 4 : Remove redundant fwd declaration. #Patch Set 5 : Use UtilityProcess rather than Host, add VerifyFailure test. #Patch Set 6 : Remove image_writer::error references that could bit-rot. #Patch Set 7 : Expose utility process mojo respecting WIN elevation. #Patch Set 8 : Remove test DLOG, minor name changes. #
Total comments: 28
Patch Set 9 : Review comments. #
Total comments: 17
Patch Set 10 : Review comments. #
Total comments: 5
Patch Set 11 : Use https: in bug references, minor comment fixes. #Messages
Total messages: 89 (64 generated)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Convert utility process ImageWriter IPC to mojo BUG=680928 ========== to ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler: Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add the WriteVerify fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ==========
noel@chromium.org changed reviewers: + dcheng@chromium.org, sammc@chromium.org, tibell@chromium.org
Description was changed from ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler: Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add the WriteVerify fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ========== to ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add the WriteVerify fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ==========
+ sammc, tibell for the mojo + dcheng for IPC security
https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:8: #include "base/callback.h" Remove. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:52: ImageWriterUtilityClient* const image_writer_utility_client_; Comment that |image_writer_utility_client_| owns |this|. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:145: OperationFailed("Utility process mojo connection error"); Is this message used for anything user-facing? https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:71: extensions::mojom::RemovableStorageWriter* UtilityProcessService() { UtilityService* utility_process_service() { https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:87: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; const https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:89: typedef extensions::mojom::RemovableStorageWriter UtilityService; Typedefs before methods. using UtilityService = extensions::mojom::RemovableStorageWriter; https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:86: bool success_ = false; These should be private with public getter methods. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:202: static void FillFile(const base::FilePath& path, int pattern) { Make this a function in an anonymous namespace. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:203: std::unique_ptr<char[]> buffer(new char[kTestFileSize]); std::vector<char> buffer(kTestFileSize, pattern); and make pattern a char. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:252: FillImageFileWithPattern(0x55555555); How about changing the pattern across tests? https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:174: const IPC::Message& message) { Shouldn't this always just return false if running elevated? https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:14: namespace { https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:15: inline bool IsTestDevice(const base::FilePath& device) { inline is not required. https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:23: } // namespace
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:8: #include "base/callback.h" On 2017/02/02 21:54:17, Sam McNally wrote: > Remove. Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:52: ImageWriterUtilityClient* const image_writer_utility_client_; On 2017/02/02 21:54:17, Sam McNally wrote: > Comment that |image_writer_utility_client_| owns |this|. Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:145: OperationFailed("Utility process mojo connection error"); On 2017/02/02 21:54:17, Sam McNally wrote: > Is this message used for anything user-facing? A chrome extension could could show it if it wanted. Looking at diff-left, the error_callback messages like this were: "IPC communication failed" "Utility process crashed with code %08x." (printf-ed) "Process launch failed with code %08x. (printf-ed) On a punt, I assumed your question was maybe about having "mojo" in the error message, and looking at the other error messages above, I changed the error message to: "Utility process crashed or failed." https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:71: extensions::mojom::RemovableStorageWriter* UtilityProcessService() { On 2017/02/02 21:54:17, Sam McNally wrote: > UtilityService* utility_process_service() { Removed this instead. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:87: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2017/02/02 21:54:17, Sam McNally wrote: > const Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:89: typedef extensions::mojom::RemovableStorageWriter UtilityService; On 2017/02/02 21:54:17, Sam McNally wrote: > Typedefs before methods. The effect I was attempting to achieve with the typedef wouldn't quite work for me if we did this. I removed the typedef instead, and wrote things out long-hand as we normally do with the content::UtilityProcessMojoClient. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:86: bool success_ = false; On 2017/02/02 21:54:17, Sam McNally wrote: > These should be private with public getter methods. Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:202: static void FillFile(const base::FilePath& path, int pattern) { On 2017/02/02 21:54:17, Sam McNally wrote: > Make this a function in an anonymous namespace. I'd prefer to leave it as an internal detail of this class. much like CopyFile from our previous browsertest on out-of-process file patching. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:203: std::unique_ptr<char[]> buffer(new char[kTestFileSize]); On 2017/02/02 21:54:17, Sam McNally wrote: > std::vector<char> buffer(kTestFileSize, pattern); Ah, ain't std::vector<> all grown-up these days. Done. > and make pattern a char. Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:252: FillImageFileWithPattern(0x55555555); On 2017/02/02 21:54:17, Sam McNally wrote: > How about changing the pattern across tests? Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/chrome_... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/chrome_... chrome/utility/chrome_content_utility_client.cc:174: const IPC::Message& message) { On 2017/02/02 21:54:17, Sam McNally wrote: > Shouldn't this always just return false if running elevated? Yeap, just checking you ain't jet-lagged. Added utility_process_running_elevated_ and used it where filter_messages_ was before. https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:14: On 2017/02/02 21:54:17, Sam McNally wrote: > namespace { Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:15: inline bool IsTestDevice(const base::FilePath& device) { On 2017/02/02 21:54:17, Sam McNally wrote: > inline is not required. Done. https://codereview.chromium.org/2663603002/diff/160001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:23: On 2017/02/02 21:54:17, Sam McNally wrote: > } // namespace Done.
Description was changed from ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add the WriteVerify fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ========== to ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Define a new member utility_process_running_elevated_, and use it to prevent IPC from being processed if the process is elevated. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add WriteVerify test fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ==========
Description was changed from ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Define a new member utility_process_running_elevated_, and use it to prevent IPC from being processed if the process is elevated. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add WriteVerify test fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ========== to ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Define a new member utility_process_running_elevated_, and use it to prevent IPC from being processed if the utility process is elevated. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add WriteVerify test fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ==========
lgtm
https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:158: ResetRequest(); It seems like we call ResetRequest() a lot more, rather than just in Shutdown(). Is there any particular reason this changed? If so, it might be nice to document this in the CL description or somewhere else. https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:74: CHECK_NE(option, WRITE); // Verify tests do not WRITE. We should ASSERT rather than CHECK in tests. https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:123: EXPECT_EQ(progress_, kTestFileSize); Nit: expected value should be on left https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:172: EXPECT_EQ(progress_, kTestFileSize); Nit: expected value should be on left https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:214: registry->AddInterface(base::Bind(&RemovableStorageWriterImpl::Create)); Out of curiosity, why can't this just talk to ImageWriterHandler directly? https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:17: extensions::mojom::RemovableStorageWriter::kTestDevice; Seeing this makes me kind of feel that this should just define this as a FILE_PATH_LITERAL and leave it outside the mojom; it seems like it should be possible to do this with a simple comparison without constructing temporaries, etc. (It makes me slightly sad to have testing code closely interleaved into production interfaces... for this sort of thing, does it make sense to use a test interface for the browser test?) https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:122: if (image_writer_.get()) Nit: no .get()
lgtm https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:73: void OperationProgress(int64_t progress); Optional: What's |progress|? A percentage?
https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.h:73: void OperationProgress(int64_t progress); On 2017/02/06 22:36:22, tibell wrote: > Optional: What's |progress|? A percentage? Nope, not a percentage. It's the number of bytes written so far in a file WRITE operation, or the number of bytes verified so far in a file VERIFY operation. Caller knows the total number of bytes of the file, and converts |progress| to a percentage of that for display.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client.cc:158: ResetRequest(); On 2017/02/06 05:52:02, dcheng wrote: > It seems like we call ResetRequest() a lot more, rather than just in Shutdown(). > Is there any particular reason this changed? If so, it might be nice to document > this in the CL description or somewhere else. a lot more -> once per operation, which seems ideal. It makes this code stateful - it wasn't before, and it wasn't clear if a VERIFY can be requested while a WRITE is in progress. DCHECK(!removable_storage_writer_client_) at the start of these operation now state that no existing operation should be in progress. That was the original code contract, it just wasn't ever enforced. https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:74: CHECK_NE(option, WRITE); // Verify tests do not WRITE. On 2017/02/06 05:52:02, dcheng wrote: > We should ASSERT rather than CHECK in tests. Done. https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:123: EXPECT_EQ(progress_, kTestFileSize); On 2017/02/06 05:52:02, dcheng wrote: > Nit: expected value should be on left Done. https://codereview.chromium.org/2663603002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/image_writer_private/image_writer_utility_client_browsertest.cc:172: EXPECT_EQ(progress_, kTestFileSize); On 2017/02/06 05:52:02, dcheng wrote: > Nit: expected value should be on left Done. https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:214: registry->AddInterface(base::Bind(&RemovableStorageWriterImpl::Create)); On 2017/02/06 05:52:02, dcheng wrote: > Out of curiosity, why can't this just talk to ImageWriterHandler directly? Maybe it could, but the goal here was not to have exactly one instance ImageWriterHandler, as a member of the mojo class, as it was in the original code, so it can be kept alive b/w Write and Verify operations, so that any open file handles remain open b/w operations. And all this to avoid bug http://crbug.com/352442. https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:17: extensions::mojom::RemovableStorageWriter::kTestDevice; On 2017/02/06 05:52:02, dcheng wrote: > Seeing this makes me kind of feel that this should just define this as a > FILE_PATH_LITERAL and leave it outside the mojom; I don't really mind either way, but other reviewers prefer it in the mojom, maybe because in code search you can search for [ extensions::mojom::RemovableStorageWriter ] and find all the related stuff. Both ways have merits I suppose. > it seems like it should be > possible to do this with a simple comparison without constructing temporaries, > etc. Not sure why constructing temporaries would matter. The file we write is a ChromeOS image, they are 2GByte or more in size based on my testing, and the image write operation takes order 10 minutes on my USB stick. > (It makes me slightly sad to have testing code closely interleaved into > production interfaces... Makes me even more sad to see production interfaces with no tests. Testing when a child process is involved is hard, and I have invent things like kTestDevice just to have the child and browser process co-ordinate during a test. for this sort of thing, does it make sense to use a > test interface for the browser test?) kTestDevice is kinda that, but is a compromise. Adding test-only API to mojo interfaces is an idea I've had also, but the idea got no traction in off-line discussion. https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:122: if (image_writer_.get()) On 2017/02/06 05:52:02, dcheng wrote: > Nit: no .get() Done.
mojo lgtm https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:17: extensions::mojom::RemovableStorageWriter::kTestDevice; On 2017/02/07 11:31:09, noel gordon wrote: > On 2017/02/06 05:52:02, dcheng wrote: > > Seeing this makes me kind of feel that this should just define this as a > > FILE_PATH_LITERAL and leave it outside the mojom; > > I don't really mind either way, but other reviewers prefer it in the mojom, > maybe because in code search you can search for > > [ extensions::mojom::RemovableStorageWriter ] > > and find all the related stuff. Both ways have merits I suppose. > > > it seems like it should be > > possible to do this with a simple comparison without constructing temporaries, > > etc. > > Not sure why constructing temporaries would matter. The file we write is a > ChromeOS image, they are 2GByte or more in size based on my testing, and the > image write operation takes order 10 minutes on my USB stick. It's true that it's a fairly minor cost, but it does add up -- creating and destroying a temporary std::string is surprisingly heavy in terms of binary size (which does matter somewhat, even if this is never hit). > > > (It makes me slightly sad to have testing code closely interleaved into > > production interfaces... > > Makes me even more sad to see production interfaces with no tests. Testing when > a child process is involved is hard, and I have invent things like kTestDevice > just to have the child and browser process co-ordinate during a test. > > for this sort of thing, does it make sense to use a > > test interface for the browser test?) > > kTestDevice is kinda that, but is a compromise. Adding test-only API to mojo > interfaces is an idea I've had also, but the idea got no traction in off-line > discussion. I was kind of thinking that we would just substitute a completely separate mojo interface when we're in test mode. I don't actually know how we would accomplish that though. Basically, it's not clear to me how scalable this test mode approach will be. If it's just a few interfaces, maybe it's not worth thinking too much about. https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:45: // http://crbug.com/352442 Nit: pointer to the bug is good, but a brief explanation of why this logic is necessary would be useful as well Also, http => https ;-) https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:83: // http://crbug.com/352442 Ditto.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:45: // http://crbug.com/352442 On 2017/02/08 09:31:18, dcheng wrote: > Nit: pointer to the bug is good, but a brief explanation of why this logic is > necessary would be useful as well > > Also, http => https ;-) Done. I think the URL is the best pointer I can give to the details. Also a brief explanation of the bug is given in the ChangeLog, and that saves me repeating myself twice :) Comments are not code, and too often don't match the actual code, leading to maintenance burden and other issues. Better to rephrase them into actual code if possible. It's not always possible, so ensure the comment adds value. [1] [1] https://lists.webkit.org/pipermail/webkit-dev/2011-January/015769.html https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:83: // http://crbug.com/352442 On 2017/02/08 09:31:18, dcheng wrote: > Ditto. Done.
Description was changed from ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Define a new member utility_process_running_elevated_, and use it to prevent IPC from being processed if the utility process is elevated. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add WriteVerify test fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ========== to ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Enforce image_writer_utility_client state The original code had state, but never checked the state assumption: that a WRITE or VERIFY operation could be in progress - just never at the same time. Add ResetRequest() between operations to reset state. Add DCHECK at when any new operation is requested to DCHECK that the previous operation if any has been completed. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Define a new member utility_process_running_elevated_, and use it to prevent IPC from being processed if the utility process is elevated. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add WriteVerify test fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ==========
On 2017/02/08 09:31:19, dcheng wrote: > mojo lgtm > > https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... > File chrome/utility/image_writer/image_writer_handler.cc (right): > > https://codereview.chromium.org/2663603002/diff/180001/chrome/utility/image_w... > chrome/utility/image_writer/image_writer_handler.cc:17: > extensions::mojom::RemovableStorageWriter::kTestDevice; > On 2017/02/07 11:31:09, noel gordon wrote: > > On 2017/02/06 05:52:02, dcheng wrote: > > > Seeing this makes me kind of feel that this should just define this as a > > > FILE_PATH_LITERAL and leave it outside the mojom; > > > > I don't really mind either way, but other reviewers prefer it in the mojom, > > maybe because in code search you can search for > > > > [ extensions::mojom::RemovableStorageWriter ] > > > > and find all the related stuff. Both ways have merits I suppose. > > > > > it seems like it should be > > > possible to do this with a simple comparison without constructing > temporaries, > > > etc. > > > > Not sure why constructing temporaries would matter. The file we write is a > > ChromeOS image, they are 2GByte or more in size based on my testing, and the > > image write operation takes order 10 minutes on my USB stick. > > It's true that it's a fairly minor cost, but it does add up -- creating and > destroying a temporary std::string is surprisingly heavy in terms of binary size > (which does matter somewhat, even if this is never hit). > > > > > > (It makes me slightly sad to have testing code closely interleaved into > > > production interfaces... > > > > Makes me even more sad to see production interfaces with no tests. Testing > when > > a child process is involved is hard, and I have invent things like kTestDevice > > just to have the child and browser process co-ordinate during a test. > > > > for this sort of thing, does it make sense to use a > > > test interface for the browser test?) > > > > kTestDevice is kinda that, but is a compromise. Adding test-only API to mojo > > interfaces is an idea I've had also, but the idea got no traction in off-line > > discussion. > > I was kind of thinking that we would just substitute a completely separate mojo > interface when we're in test mode. I don't actually know how we would accomplish > that though. Nor do I. The idea of having a separate mojo test interface was the idea I mentioned that got shot down :) > Basically, it's not clear to me how scalable this test mode > approach will be. If it's just a few interfaces, maybe it's not worth thinking > too much about. This "test mode" approach has only turned up in the few special cases: mojo wifi credentials bots are not connected to any wifi network. mojo image writer bots do not have USB sticks in them. and it has been useful in these cases to allow testing more of the API surface, but not all of course. I would for now advise that we restrict it to these special cases, rather than try to scale it. Finally, in doing this patch, I noted that the bug component for image-writer needs some love. Might be better to throw it all into Eraser if we don't care about it this component, and so I added an entry for image-writer therein (viz., in Eraser). There are others ways to do what image-writer does, so maybe this code does not need to exist in Chrome at all. We'll see.
noel@chromium.org changed reviewers: + jochen@chromium.org
+ jochen for OWNERS.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... File chrome/utility/image_writer/image_writer_handler.cc (right): https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... chrome/utility/image_writer/image_writer_handler.cc:45: // http://crbug.com/352442 On 2017/02/08 12:12:59, noel gordon wrote: > On 2017/02/08 09:31:18, dcheng wrote: > > Nit: pointer to the bug is good, but a brief explanation of why this logic is > > necessary would be useful as well > > > > Also, http => https ;-) > > Done. I think the URL is the best pointer I can give to the details. Also a > brief explanation of the bug is given in the ChangeLog, and that saves me > repeating myself twice :) > > Comments are not code, and too often don't match the actual code, leading to > maintenance burden and other issues. Better to rephrase them into actual code > if possible. It's not always possible, so ensure the comment adds value. [1] > > [1] https://lists.webkit.org/pipermail/webkit-dev/2011-January/015769.html Hmm... I personally don't like bug URLs as the only explanation, because you often have to read through all the comments and try to reverse engineer what the original bug was from the comments/diffs. As best as I can tell, we just want to avoid creating a new ImageWriter if the image path and the target device are identical, so we can keep any existing file handles open. Given that the Mojo set up / ImageWriter setup looks largely shared, another option is to just factor it out into a helper method. Then the comment won't be duplicated =)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/09 01:25:36, dcheng wrote: > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > File chrome/utility/image_writer/image_writer_handler.cc (right): > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > chrome/utility/image_writer/image_writer_handler.cc:45: // > http://crbug.com/352442 > On 2017/02/08 12:12:59, noel gordon wrote: > > On 2017/02/08 09:31:18, dcheng wrote: > > > Nit: pointer to the bug is good, but a brief explanation of why this logic > is > > > necessary would be useful as well > > > > > > Also, http => https ;-) > > > > Done. I think the URL is the best pointer I can give to the details. Also a > > brief explanation of the bug is given in the ChangeLog, and that saves me > > repeating myself twice :) > > > > Comments are not code, and too often don't match the actual code, leading to > > maintenance burden and other issues. Better to rephrase them into actual code > > if possible. It's not always possible, so ensure the comment adds value. [1] > > > > [1] https://lists.webkit.org/pipermail/webkit-dev/2011-January/015769.html > > Hmm... I personally don't like bug URLs as the only explanation, because you > often have to read through all the comments and try to reverse engineer what the > original bug was from the comments/diffs. The bug URL is not the only explanation though. Recommend you try the blame layer in codesearch: it is fast, saves you having to read a bug since you'd like to avoid that, and it even comes with change description and left-right code diffs. > As best as I can tell, we just want to > avoid creating a new ImageWriter if the image path and the target device are > identical, so we can keep any existing file handles open. > > Given that the Mojo set up / ImageWriter setup looks largely shared, another > option is to just factor it out into a helper method. Then the comment won't be > duplicated =) Possible. A possible con would be blame would be assigned to me, and would also require you to click through more layers of blame to find the relevant change. That kinds thing really annoys me due to the waste of precious developer time.
On 2017/02/09 12:40:19, noel gordon wrote: > That kinds thing really annoys me due to the waste of precious developer time. Speaking of which, not actionable things left to do here, so ... +jochen polite ping.
lgtm
lgtm
The CQ bit was checked by noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, tibell@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2663603002/#ps220001 (title: "Use https: in bug references, minor comment fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1486652159268050,
"parent_rev": "743f8d6db5bc0efd51c3e7ce0fcec1a86886a364", "commit_rev":
"6b03ae83bd948f7389a359acfd3ef8055c142c45"}
Message was sent while issue was closed.
Description was changed from ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Enforce image_writer_utility_client state The original code had state, but never checked the state assumption: that a WRITE or VERIFY operation could be in progress - just never at the same time. Add ResetRequest() between operations to reset state. Add DCHECK at when any new operation is requested to DCHECK that the previous operation if any has been completed. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Define a new member utility_process_running_elevated_, and use it to prevent IPC from being processed if the utility process is elevated. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add WriteVerify test fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 ========== to ========== Convert utility process ImageWriter IPC to mojo Add extensions::mojo::RemovableStorageWriter Define a new mojo interface that is exposed by the utility process to the browser. Update the extensions private api image writer caller to use the new interface, rather than IPC, for image writing. Add mojo RemovableStorageWriterClient interface Define a mojo interface to send to utility process image writer, which it can use to send Write and Verify operation progress and completion status back to the caller. Use content::UtilityProcessMojoClient The code used content::UtilityProcessHost to control the utility process. Replace that with the mojo utility client. The mojo channel that connects the caller and the utility process can now be intentionally disconnected, from either end, to implement the Cancel operation. Add a browsertest for WriteCancel and VerifyCancel. Enforce image_writer_utility_client state The original code had state, but never checked the state assumption: that a WRITE or VERIFY operation could be in progress - just never at the same time. Add ResetRequest() between operations to reset state. Add DCHECK at when any new operation is requested to DCHECK that the previous operation if any has been completed. Remove the utility process IPC whitelist The image writer IPC (removed in this CL) were the last remaining entries in the utility process IPC whitelist (these IPC ran elevated on WIN). The whitelist is now unused. Remove all its code and associated files. Define a new member utility_process_running_elevated_, and use it to prevent IPC from being processed if the utility process is elevated. Add image_writer_utility_client_browsertest Again a utility process service with no test. Add a browsertest for Write and Verify operations (invalid, valid, and cancel). Note: the browsertest can't test the low-level USB driver code on any port since our bots don't have USB sticks. To test that aspect, I manually tested with the ChromeOS Recovery Tool [1] and was able to write and verify a ChromeOS image to my USB stick on both Windows and Mac OSX. ImageWriterHandler Add a test mode to allow the class to be browser-tested (with the caveats mentioned above). Add reference to bug 352442: file handles to the device and image files must remain open between Write and Verify operations. Add WriteVerify test fixture to specifically test that case. [1] http://bit.ly/2jFtsZZ BUG=680928 Review-Url: https://codereview.chromium.org/2663603002 Cr-Commit-Position: refs/heads/master@{#449296} Committed: https://chromium.googlesource.com/chromium/src/+/6b03ae83bd948f7389a359acfd3e... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/6b03ae83bd948f7389a359acfd3e...
Message was sent while issue was closed.
Patchset #12 (id:240001) has been deleted
Message was sent while issue was closed.
Patchset #12 (id:260001) has been deleted
Message was sent while issue was closed.
On 2017/02/09 12:40:19, noel gordon wrote: > On 2017/02/09 01:25:36, dcheng wrote: > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > File chrome/utility/image_writer/image_writer_handler.cc (right): > > > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > chrome/utility/image_writer/image_writer_handler.cc:45: // > > http://crbug.com/352442 > > On 2017/02/08 12:12:59, noel gordon wrote: > > > On 2017/02/08 09:31:18, dcheng wrote: > > > > Nit: pointer to the bug is good, but a brief explanation of why this logic > > is > > > > necessary would be useful as well > > > > > > > > Also, http => https ;-) > > > > > > Done. I think the URL is the best pointer I can give to the details. Also > a > > > brief explanation of the bug is given in the ChangeLog, and that saves me > > > repeating myself twice :) > > > > > > Comments are not code, and too often don't match the actual code, leading to > > > maintenance burden and other issues. Better to rephrase them into actual > code > > > if possible. It's not always possible, so ensure the comment adds value. > [1] > > > > > > [1] https://lists.webkit.org/pipermail/webkit-dev/2011-January/015769.html > > > > Hmm... I personally don't like bug URLs as the only explanation, because you > > often have to read through all the comments and try to reverse engineer what > the > > original bug was from the comments/diffs. > > The bug URL is not the only explanation though. Recommend you try the blame > layer in codesearch: it is fast, saves you having to read a bug since you'd like > to avoid that, and it even comes with change description and left-right code > diffs. 1) I'm not asking for a "what" comment, I'm asking for a "why" comment. The "why" is we need to keep file handles open if we're doing consecutive operations on the same image/target device, to avoid <weird failures>. This is not something I can easily deduce from the code. 2) This isn't WebKit. We've been trying to encourage more comments throughout the codebase. > > > As best as I can tell, we just want to > > avoid creating a new ImageWriter if the image path and the target device are > > identical, so we can keep any existing file handles open. > > > > Given that the Mojo set up / ImageWriter setup looks largely shared, another > > option is to just factor it out into a helper method. Then the comment won't > be > > duplicated =) > > Possible. A possible con would be blame would be assigned to me, and would also > require you to click through more layers of blame to find the relevant change. > That kinds thing really annoys me due to the waste of precious developer time. Skipping through one revision for blame isn't hard and happens all the time. This isn't a good reason not to refactor duplicated code. Ultimately, I do not think the comment meets the bar of readability for chromium. Please refactor out the common code and make it self-sufficient.
Message was sent while issue was closed.
On 2017/02/11 01:19:15, dcheng wrote: > On 2017/02/09 12:40:19, noel gordon wrote: > > On 2017/02/09 01:25:36, dcheng wrote: > > > > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > > File chrome/utility/image_writer/image_writer_handler.cc (right): > > > > > > > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > > chrome/utility/image_writer/image_writer_handler.cc:45: // > > > http://crbug.com/352442 > > > On 2017/02/08 12:12:59, noel gordon wrote: > > > > On 2017/02/08 09:31:18, dcheng wrote: > > > > > Nit: pointer to the bug is good, but a brief explanation of why this > logic > > > is > > > > > necessary would be useful as well > > > > > > > > > > Also, http => https ;-) > > > > > > > > Done. I think the URL is the best pointer I can give to the details. > Also > > a > > > > brief explanation of the bug is given in the ChangeLog, and that saves me > > > > repeating myself twice :) > > > > > > > > Comments are not code, and too often don't match the actual code, leading > to > > > > maintenance burden and other issues. Better to rephrase them into actual > > code > > > > if possible. It's not always possible, so ensure the comment adds value. > > [1] > > > > > > > > [1] https://lists.webkit.org/pipermail/webkit-dev/2011-January/015769.html > > > > > > Hmm... I personally don't like bug URLs as the only explanation, because you > > > often have to read through all the comments and try to reverse engineer what > > the > > > original bug was from the comments/diffs. > > > > The bug URL is not the only explanation though. Recommend you try the blame > > layer in codesearch: it is fast, saves you having to read a bug since you'd > like > > to avoid that, and it even comes with change description and left-right code > > diffs. > > 1) I'm not asking for a "what" comment, I'm asking for a "why" comment. The > "why" is we need to keep file handles open if we're doing consecutive operations > on the same image/target device, to avoid <weird failures>. This is not > something I can easily deduce from the code. > 2) This isn't WebKit. We've been trying to encourage more comments throughout > the codebase. > > > > > > As best as I can tell, we just want to > > > avoid creating a new ImageWriter if the image path and the target device are > > > identical, so we can keep any existing file handles open. > > > > > > Given that the Mojo set up / ImageWriter setup looks largely shared, another > > > option is to just factor it out into a helper method. Then the comment won't > > be > > > duplicated =) > > > > Possible. A possible con would be blame would be assigned to me, and would > also > > require you to click through more layers of blame to find the relevant change. > > > That kinds thing really annoys me due to the waste of precious developer time. > > Skipping through one revision for blame isn't hard and happens all the time. > This isn't a good reason not to refactor duplicated code. Ultimately, I do not > think the comment meets the bar of readability for chromium. Please refactor out > the common code and make it self-sufficient. (Pinging to make sure this doesn't get lost even though the CL is already committed)
Message was sent while issue was closed.
On 2017/02/20 03:04:32, dcheng wrote: > On 2017/02/11 01:19:15, dcheng wrote: > > On 2017/02/09 12:40:19, noel gordon wrote: > > > On 2017/02/09 01:25:36, dcheng wrote: > > > > > > > > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > > > File chrome/utility/image_writer/image_writer_handler.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > > > chrome/utility/image_writer/image_writer_handler.cc:45: // > > > > http://crbug.com/352442 > > > > On 2017/02/08 12:12:59, noel gordon wrote: > > > > > On 2017/02/08 09:31:18, dcheng wrote: > > > > > > Nit: pointer to the bug is good, but a brief explanation of why this > > logic > > > > is > > > > > > necessary would be useful as well > > > > > > > > > > > > Also, http => https ;-) > > > > > > > > > > Done. I think the URL is the best pointer I can give to the details. > > Also > > > a > > > > > brief explanation of the bug is given in the ChangeLog, and that saves > me > > > > > repeating myself twice :) > > > > > > > > > > Comments are not code, and too often don't match the actual code, > leading > > to > > > > > maintenance burden and other issues. Better to rephrase them into > actual > > > code > > > > > if possible. It's not always possible, so ensure the comment adds > value. > > > [1] > > > > > > > > > > [1] > https://lists.webkit.org/pipermail/webkit-dev/2011-January/015769.html > > > > > > > > Hmm... I personally don't like bug URLs as the only explanation, because > you > > > > often have to read through all the comments and try to reverse engineer > what > > > the > > > > original bug was from the comments/diffs. > > > > > > The bug URL is not the only explanation though. Recommend you try the blame > > > layer in codesearch: it is fast, saves you having to read a bug since you'd > > like > > > to avoid that, and it even comes with change description and left-right code > > > diffs. > > > > 1) I'm not asking for a "what" comment, I'm asking for a "why" comment. The > > "why" is we need to keep file handles open if we're doing consecutive > operations > > on the same image/target device, to avoid <weird failures>. This is not > > something I can easily deduce from the code. > > 2) This isn't WebKit. We've been trying to encourage more comments throughout > > the codebase. > > > > > > > > > As best as I can tell, we just want to > > > > avoid creating a new ImageWriter if the image path and the target device > are > > > > identical, so we can keep any existing file handles open. > > > > > > > > Given that the Mojo set up / ImageWriter setup looks largely shared, > another > > > > option is to just factor it out into a helper method. Then the comment > won't > > > be > > > > duplicated =) > > > > > > Possible. A possible con would be blame would be assigned to me, and would > > also > > > require you to click through more layers of blame to find the relevant > change. > > > > > That kinds thing really annoys me due to the waste of precious developer > time. > > > > Skipping through one revision for blame isn't hard and happens all the time. > > This isn't a good reason not to refactor duplicated code. Ultimately, I do not > > think the comment meets the bar of readability for chromium. Please refactor > out > > the common code and make it self-sufficient. > > (Pinging to make sure this doesn't get lost even though the CL is already > committed) Can you please address my comments in a followup CL? Thanks!
Message was sent while issue was closed.
On 2017/02/27 03:20:51, dcheng wrote: > On 2017/02/20 03:04:32, dcheng wrote: > > On 2017/02/11 01:19:15, dcheng wrote: > > > On 2017/02/09 12:40:19, noel gordon wrote: > > > > On 2017/02/09 01:25:36, dcheng wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > > > > File chrome/utility/image_writer/image_writer_handler.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2663603002/diff/200001/chrome/utility/image_w... > > > > > chrome/utility/image_writer/image_writer_handler.cc:45: // > > > > > http://crbug.com/352442 > > > > > On 2017/02/08 12:12:59, noel gordon wrote: > > > > > > On 2017/02/08 09:31:18, dcheng wrote: > > > > > > > Nit: pointer to the bug is good, but a brief explanation of why this > > > logic > > > > > is > > > > > > > necessary would be useful as well > > > > > > > > > > > > > > Also, http => https ;-) > > > > > > > > > > > > Done. I think the URL is the best pointer I can give to the details. > > > Also > > > > a > > > > > > brief explanation of the bug is given in the ChangeLog, and that saves > > me > > > > > > repeating myself twice :) > > > > > > > > > > > > Comments are not code, and too often don't match the actual code, > > leading > > > to > > > > > > maintenance burden and other issues. Better to rephrase them into > > actual > > > > code > > > > > > if possible. It's not always possible, so ensure the comment adds > > value. > > > > [1] > > > > > > > > > > > > [1] > > https://lists.webkit.org/pipermail/webkit-dev/2011-January/015769.html > > > > > > > > > > Hmm... I personally don't like bug URLs as the only explanation, because > > you > > > > > often have to read through all the comments and try to reverse engineer > > what > > > > the > > > > > original bug was from the comments/diffs. > > > > > > > > The bug URL is not the only explanation though. Recommend you try the > blame > > > > layer in codesearch: it is fast, saves you having to read a bug since > you'd > > > like > > > > to avoid that, and it even comes with change description and left-right > code > > > > diffs. > > > > > > 1) I'm not asking for a "what" comment, I'm asking for a "why" comment. The > > > "why" is we need to keep file handles open if we're doing consecutive > > operations > > > on the same image/target device, to avoid <weird failures>. This is not > > > something I can easily deduce from the code. > > > 2) This isn't WebKit. We've been trying to encourage more comments > throughout > > > the codebase. > > > > > > > > > > > > As best as I can tell, we just want to > > > > > avoid creating a new ImageWriter if the image path and the target device > > are > > > > > identical, so we can keep any existing file handles open. > > > > > > > > > > Given that the Mojo set up / ImageWriter setup looks largely shared, > > another > > > > > option is to just factor it out into a helper method. Then the comment > > won't > > > > be > > > > > duplicated =) > > > > > > > > Possible. A possible con would be blame would be assigned to me, and > would > > > also > > > > require you to click through more layers of blame to find the relevant > > change. > > > > > > > That kinds thing really annoys me due to the waste of precious developer > > time. > > > > > > Skipping through one revision for blame isn't hard and happens all the time. > > > This isn't a good reason not to refactor duplicated code. Ultimately, I do > not > > > think the comment meets the bar of readability for chromium. Please refactor > > out > > > the common code and make it self-sufficient. > > > > (Pinging to make sure this doesn't get lost even though the CL is already > > committed) > > Can you please address my comments in a followup CL? Thanks! Done. |
