Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562393005:
[Chromecast] Reference internal services manifest. (Closed)
|
Created:
4 years ago by slan Modified:
4 years ago Reviewers:
alokp CC:
chromium-reviews, qsr+mojo_chromium.org, feature-media-reviews_chromium.org, lcwu+watch_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, halliwell+watch_chromium.org, alokp+watch_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Reference internal services manifest.
Apply an additional overlay to the content_browser manifest, which will allow
internal Chromecast builds to host embedded services and interfaces in the
"content_browser" service.
BUG=internal b/33584507
TEST=All services connect correctly
Committed: https://crrev.com/05eae196f8d7392e4555b8eb592941a95dc2fe03
Cr-Commit-Position: refs/heads/master@{#439591}
Patch Set 1 #Patch Set 2 : Remove ProvisionFetcher code. #Patch Set 3 : Remove dangling changes. #Patch Set 4 : Fix CastContentClient header #
Total comments: 5
Patch Set 5 : Remove ConnectionFilter logic #Patch Set 6 : No more ProvisionFetcher #Patch Set 7 : No content/ changes #
Total comments: 4
Messages
Total messages: 33 (23 generated)
slan@chromium.org changed reviewers: + alokp@chromium.org
Hey Alok, will you take the first pass at this before I broaden it to affected Chromium folks? Internal change here: https://eureka-internal-review.git.corp.google.com/#/c/61603/
The CQ bit was checked by slan@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Description was changed from ========== [Chromecast] Use ProvisionFetcher to provision device for CDM. Rather than pass a URLReequestContextGetter to the CDM factory, host a ProvisionFetcher in "content_browser", which the media service can use to request device certificates. This interface is exposed via a ConnectionFilter, which is added by the ContentClient. Also generate the Cast-specific manifest overlay for the "content_browser" service to better organize the interface specs. Add ample documentation so people know how to use this code. BUG=internal b/33584507 TEST=rm cert from device, Cast WV-protected content. ========== to ========== [Chromecast] Allow ContentClients to add ConnectionFilters. This will allow content embedders to host additional interfaces within "content_browser". This is particularly useful for Chromecast, which needs to host Cast-specific interfaces, some of which only exist in internal code. It is also used on ChromeOS to host a large suite of interfaces. Also apply an additional overlay to the content_browser manifest, which will allow internal Chromecast builds to host embedded services and interfaces in the "content_browser" service. BUG=internal b/33584507 TEST=All services connect correctly ==========
OK, this change has been reduced in scope to lay the groundwork for the ProvisionFetcher change. PTAL.
The CQ bit was checked by slan@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by slan@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
This looks pretty good although I would suggest postponing connection filter changes or at least making them in another patch. https://codereview.chromium.org/2562393005/diff/60001/chromecast/browser/cast... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/2562393005/diff/60001/chromecast/browser/cast... chromecast/browser/cast_content_browser_client.cc:123: class ContentBrowserConnectionFilter : public content::ConnectionFilter { Are we using connection filter anywhere? If not I would suggest making these changes only when we need them. https://codereview.chromium.org/2562393005/diff/60001/content/public/app/BUIL... File content/public/app/BUILD.gn (left): https://codereview.chromium.org/2562393005/diff/60001/content/public/app/BUIL... content/public/app/BUILD.gn:180: "media", should we make it conditional upon mojo_media_host == "browser" instead of removing them?
Description was changed from ========== [Chromecast] Allow ContentClients to add ConnectionFilters. This will allow content embedders to host additional interfaces within "content_browser". This is particularly useful for Chromecast, which needs to host Cast-specific interfaces, some of which only exist in internal code. It is also used on ChromeOS to host a large suite of interfaces. Also apply an additional overlay to the content_browser manifest, which will allow internal Chromecast builds to host embedded services and interfaces in the "content_browser" service. BUG=internal b/33584507 TEST=All services connect correctly ========== to ========== [Chromecast] Reference internal services manifest. Apply an additional overlay to the content_browser manifest, which will allow internal Chromecast builds to host embedded services and interfaces in the "content_browser" service. BUG=internal b/33584507 TEST=All services connect correctly ==========
PTAL https://codereview.chromium.org/2562393005/diff/60001/chromecast/browser/cast... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/2562393005/diff/60001/chromecast/browser/cast... chromecast/browser/cast_content_browser_client.cc:123: class ContentBrowserConnectionFilter : public content::ConnectionFilter { On 2016/12/14 05:46:16, alokp wrote: > Are we using connection filter anywhere? If not I would suggest making these > changes only when we need them. Removed. https://codereview.chromium.org/2562393005/diff/60001/content/public/app/BUIL... File content/public/app/BUILD.gn (left): https://codereview.chromium.org/2562393005/diff/60001/content/public/app/BUIL... content/public/app/BUILD.gn:180: "media", On 2016/12/14 05:46:16, alokp wrote: > should we make it conditional upon mojo_media_host == "browser" instead of > removing them? Sure, that makes sense.
PTAL. https://codereview.chromium.org/2562393005/diff/60001/content/public/app/BUIL... File content/public/app/BUILD.gn (left): https://codereview.chromium.org/2562393005/diff/60001/content/public/app/BUIL... content/public/app/BUILD.gn:180: "media", On 2016/12/19 17:58:18, slan wrote: > On 2016/12/14 05:46:16, alokp wrote: > > should we make it conditional upon mojo_media_host == "browser" instead of > > removing them? > > Sure, that makes sense. OK, I decided to keep this as-is for now. This doesn't affect us negatively, and we will need to re-wire most of this when "media" becomes a standalone service. Makes more sense to do it then.
The CQ bit was checked by slan@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.
lgtm https://codereview.chromium.org/2562393005/diff/120001/chromecast/browser/BUI... File chromecast/browser/BUILD.gn (right): https://codereview.chromium.org/2562393005/diff/120001/chromecast/browser/BUI... chromecast/browser/BUILD.gn:179: packaged_services = [ "media" ] since you are not changing content manifest, this is not necessary? https://codereview.chromium.org/2562393005/diff/120001/chromecast/browser/BUI... chromecast/browser/BUILD.gn:188: deps += [ "//chromecast/internal/shell/browser:cast_content_browser_internal_manifest_overlay" ] Not related to this patch, but I find it weird that service_manifest_overlay requires specifying both overlay file and deps. Would it be possible to infer the overlay file from deps?
https://codereview.chromium.org/2562393005/diff/120001/chromecast/browser/BUI... File chromecast/browser/BUILD.gn (right): https://codereview.chromium.org/2562393005/diff/120001/chromecast/browser/BUI... chromecast/browser/BUILD.gn:179: packaged_services = [ "media" ] On 2016/12/19 21:45:00, alokp wrote: > since you are not changing content manifest, this is not necessary? It's not necessary, but I would like to keep it because it serves as an example if somebody wants to add another service. It doesn't result in wrong behavior if we leave it in here. https://codereview.chromium.org/2562393005/diff/120001/chromecast/browser/BUI... chromecast/browser/BUILD.gn:188: deps += [ "//chromecast/internal/shell/browser:cast_content_browser_internal_manifest_overlay" ] On 2016/12/19 21:45:00, alokp wrote: > Not related to this patch, but I find it weird that service_manifest_overlay > requires specifying both overlay file and deps. Would it be possible to infer > the overlay file from deps? Unfortunately, no. In GN, there is not a really good way to query the values of variables within a target scope, so there isn't a way to distinguish between an overlay dependency and a manifest dependency.
The CQ bit was checked by slan@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": 120001, "attempt_start_ts": 1482185659832020,
"parent_rev": "4a9d5a000ece1d9c437f37fa381ed9f61443d665", "commit_rev":
"bbd6b319be41c428a46d6b622fd440904d002333"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Reference internal services manifest. Apply an additional overlay to the content_browser manifest, which will allow internal Chromecast builds to host embedded services and interfaces in the "content_browser" service. BUG=internal b/33584507 TEST=All services connect correctly ========== to ========== [Chromecast] Reference internal services manifest. Apply an additional overlay to the content_browser manifest, which will allow internal Chromecast builds to host embedded services and interfaces in the "content_browser" service. BUG=internal b/33584507 TEST=All services connect correctly Review-Url: https://codereview.chromium.org/2562393005 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Reference internal services manifest. Apply an additional overlay to the content_browser manifest, which will allow internal Chromecast builds to host embedded services and interfaces in the "content_browser" service. BUG=internal b/33584507 TEST=All services connect correctly Review-Url: https://codereview.chromium.org/2562393005 ========== to ========== [Chromecast] Reference internal services manifest. Apply an additional overlay to the content_browser manifest, which will allow internal Chromecast builds to host embedded services and interfaces in the "content_browser" service. BUG=internal b/33584507 TEST=All services connect correctly Committed: https://crrev.com/05eae196f8d7392e4555b8eb592941a95dc2fe03 Cr-Commit-Position: refs/heads/master@{#439591} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/05eae196f8d7392e4555b8eb592941a95dc2fe03 Cr-Commit-Position: refs/heads/master@{#439591} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
