Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(37)

Issue 2730583002: Convert utility process zip creator IPC to mojo: part 2 (Closed)

Created:
3 years, 9 months ago by Noel Gordon
Modified:
3 years, 9 months ago
CC:
chromium-reviews, yamaguchi+watch_chromium.org, oka+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, fukino+watch_chromium.org, davemoore+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert utility process zip creator IPC to mojo: part 2 Follow on from [1], the comment added there about |this| during the mojo callback is wrong. Checking this->hasOneRef() after we reset() the mojo client shows there's always one reference left in all cases (mojo error callback, mojo success callback). Ditch the comment. The reset and client callback can be done in any order. Invoke the client callback with a one-liner. Covered by existing tests: ZipFileCreatorTest.SomeFilesZip, per https://codereview.chromium.org/2703463005 [1] https://codereview.chromium.org/2705613003 TBR=jochen@chromium.org BUG=597124 Review-Url: https://codereview.chromium.org/2730583002 Cr-Commit-Position: refs/heads/master@{#454181} Committed: https://chromium.googlesource.com/chromium/src/+/e9cc49145670e9a034505aeda4f9ee7bb0a7b3c6

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -5 lines) Patch
M chrome/browser/chromeos/file_manager/zip_file_creator.cc View 1 chunk +1 line, -5 lines 0 comments Download

Messages

Total messages: 20 (13 generated)
Noel Gordon
PTAL.
3 years, 9 months ago (2017-03-02 03:49:23 UTC) #7
Sam McNally
lgtm
3 years, 9 months ago (2017-03-02 03:57:15 UTC) #8
tibell
lgtm
3 years, 9 months ago (2017-03-02 03:58:10 UTC) #9
dcheng
lgtm
3 years, 9 months ago (2017-03-02 04:22:08 UTC) #10
Noel Gordon
TBR=jochen I'm sure jochen will enjoy telling us that mojo is well designed for this ...
3 years, 9 months ago (2017-03-02 04:32:41 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2730583002/1
3 years, 9 months ago (2017-03-02 04:34:31 UTC) #17
commit-bot: I haz the power
3 years, 9 months ago (2017-03-02 04:43:11 UTC) #20
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/e9cc49145670e9a034505aeda4f9...

Powered by Google App Engine
This is Rietveld 408576698