arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG=643059
Committed: https://crrev.com/3c74f1cb2742e9b050fbc830f1ac0e06163c2370
Cr-Commit-Position: refs/heads/master@{#416194}

[hashimoto, 2016-09-01 07:34:20]

hashimoto@chromium.org changed reviewers:
+ hidehiko@chromium.org

[hashimoto, 2016-09-01 07:34:20]

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right):

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:38:
std::vector<std::string> CollectChangedPaths(
Semantically, base::FilePath suits better than std::string here, but
base::FilePath is not movable so it's bad for performance.
Which do you prefer?

[hashimoto, 2016-09-01 07:34:29]

The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 07:34:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 08:08:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 08:08:08]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-09-01 17:05:45]

Thank you for quick fix.

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right):

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:38:
std::vector<std::string> CollectChangedPaths(
On 2016/09/01 07:34:20, hashimoto wrote:
> Semantically, base::FilePath suits better than std::string here, but
> base::FilePath is not movable so it's bad for performance.
> Which do you prefer?

Is it measurable? If so, it's good to note.
If not, I prefer cleaner semantic code, so base:FilePath, personally, but not
strong opinion in this case and either is fine.

Alternatively, you could add no-except-move-ctor for chrome-wide
vector<base::FilePath> better performance.

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:151:
base::MakeUnique<mojo::Array<mojo::String>>(changed_paths.size());
Is it necessary to be unique_ptr? I thought mojo::Array supports std::move so it
should work with base::Passed()?

[hashimoto, 2016-09-02 03:56:01]

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right):

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:38:
std::vector<std::string> CollectChangedPaths(
On 2016/09/01 17:05:45, hidehiko wrote:
> On 2016/09/01 07:34:20, hashimoto wrote:
> > Semantically, base::FilePath suits better than std::string here, but
> > base::FilePath is not movable so it's bad for performance.
> > Which do you prefer?
> 
> Is it measurable? If so, it's good to note.
> If not, I prefer cleaner semantic code, so base:FilePath, personally, but not
> strong opinion in this case and either is fine.
> 
> Alternatively, you could add no-except-move-ctor for chrome-wide
> vector<base::FilePath> better performance.

I just thought it might be nice to do as there is a performance issue in this
class, but I didn't measure the performance. 
If your preference is base::FilePath, let's stick to it as the performance
difference shouldn't be huge.

https://codereview.chromium.org/2304523002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:151:
base::MakeUnique<mojo::Array<mojo::String>>(changed_paths.size());
On 2016/09/01 17:05:45, hidehiko wrote:
> Is it necessary to be unique_ptr? I thought mojo::Array supports std::move so
it
> should work with base::Passed()?

Thank you for pointing this out.
I wasn't aware of the fact that base::Passed can work with classes other than
scoped_ptr (which no longer exists).

[hashimoto, 2016-09-02 03:56:07]

The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 03:56:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 04:40:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 04:40:15]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-09-02 04:43:44]

lgtm

[hashimoto, 2016-09-02 05:26:38]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2016-09-02 05:27:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 05:30:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-02 05:33:13]

Description was changed from

==========
arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG=643059
==========

to

==========
arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

ArcBridgeService is not thread-safe.
WeakPtr should be defererenced only on the thread to which the
factory belongs.

BUG=643059

Committed: https://crrev.com/3c74f1cb2742e9b050fbc830f1ac0e06163c2370
Cr-Commit-Position: refs/heads/master@{#416194}
==========

[commit-bot: I haz the power, 2016-09-02 05:33:18]

Patchset 4 (id:??) landed as
https://crrev.com/3c74f1cb2742e9b050fbc830f1ac0e06163c2370
Cr-Commit-Position: refs/heads/master@{#416194}