Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
505393002:
Chromecast devtools API fixes. (Closed)
|
Created:
6 years, 3 months ago by gunsch Modified:
6 years, 3 months ago CC:
chromium-reviews, ozone-reviews_chromium.org, vsevik, yurys, paulirish+reviews_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionChromecast devtools API fixes.
R=lcwu@chromium.org
BUG=None
Committed: https://crrev.com/f06cac17d72816ea796d3e4d6ea74344c1fd9781
Cr-Commit-Position: refs/heads/master@{#292307}
Patch Set 1 #
Total comments: 2
Patch Set 2 : reduce change surface area (lcwu comments) #Patch Set 3 : restoring patch set 1 #Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/505393002/diff/1/chromecast/shell/browser/dev... File chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc (right): https://codereview.chromium.org/505393002/diff/1/chromecast/shell/browser/dev... chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:27: const char kTargetTypeOther[] = "other"; Why do we care about "service_worker" (or other) targets? I think we only need one type of target which is the web_contents target (although admittedly I don't know exactly what service_worker target is for). https://codereview.chromium.org/505393002/diff/1/chromecast/shell/browser/dev... chromecast/shell/browser/devtools/cast_dev_tools_delegate.cc:31: explicit Target(scoped_refptr<content::DevToolsAgentHost> agent_host); If we don't need other types of target, do we still need to change the ctor signature?
pfeldman@chromium.org changed reviewers: + dgozman@chromium.org, vkuzkokov@chromium.org
On 2014/08/27 07:31:22, pfeldman-OOO wrote: > mailto:pfeldman@chromium.org changed reviewers: > + mailto:dgozman@chromium.org, mailto:vkuzkokov@chromium.org PTAL: how about this instead? Fixes our build but with less aggressive changes.
vkuzkokov@chromium.org changed reviewers: + dominicc@chromium.org, horo@chromium.org
On 2014/08/27 16:55:04, gunsch wrote: > On 2014/08/27 07:31:22, pfeldman-OOO wrote: > > mailto:pfeldman@chromium.org changed reviewers: > > + mailto:dgozman@chromium.org, mailto:vkuzkokov@chromium.org > > PTAL: how about this instead? Fixes our build but with less aggressive changes. You'd better model your dev tools server by aw_dev_tools_server.cc: Target mostly delegates methods to DevToolsAgentHost. If you think that ChromeCast should only expose actual pages (though they will be an essential part of web platform), filter out agent hosts with type DevToolsAgentHost::TYPE_WEB_CONTENTS. Btw, do you have at least compile bots running? This should have been catch by infrastructure.
On 2014/08/27 17:09:56, vkuzkokov wrote: > mailto:vkuzkokov@chromium.org changed reviewers: > + mailto:dominicc@chromium.org, mailto:horo@chromium.org Bringing in service worker guys on it. Do you need ability to remotely debug service workers on Chromecast?
On 2014/08/27 17:12:18, vkuzkokov wrote: > On 2014/08/27 17:09:56, vkuzkokov wrote: > > mailto:vkuzkokov@chromium.org changed reviewers: > > + mailto:dominicc@chromium.org, mailto:horo@chromium.org > > Bringing in service worker guys on it. > Do you need ability to remotely debug service workers on Chromecast? I don't think we specifically disallow the Chromecast app developers from using shared worker or service worker (in our SDK/developer documentation), so app developers could potentially use these workers in their apps and it looks like we will need to add supports for target types of not only service worker but also shared worker. And in that case, we should probably do what chrome/browser/devtools/devtools_target_impl.cc is doing.
On 2014/08/27 18:41:44, lcwu1 wrote: > On 2014/08/27 17:12:18, vkuzkokov wrote: > > On 2014/08/27 17:09:56, vkuzkokov wrote: > > > mailto:vkuzkokov@chromium.org changed reviewers: > > > + mailto:dominicc@chromium.org, mailto:horo@chromium.org > > > > Bringing in service worker guys on it. > > Do you need ability to remotely debug service workers on Chromecast? > > I don't think we specifically disallow the Chromecast app developers from using > shared worker or service worker (in our SDK/developer documentation), so app > developers could potentially use these workers in their apps and it looks like > we will need to add supports for target types of not only service worker but > also shared worker. And in that case, we should probably do what > chrome/browser/devtools/devtools_target_impl.cc is doing. Since Chromecast doesn't introduce any specific targets like background pages for extensions (am I right), DevToolsTargetImpl is too heavy for this. You can just wrap all the DevToolsAgentHost instances into DevToolsTarget and delegate all the work to the latter. See android_webview/native/aw_dev_tools_server.cc for example.
On 2014/08/27 19:05:09, dgozman wrote: > On 2014/08/27 18:41:44, lcwu1 wrote: > > On 2014/08/27 17:12:18, vkuzkokov wrote: > > > On 2014/08/27 17:09:56, vkuzkokov wrote: > > > > mailto:vkuzkokov@chromium.org changed reviewers: > > > > + mailto:dominicc@chromium.org, mailto:horo@chromium.org > > > > > > Bringing in service worker guys on it. > > > Do you need ability to remotely debug service workers on Chromecast? > > > > I don't think we specifically disallow the Chromecast app developers from > using > > shared worker or service worker (in our SDK/developer documentation), so app > > developers could potentially use these workers in their apps and it looks like > > we will need to add supports for target types of not only service worker but > > also shared worker. And in that case, we should probably do what > > chrome/browser/devtools/devtools_target_impl.cc is doing. > > Since Chromecast doesn't introduce any specific targets like background pages > for extensions (am I right), DevToolsTargetImpl is too heavy for this. You can > just wrap all the DevToolsAgentHost instances into DevToolsTarget and delegate > all the work to the latter. See android_webview/native/aw_dev_tools_server.cc > for example. Chatted with lcwu@ offline. It seems reasonable to support what AwDevToolsServer/ShellDevToolsDelegate are also doing. Restored patch set 1, PTAL.
lgtm
The CQ bit was checked by gunsch@chromium.org
FYI to others on this thread: I am interested in more feedback if you have it, but am also interested in unbreaking our build, so submitting now for the time being. We do not yet have trybots but are expecting to in the not-too-distant future.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gunsch@chromium.org/505393002/40001
On 2014/08/27 23:23:19, gunsch wrote: > On 2014/08/27 19:05:09, dgozman wrote: > > On 2014/08/27 18:41:44, lcwu1 wrote: > > > On 2014/08/27 17:12:18, vkuzkokov wrote: > > > > On 2014/08/27 17:09:56, vkuzkokov wrote: > > > > > mailto:vkuzkokov@chromium.org changed reviewers: > > > > > + mailto:dominicc@chromium.org, mailto:horo@chromium.org > > > > > > > > Bringing in service worker guys on it. > > > > Do you need ability to remotely debug service workers on Chromecast? > > > > > > I don't think we specifically disallow the Chromecast app developers from > > using > > > shared worker or service worker (in our SDK/developer documentation), so app > > > developers could potentially use these workers in their apps and it looks > like > > > we will need to add supports for target types of not only service worker but > > > also shared worker. And in that case, we should probably do what > > > chrome/browser/devtools/devtools_target_impl.cc is doing. > > > > Since Chromecast doesn't introduce any specific targets like background pages > > for extensions (am I right), DevToolsTargetImpl is too heavy for this. You can > > just wrap all the DevToolsAgentHost instances into DevToolsTarget and delegate > > all the work to the latter. See android_webview/native/aw_dev_tools_server.cc > > for example. > > Chatted with lcwu@ offline. It seems reasonable to support what > AwDevToolsServer/ShellDevToolsDelegate are also doing. Restored patch set 1, > PTAL. I don't know whether ServiceWorker should be supported in Chromecast. vkuzkokov@ How do you think about adding GetTypeString() to DevToolsAgentHost or converting type to string in DevToolsHttpHandlerImpl::SerializeTarget()?
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 82647072ba96671cd2b0178ae23ac6b5d976f2bc
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f06cac17d72816ea796d3e4d6ea74344c1fd9781 Cr-Commit-Position: refs/heads/master@{#292307} |
