Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2697463002: Convert utility process extension unpacker IPC to mojo

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by noel gordon
Modified:
1 day, 19 hours ago
CC:
chromium-reviews, extensions-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, darin (slow to review), catmullings
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert utility process extension unpacker IPC to mojo BUG=691410

Patch Set 1 #

Patch Set 2 : Support manifest_overlay.json in extensions unittests. #

Patch Set 3 : Add TestContentUtilityClient::ExposeInterfacesToBrowser. #

Patch Set 4 : Add extensions::unpacker::TakeParsedManifest(). #

Total comments: 10

Patch Set 5 : Use MakeUnique when creating the utility mojo client. #

Total comments: 30

Patch Set 6 : Review comments. #

Patch Set 7 : #include sequenced_worker_pool #

Patch Set 8 : Add it again (from a different machine). #

Total comments: 3

Patch Set 9 #

Patch Set 10 : Sync to ToT. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+512 lines, -457 lines) Patch
M chrome/browser/chrome_content_utility_manifest_overlay.json View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/zipfile_installer.h View 1 2 3 4 5 6 7 8 9 1 chunk +27 lines, -24 lines 0 comments Download
M chrome/browser/extensions/zipfile_installer.cc View 1 2 3 4 5 6 7 8 1 chunk +74 lines, -87 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/utility/extensions/extensions_handler.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -2 lines 0 comments Download
M content/public/browser/utility_process_mojo_client.h View 1 2 3 4 5 6 7 8 7 chunks +25 lines, -9 lines 0 comments Download
M extensions/BUILD.gn View 1 1 chunk +2 lines, -0 lines 0 comments Download
M extensions/DEPS View 1 1 chunk +1 line, -0 lines 0 comments Download
M extensions/browser/extensions_test.h View 1 2 chunks +2 lines, -0 lines 0 comments Download
M extensions/browser/extensions_test.cc View 1 2 chunks +4 lines, -1 line 0 comments Download
M extensions/browser/sandboxed_unpacker.h View 1 2 3 4 5 6 7 8 9 11 chunks +33 lines, -67 lines 0 comments Download
M extensions/browser/sandboxed_unpacker.cc View 1 2 3 4 5 6 7 8 9 13 chunks +85 lines, -119 lines 0 comments Download
M extensions/browser/sandboxed_unpacker_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M extensions/common/BUILD.gn View 1 chunk +6 lines, -0 lines 0 comments Download
M extensions/common/OWNERS View 1 chunk +2 lines, -0 lines 0 comments Download
A extensions/common/extension_unpacker.mojom View 1 chunk +29 lines, -0 lines 0 comments Download
M extensions/common/extension_utility_messages.h View 3 chunks +0 lines, -49 lines 0 comments Download
A extensions/common/extension_utility_types.h View 1 chunk +17 lines, -0 lines 0 comments Download
A extensions/test/data/extensions_test_content_utility_manifest_overlay.json View 1 1 chunk +12 lines, -0 lines 0 comments Download
A extensions/test/test_content_browser_client.h View 1 1 chunk +31 lines, -0 lines 0 comments Download
A extensions/test/test_content_browser_client.cc View 1 2 3 4 5 1 chunk +37 lines, -0 lines 0 comments Download
M extensions/test/test_content_utility_client.h View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M extensions/test/test_content_utility_client.cc View 1 2 1 chunk +7 lines, -5 lines 0 comments Download
M extensions/utility/unpacker.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M extensions/utility/unpacker.cc View 1 chunk +1 line, -0 lines 0 comments Download
M extensions/utility/utility_handler.h View 2 chunks +8 lines, -10 lines 0 comments Download
M extensions/utility/utility_handler.cc View 1 2 3 4 5 5 chunks +96 lines, -82 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 77 (69 generated)
noel gordon
PTAL. A DEPS change here to allow utilty_handler to use service_manager (so utility_handler can expose ...
1 week, 4 days ago (2017-02-14 02:53:45 UTC) #18
Sam McNally
https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensions/zipfile_installer.cc File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensions/zipfile_installer.cc#newcode60 chrome/browser/extensions/zipfile_installer.cc:60: if (!base::CreateTemporaryDirInDir(dir_temp, dir_name, &temp_dir_)) { This class would be ...
1 week, 4 days ago (2017-02-14 05:41:52 UTC) #25
noel gordon
https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensions/zipfile_installer.cc File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensions/zipfile_installer.cc#newcode60 chrome/browser/extensions/zipfile_installer.cc:60: if (!base::CreateTemporaryDirInDir(dir_temp, dir_name, &temp_dir_)) { On 2017/02/14 05:41:52, Sam ...
1 week, 4 days ago (2017-02-14 12:35:37 UTC) #28
Devlin
FYI, +catmullings@ is also looking into converting extension messages to mojo (she has an in-progress ...
1 week, 4 days ago (2017-02-14 17:24:59 UTC) #31
noel gordon
On 2017/02/14 17:24:59, Devlin wrote: > FYI, +catmullings@ is also looking into converting extension messages ...
1 week, 3 days ago (2017-02-15 13:35:13 UTC) #32
noel gordon
https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensions/zipfile_installer.cc File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensions/zipfile_installer.cc#newcode61 chrome/browser/extensions/zipfile_installer.cc:61: base::FilePath temp_dir; On 2017/02/14 17:24:59, Devlin wrote: > dir_temp ...
1 week, 3 days ago (2017-02-15 17:28:09 UTC) #34
noel gordon
On 2017/02/15 13:35:13, noel gordon wrote: > On 2017/02/14 17:24:59, Devlin wrote: > > FYI, ...
1 week, 2 days ago (2017-02-16 01:39:53 UTC) #38
Devlin
1 week, 1 day ago (2017-02-17 15:53:56 UTC) #59
https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/zipfile_installer.h (right):

https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/zipfile_installer.h:16: #include
"extensions/common/extension_unpacker.mojom.h"
On 2017/02/15 17:28:08, noel gordon wrote:
> On 2017/02/14 17:24:59, Devlin wrote:
> > Could this just be a forward declaration?
> 
> Possible, but not something we've conventionally done when converting utility
> process IPC to mojo ... since mojo tends to be very namespace nested.  Once we
> have compilers with nested namespace support, it'll be a lot easier to do
then.
> 
> Maybe we should change the mojo code generator to create a fwd declaration .h
or
> something, dunno.

Sorry, I don't follow - what's wrong with

namespace extensions {
namespace mojom {
class ExtensionUnpacker;
}

...

}  // namespace extensions
?

We have nested namespace forward declarations in a lot of places in the code...

https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi...
File chrome/utility/extensions/extensions_handler.cc (right):

https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi...
chrome/utility/extensions/extensions_handler.cc:198:
UtilityHandler::UtilityThreadStarted();
On 2017/02/15 17:28:08, noel gordon wrote:
> On 2017/02/14 17:24:59, Devlin wrote:
> > Why do we need this method, as opposed to calling
> > UtilityHandler::UtilityThreadStarted() directly?
> 
> ExtensionsHandler owns a UtilityHandler utility_handler_, so layering, we let
> ExtensionsHandler control it.  And similarly below in
> ExtensionsHandler::ExposeInterfacesToBrowser.

I don't quite follow - this is a static method, so ownership in member variables
doesn't really matter.  In ExposeInterfacesToBrowser(), the layering is
important, because it adds extra logic that UtilityHandler doesn't do.  Here, it
just seems to add an extra function call.

https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte...
File extensions/common/extension_unpacker.mojom (right):

https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte...
extensions/common/extension_unpacker.mojom:25: int32 location,
On 2017/02/15 17:28:09, noel gordon wrote:
> On 2017/02/14 17:24:59, Devlin wrote:
> > Mojo supports enums - can we do that instead?
> 
> Yeah I cargo-culted the existing API here, there were CHECK's in the existing
> IPC handler to test if location is sain.  I retained those.
> 
> "Can we do that instead?"  yeah we could, but all clients of SandboxedUnpacker
> would need to be changed to use the mojo enum defines (unless you want to
> maintain two enums).
> 
> My feeling is there are lots of clients, and updating them might be a patch in
> itself.  Maybe a job for crbug.com/692120 ?

My understanding from rockot@ is that mojo can reuse the existing
IPC_ENUM_TRAITS declarations, which would mean that we wouldn't need to update
callers.  Is that not the case?

https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/zipfile_installer.cc (right):

https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/zipfile_installer.cc:83:
content::UtilityProcessMojoClient<extensions::mojom::ExtensionUnpacker>>(
nit: don't need extensions:: prefix here

https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/zipfile_installer.cc:102:
ReportErrorOnUIThread(std::string(kExtensionHandlerFileUnzipError));
nit: I think it's okay to use the implicit ctor of std::string and just pass
kExtensionHandlerFileUnzipError directly here.

https://codereview.chromium.org/2697463002/diff/160001/extensions/browser/san...
File extensions/browser/sandboxed_unpacker.cc (right):

https://codereview.chromium.org/2697463002/diff/160001/extensions/browser/san...
extensions/browser/sandboxed_unpacker.cc:344: void
SandboxedUnpacker::StartUtilityProcess(const base::FilePath& dir) {
Given both instances of this call it with temp_dir_.GetPath(), can we remove the
param and do that in the body of the function?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd