Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(258)

Issue 2699663003: Convert utility process extension ParseUpdate IPC to mojo (Closed)

Created:
1 year ago by Noel Gordon
Modified:
10 months, 3 weeks ago
CC:
Aaron Boodman, abarth-chromium, catmullings, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, extensions-reviews_chromium.org, jam, qsr+mojo_chromium.org, Devlin, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert utility process extension ParseUpdate IPC to mojo Add ManifestParser mojo, used to parse an extensions update XML manifest document, and expose it to the browser via the utility process policy file. Change the caller to use a utility process mojo client. Add mojo structure traits to handle the API return values UpdateManifest::{Results,Result}. Remove the IPC message file: no longer needed or used. Covered by tests: ExtensionManagementTest.ExternalUrlUpdate and ExtensionManagementTest.AutoUpdate and others. Refer to [1] for a complete list. [1] https://codereview.chromium.org/2761763002 BUG=691410 Review-Url: https://codereview.chromium.org/2699663003 Cr-Commit-Position: refs/heads/master@{#460280} Committed: https://chromium.googlesource.com/chromium/src/+/825202ab2ec8a1d8eba2a08f481773e07641dda3

Patch Set 1 #

Patch Set 2 : Mojo Traits: Add an out-of-line UpdateManifest::Results copy constructor. #

Patch Set 3 : Rename updater to parser. #

Patch Set 4 : Workaround bot patch apply issue. #ifdef 0 the code to be git rm-ed. #

Patch Set 5 #

Patch Set 6 : Sync to ToT, see if git apply issue and try jobs are happy. #

Patch Set 7 : Sync up with dependent patch set. #

Patch Set 8 : Dependent patch http://crrev.com/2697463002 landed: sync up. #

Total comments: 8

Patch Set 9 : Add Results operator=, move static helpers into a namespace. #

Total comments: 2

Patch Set 10 : Add comment about Results* being null on failure. #

Patch Set 11 : Another change landed in this area, sync up to ToT. #

Total comments: 8

Patch Set 12 : Extensions review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+294 lines, -258 lines) Patch
M chrome/browser/chrome_content_utility_manifest_overlay.json View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/utility/chrome_content_utility_client.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/utility/extensions/extensions_handler.h View 1 2 3 4 5 6 7 4 chunks +2 lines, -5 lines 0 comments Download
M chrome/utility/extensions/extensions_handler.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +2 lines, -10 lines 0 comments Download
M extensions/browser/updater/extension_downloader.cc View 1 1 chunk +5 lines, -7 lines 0 comments Download
M extensions/browser/updater/safe_manifest_parser.h View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -37 lines 0 comments Download
M extensions/browser/updater/safe_manifest_parser.cc View 1 2 3 4 5 6 7 1 chunk +28 lines, -55 lines 0 comments Download
M extensions/common/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M extensions/common/extension_message_generator.h View 1 chunk +0 lines, -1 line 0 comments Download
D extensions/common/extension_utility_messages.h View 1 4 5 6 7 1 chunk +0 lines, -48 lines 0 comments Download
A extensions/common/manifest_parser.mojom View 1 2 1 chunk +36 lines, -0 lines 0 comments Download
A extensions/common/manifest_parser.typemap View 1 2 1 chunk +18 lines, -0 lines 0 comments Download
A extensions/common/manifest_parser_struct_traits.h View 1 2 1 chunk +81 lines, -0 lines 0 comments Download
A extensions/common/manifest_parser_struct_traits.cc View 1 2 1 chunk +47 lines, -0 lines 0 comments Download
M extensions/common/typemaps.gni View 1 2 3 4 5 6 1 chunk +4 lines, -1 line 0 comments Download
M extensions/common/update_manifest.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -0 lines 0 comments Download
M extensions/common/update_manifest.cc View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -8 lines 0 comments Download
M extensions/shell/utility/shell_content_utility_client.h View 1 chunk +0 lines, -4 lines 0 comments Download
M extensions/shell/utility/shell_content_utility_client.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -9 lines 0 comments Download
M extensions/test/data/extensions_test_content_utility_manifest_overlay.json View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M extensions/test/test_content_utility_client.h View 1 1 chunk +0 lines, -4 lines 0 comments Download
M extensions/test/test_content_utility_client.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -6 lines 0 comments Download
M extensions/utility/utility_handler.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -25 lines 0 comments Download
M extensions/utility/utility_handler.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +31 lines, -35 lines 0 comments Download

Messages

Total messages: 136 (113 generated)
Noel Gordon
On 2017/02/16 01:53:40, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...
1 year ago (2017-02-16 03:51:40 UTC) #7
Noel Gordon
+ sammc, tibell for mojo, +martin for IPC security
11 months ago (2017-03-20 02:46:36 UTC) #80
Sam McNally
https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h File extensions/common/update_manifest.h (right): https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h#newcode67 extensions/common/update_manifest.h:67: Results(const Results& other); Why? https://codereview.chromium.org/2699663003/diff/240001/extensions/utility/utility_handler.h File extensions/utility/utility_handler.h (right): https://codereview.chromium.org/2699663003/diff/240001/extensions/utility/utility_handler.h#newcode16 ...
11 months ago (2017-03-20 04:22:06 UTC) #81
Noel Gordon
https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h File extensions/common/update_manifest.h (right): https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h#newcode67 extensions/common/update_manifest.h:67: Results(const Results& other); On 2017/03/20 04:22:06, Sam McNally wrote: ...
11 months ago (2017-03-20 05:53:57 UTC) #82
Sam McNally
https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h File extensions/common/update_manifest.h (right): https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h#newcode67 extensions/common/update_manifest.h:67: Results(const Results& other); On 2017/03/20 05:53:57, noel gordon wrote: ...
11 months ago (2017-03-20 05:56:26 UTC) #83
Noel Gordon
https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h File extensions/common/update_manifest.h (right): https://codereview.chromium.org/2699663003/diff/240001/extensions/common/update_manifest.h#newcode67 extensions/common/update_manifest.h:67: Results(const Results& other); On 2017/03/20 05:56:26, Sam McNally wrote: ...
11 months ago (2017-03-20 11:32:53 UTC) #84
tibell
lgtm https://codereview.chromium.org/2699663003/diff/260001/extensions/browser/updater/safe_manifest_parser.h File extensions/browser/updater/safe_manifest_parser.h (right): https://codereview.chromium.org/2699663003/diff/260001/extensions/browser/updater/safe_manifest_parser.h#newcode16 extensions/browser/updater/safe_manifest_parser.h:16: // |callback| with the results. Runs on the ...
11 months ago (2017-03-20 22:55:36 UTC) #90
Noel Gordon
https://codereview.chromium.org/2699663003/diff/260001/extensions/browser/updater/safe_manifest_parser.h File extensions/browser/updater/safe_manifest_parser.h (right): https://codereview.chromium.org/2699663003/diff/260001/extensions/browser/updater/safe_manifest_parser.h#newcode16 extensions/browser/updater/safe_manifest_parser.h:16: // |callback| with the results. Runs on the UI ...
11 months ago (2017-03-20 23:21:08 UTC) #92
Martin Barbella
security lgtm
11 months ago (2017-03-20 23:51:01 UTC) #94
Sam McNally
lgtm
11 months ago (2017-03-20 23:54:57 UTC) #95
Noel Gordon
+devlin for extensions.
11 months ago (2017-03-21 01:57:29 UTC) #99
Devlin
lgtm https://codereview.chromium.org/2699663003/diff/300001/extensions/browser/updater/safe_manifest_parser.cc File extensions/browser/updater/safe_manifest_parser.cc (right): https://codereview.chromium.org/2699663003/diff/300001/extensions/browser/updater/safe_manifest_parser.cc#newcode22 extensions/browser/updater/safe_manifest_parser.cc:22: using ResultCallback = base::Callback<void(const UpdateManifest::Results*)>; Can we move ...
11 months ago (2017-03-21 23:51:17 UTC) #104
Noel Gordon
https://codereview.chromium.org/2699663003/diff/300001/extensions/browser/updater/safe_manifest_parser.cc File extensions/browser/updater/safe_manifest_parser.cc (right): https://codereview.chromium.org/2699663003/diff/300001/extensions/browser/updater/safe_manifest_parser.cc#newcode22 extensions/browser/updater/safe_manifest_parser.cc:22: using ResultCallback = base::Callback<void(const UpdateManifest::Results*)>; On 2017/03/21 23:51:16, Devlin ...
11 months ago (2017-03-22 01:34:51 UTC) #107
Noel Gordon
+ jochen for OWNERS What's test cover this change, a shed-load it seems [1]. [1] ...
11 months ago (2017-03-22 04:04:57 UTC) #112
Noel Gordon
On 2017/03/22 04:04:57, noel gordon wrote: > + jochen for OWNERS > > What's test ...
11 months ago (2017-03-22 04:28:43 UTC) #113
Noel Gordon
11 months ago (2017-03-23 15:43:42 UTC) #119
jochen (gone - plz use gerrit)
lgtm
10 months, 3 weeks ago (2017-03-27 12:36:10 UTC) #120
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/2699663003/320001
10 months, 3 weeks ago (2017-03-29 00:01:05 UTC) #124
commit-bot: I haz the power
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_android_rel_ng/builds/259811)
10 months, 3 weeks ago (2017-03-29 01:43:10 UTC) #126
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/2699663003/320001
10 months, 3 weeks ago (2017-03-29 01:48:25 UTC) #128
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder ...
10 months, 3 weeks ago (2017-03-29 02:05:24 UTC) #131
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/2699663003/320001
10 months, 3 weeks ago (2017-03-29 02:57:09 UTC) #133
commit-bot: I haz the power
10 months, 3 weeks ago (2017-03-29 04:38:10 UTC) #136
Message was sent while issue was closed.
Committed patchset #12 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/825202ab2ec8a1d8eba2a08f4817...

Powered by Google App Engine
This is Rietveld 408576698