Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1621753002:
First rough draft of skiaserve (Closed)
|
Created:
4 years, 11 months ago by joshua.litt Modified:
4 years, 11 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFirst rough draft of skiaserve
Please excuse the mess while we iterate
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1621753002
Committed: https://skia.googlesource.com/skia/+/7f6a1e078641c1acd135da6ad04b9be53b29cf0d
Patch Set 1 #
Total comments: 5
Patch Set 2 : tweaks #
Messages
Total messages: 38 (6 generated)
Description was changed from ========== First rough draft of skiaserve Please excuse the mess while we iterate BUG=skia: ========== to ========== First rough draft of skiaserve Please excuse the mess while we iterate BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
joshualitt@google.com changed reviewers: + ethannicholas@google.com, jcgregorio@google.com, joshualitt@google.com
ptal, sorry there are almost as many todos as lines of code, but I want to get this landed so we can iterate
LGTM modulo the one question. https://codereview.chromium.org/1621753002/diff/1/gyp/most.gyp File gyp/most.gyp (right): https://codereview.chromium.org/1621753002/diff/1/gyp/most.gyp#newcode82 gyp/most.gyp:82: [ 'skia_build_server', { Won't this break "most" for anyone who doesn't have libmicrohttpd installed?
https://codereview.chromium.org/1621753002/diff/1/gyp/most.gyp File gyp/most.gyp (right): https://codereview.chromium.org/1621753002/diff/1/gyp/most.gyp#newcode82 gyp/most.gyp:82: [ 'skia_build_server', { On 2016/01/22 16:50:21, jcgregorio wrote: > Won't this break "most" for anyone who doesn't have libmicrohttpd installed? I thought this would only build if skia_build_server == 1? Since it defaults to 0 in common_variables I assumed it would not build.
https://codereview.chromium.org/1621753002/diff/1/gyp/most.gyp File gyp/most.gyp (right): https://codereview.chromium.org/1621753002/diff/1/gyp/most.gyp#newcode82 gyp/most.gyp:82: [ 'skia_build_server', { On 2016/01/22 at 16:51:19, joshualitt wrote: > On 2016/01/22 16:50:21, jcgregorio wrote: > > Won't this break "most" for anyone who doesn't have libmicrohttpd installed? > > I thought this would only build if skia_build_server == 1? Since it defaults to 0 in common_variables I assumed it would not build. I know nothing of gyp, so LGTM :-)
https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp File tools/skiaserve/skiaserve.cpp (right): https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) { Is the fact that the HTML is hard-coded here just temporary?
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp File tools/skiaserve/skiaserve.cpp (right): https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) { On 2016/01/22 17:09:51, ethannicholas wrote: > Is the fact that the HTML is hard-coded here just temporary? (If not, you don't really need a server.)
On 2016/01/22 17:11:57, mtklein wrote: > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > File tools/skiaserve/skiaserve.cpp (right): > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) { > On 2016/01/22 17:09:51, ethannicholas wrote: > > Is the fact that the HTML is hard-coded here just temporary? > > (If not, you don't really need a server.) I meant "HTML embedded in the C++" as opposed to "stored in a template file".
On 2016/01/22 17:11:57, mtklein wrote: > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > File tools/skiaserve/skiaserve.cpp (right): > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) { > On 2016/01/22 17:09:51, ethannicholas wrote: > > Is the fact that the HTML is hard-coded here just temporary? > > (If not, you don't really need a server.) Mike - how would we serve up the template without some kind of server, especially if this is running locally? The template is there to pull styles and js, from either the web or locally, but we still need someway to serve this up to a browser.
On 2016/01/22 17:18:20, joshualitt wrote: > On 2016/01/22 17:11:57, mtklein wrote: > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > > File tools/skiaserve/skiaserve.cpp (right): > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) { > > On 2016/01/22 17:09:51, ethannicholas wrote: > > > Is the fact that the HTML is hard-coded here just temporary? > > > > (If not, you don't really need a server.) > > Mike - how would we serve up the template without some kind of server, > especially if this is running locally? The template is there to pull styles and > js, from either the web or locally, but we still need someway to serve this up > to a browser. file://
On 2016/01/22 17:17:37, ethannicholas wrote: > On 2016/01/22 17:11:57, mtklein wrote: > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > > File tools/skiaserve/skiaserve.cpp (right): > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) { > > On 2016/01/22 17:09:51, ethannicholas wrote: > > > Is the fact that the HTML is hard-coded here just temporary? > > > > (If not, you don't really need a server.) > > I meant "HTML embedded in the C++" as opposed to "stored in a template file". Correct, I will add another TODO to move the template to a file.
On 2016/01/22 17:18:54, mtklein wrote: > On 2016/01/22 17:18:20, joshualitt wrote: > > On 2016/01/22 17:11:57, mtklein wrote: > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > > > File tools/skiaserve/skiaserve.cpp (right): > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > > > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) > { > > > On 2016/01/22 17:09:51, ethannicholas wrote: > > > > Is the fact that the HTML is hard-coded here just temporary? > > > > > > (If not, you don't really need a server.) > > > > Mike - how would we serve up the template without some kind of server, > > especially if this is running locally? The template is there to pull styles > and > > js, from either the web or locally, but we still need someway to serve this up > > to a browser. > > file:// The intent is for this to work both local and remote.
On 2016/01/22 17:18:54, mtklein wrote: > On 2016/01/22 17:18:20, joshualitt wrote: > > On 2016/01/22 17:11:57, mtklein wrote: > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > > > File tools/skiaserve/skiaserve.cpp (right): > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > > > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString source) > { > > > On 2016/01/22 17:09:51, ethannicholas wrote: > > > > Is the fact that the HTML is hard-coded here just temporary? > > > > > > (If not, you don't really need a server.) > > > > Mike - how would we serve up the template without some kind of server, > > especially if this is running locally? The template is there to pull styles > and > > js, from either the web or locally, but we still need someway to serve this up > > to a browser. > > file:// not sure I follow, how would we render skps and such using static html?
On 2016/01/22 17:19:56, joshualitt wrote: > On 2016/01/22 17:18:54, mtklein wrote: > > On 2016/01/22 17:18:20, joshualitt wrote: > > > On 2016/01/22 17:11:57, mtklein wrote: > > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > > > > File tools/skiaserve/skiaserve.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > > > > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString > source) > > { > > > > On 2016/01/22 17:09:51, ethannicholas wrote: > > > > > Is the fact that the HTML is hard-coded here just temporary? > > > > > > > > (If not, you don't really need a server.) > > > > > > Mike - how would we serve up the template without some kind of server, > > > especially if this is running locally? The template is there to pull styles > > and > > > js, from either the web or locally, but we still need someway to serve this > up > > > to a browser. > > > > file:// > > not sure I follow, how would we render skps and such using static html? e.g. out/Release/debug -i some.skp -o some.html && google-chrome some.html
On 2016/01/22 17:19:56, joshualitt wrote: > On 2016/01/22 17:18:54, mtklein wrote: > > On 2016/01/22 17:18:20, joshualitt wrote: > > > On 2016/01/22 17:11:57, mtklein wrote: > > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > > > > File tools/skiaserve/skiaserve.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > > > > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString > source) > > { > > > > On 2016/01/22 17:09:51, ethannicholas wrote: > > > > > Is the fact that the HTML is hard-coded here just temporary? > > > > > > > > (If not, you don't really need a server.) > > > > > > Mike - how would we serve up the template without some kind of server, > > > especially if this is running locally? The template is there to pull styles > > and > > > js, from either the web or locally, but we still need someway to serve this > up > > > to a browser. > > > > file:// > > not sure I follow, how would we render skps and such using static html? Oh, I should clarify because its not at all clear from this code. The template serves up a page which then hits skiaserve to generate an skp and render it as a png which it inlines.
On 2016/01/22 at 17:21:32, mtklein wrote: > On 2016/01/22 17:19:56, joshualitt wrote: > > On 2016/01/22 17:18:54, mtklein wrote: > > > On 2016/01/22 17:18:20, joshualitt wrote: > > > > On 2016/01/22 17:11:57, mtklein wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.cpp > > > > > File tools/skiaserve/skiaserve.cpp (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1621753002/diff/1/tools/skiaserve/skiaserve.c... > > > > > tools/skiaserve/skiaserve.cpp:99: SkString generateTemplate(SkString > > source) > > > { > > > > > On 2016/01/22 17:09:51, ethannicholas wrote: > > > > > > Is the fact that the HTML is hard-coded here just temporary? > > > > > > > > > > (If not, you don't really need a server.) > > > > > > > > Mike - how would we serve up the template without some kind of server, > > > > especially if this is running locally? The template is there to pull styles > > > and > > > > js, from either the web or locally, but we still need someway to serve this > > up > > > > to a browser. > > > > > > file:// > > > > not sure I follow, how would we render skps and such using static html? > > e.g. > > out/Release/debug -i some.skp -o some.html && google-chrome some.html That won't work, file:// is not equivalent to http://localhost.
> That won't work, file:// is not equivalent to http://localhost. I'm not sure I follow. Where's http://localhost come into play?
On 2016/01/22 17:25:43, mtklein wrote: > > That won't work, file:// is not equivalent to http://localhost. > > I'm not sure I follow. Where's http://localhost come into play? The eventual intent of this service is to create a server which functions as an interactive debugger, allowing one to render SKPs, examine the contents of the draws, modify and re-render them, and so forth. Since we need this service to be both interactive and accessible from remote machines, we need http:// and a real server rather than file://.
On 2016/01/22 at 17:25:43, mtklein wrote: > > That won't work, file:// is not equivalent to http://localhost. > > I'm not sure I follow. Where's http://localhost come into play? See the design doc: https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md
On 2016/01/22 17:29:28, jcgregorio wrote: > On 2016/01/22 at 17:25:43, mtklein wrote: > > > That won't work, file:// is not equivalent to http://localhost. > > > > I'm not sure I follow. Where's http://localhost come into play? > > See the design doc: > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md You guys are sure you've got people who need to use a debugger remotely? That seems like a feature no one will ever use.
On 2016/01/22 at 17:37:48, mtklein wrote: > On 2016/01/22 17:29:28, jcgregorio wrote: > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > See the design doc: > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > You guys are sure you've got people who need to use a debugger remotely? That seems like a feature no one will ever use. The bikeshed is blue. Let's commit.
On 2016/01/22 17:41:54, jcgregorio wrote: > On 2016/01/22 at 17:37:48, mtklein wrote: > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > See the design doc: > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > You guys are sure you've got people who need to use a debugger remotely? That > seems like a feature no one will ever use. > > The bikeshed is blue. Let's commit. This isn't really a bikeshed issue. You guys have greatly complicated your design to accommodate this one feature. Seems like you might want to make sure that feature matters.
On 2016/01/22 at 17:43:22, mtklein wrote: > On 2016/01/22 17:41:54, jcgregorio wrote: > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > > > See the design doc: > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > You guys are sure you've got people who need to use a debugger remotely? That > > seems like a feature no one will ever use. > > > > The bikeshed is blue. Let's commit. > > This isn't really a bikeshed issue. You guys have greatly complicated your design to accommodate this one feature. Seems like you might want to make sure that feature matters. No, this is a very simple design, it keeps the C++ code as simple as possible, just processing SKPs and serving up JSON/images over http to localhost. The rest of the UI is hosted on debugger.skia.org where we can use all the infra tools to monitor/manage/push updates. The only way for a browser to talk to a running app is over HTTP, which file:// is not, so it wouldn't work to just server the html file off of a file:// url. The ability of doing remote debugging is just a nice side effect of having a properly partitioned application. Even if we didn't want to do remove debugging, this would be the same design.
On 2016/01/22 at 17:48:36, jcgregorio wrote: > On 2016/01/22 at 17:43:22, mtklein wrote: > > On 2016/01/22 17:41:54, jcgregorio wrote: > > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > > > > > See the design doc: > > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > > > You guys are sure you've got people who need to use a debugger remotely? That > > > seems like a feature no one will ever use. > > > > > > The bikeshed is blue. Let's commit. > > > > This isn't really a bikeshed issue. You guys have greatly complicated your design to accommodate this one feature. Seems like you might want to make sure that feature matters. > > No, this is a very simple design, it keeps the C++ code as simple as possible, just processing SKPs and serving up JSON/images over http to localhost. The rest of the UI is hosted on debugger.skia.org where we can use all the infra tools to monitor/manage/push updates. The only way for a browser to talk to a running app is over HTTP, which file:// is not, so it wouldn't work to just server the html file off of a file:// url. > > The ability of doing remote debugging is just a nice side effect of having a properly partitioned application. Even if we didn't want to do remove debugging, this would be the same design. These were all issues covered in the meeting on Tuesday, let's not re-litigate them here.
On 2016/01/22 17:43:22, mtklein wrote: > On 2016/01/22 17:41:54, jcgregorio wrote: > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > > > See the design doc: > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > You guys are sure you've got people who need to use a debugger remotely? > That > > seems like a feature no one will ever use. > > > > The bikeshed is blue. Let's commit. > > This isn't really a bikeshed issue. You guys have greatly complicated your > design to accommodate this one feature. Seems like you might want to make sure > that feature matters. Strongly disagree with this. The HTTP support is primarily to simplify implementation by leveraging web browsers, rather than create our own cross-platform UI for it. The fact that we trivially get remote debugging support out of it is a nice side effect.
On 2016/01/22 17:50:14, ethannicholas wrote: > On 2016/01/22 17:43:22, mtklein wrote: > > On 2016/01/22 17:41:54, jcgregorio wrote: > > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > > > > > See the design doc: > > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > > > You guys are sure you've got people who need to use a debugger remotely? > > That > > > seems like a feature no one will ever use. > > > > > > The bikeshed is blue. Let's commit. > > > > This isn't really a bikeshed issue. You guys have greatly complicated your > > design to accommodate this one feature. Seems like you might want to make > sure > > that feature matters. > > Strongly disagree with this. The HTTP support is primarily to simplify > implementation by leveraging web browsers, rather than create our own > cross-platform UI for it. The fact that we trivially get remote debugging > support out of it is a nice side effect. I'm not saying using a web browser for the UI is a bad idea. I think that's a great idea. I'm saying that contorting your design to support remote use is a bad idea if no one will use it. HTML and browsers for UI, right there with you. I don't see any need to use HTTP here. If you drop support for remote debugging, your program doesn't need to serve anything. It can just dump out a .html file.
On 2016/01/22 18:12:19, mtklein wrote: > On 2016/01/22 17:50:14, ethannicholas wrote: > > On 2016/01/22 17:43:22, mtklein wrote: > > > On 2016/01/22 17:41:54, jcgregorio wrote: > > > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > > > > > > > See the design doc: > > > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > > > > > You guys are sure you've got people who need to use a debugger remotely? > > > > That > > > > seems like a feature no one will ever use. > > > > > > > > The bikeshed is blue. Let's commit. > > > > > > This isn't really a bikeshed issue. You guys have greatly complicated your > > > design to accommodate this one feature. Seems like you might want to make > > sure > > > that feature matters. > > > > Strongly disagree with this. The HTTP support is primarily to simplify > > implementation by leveraging web browsers, rather than create our own > > cross-platform UI for it. The fact that we trivially get remote debugging > > support out of it is a nice side effect. > > I'm not saying using a web browser for the UI is a bad idea. I think that's a > great idea. > I'm saying that contorting your design to support remote use is a bad idea if no > one will use it. > > HTML and browsers for UI, right there with you. I don't see any need to use > HTTP here. > If you drop support for remote debugging, your program doesn't need to serve > anything. It can just dump out a .html file. How would you propose handling the interactive features in that case?
On 2016/01/22 18:13:49, ethannicholas wrote: > On 2016/01/22 18:12:19, mtklein wrote: > > On 2016/01/22 17:50:14, ethannicholas wrote: > > > On 2016/01/22 17:43:22, mtklein wrote: > > > > On 2016/01/22 17:41:54, jcgregorio wrote: > > > > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > > > > That won't work, file:// is not equivalent to http://localhost. > > > > > > > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > > > > > > > > > See the design doc: > > > > > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > > > > > > > You guys are sure you've got people who need to use a debugger > remotely? > > > > > > That > > > > > seems like a feature no one will ever use. > > > > > > > > > > The bikeshed is blue. Let's commit. > > > > > > > > This isn't really a bikeshed issue. You guys have greatly complicated > your > > > > design to accommodate this one feature. Seems like you might want to > make > > > sure > > > > that feature matters. > > > > > > Strongly disagree with this. The HTTP support is primarily to simplify > > > implementation by leveraging web browsers, rather than create our own > > > cross-platform UI for it. The fact that we trivially get remote debugging > > > support out of it is a nice side effect. > > > > I'm not saying using a web browser for the UI is a bad idea. I think that's a > > great idea. > > I'm saying that contorting your design to support remote use is a bad idea if > no > > one will use it. > > > > HTML and browsers for UI, right there with you. I don't see any need to use > > HTTP here. > > If you drop support for remote debugging, your program doesn't need to serve > > anything. It can just dump out a .html file. > > How would you propose handling the interactive features in that case? Javascript of course, in a <script> tag in the file.
On 2016/01/22 18:14:36, mtklein wrote: > On 2016/01/22 18:13:49, ethannicholas wrote: > > On 2016/01/22 18:12:19, mtklein wrote: > > > On 2016/01/22 17:50:14, ethannicholas wrote: > > > > On 2016/01/22 17:43:22, mtklein wrote: > > > > > On 2016/01/22 17:41:54, jcgregorio wrote: > > > > > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > > > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > > > > > That won't work, file:// is not equivalent to > http://localhost. > > > > > > > > > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into play? > > > > > > > > > > > > > > > > See the design doc: > > > > > > > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > > > > > > > > > You guys are sure you've got people who need to use a debugger > > remotely? > > > > > > > > That > > > > > > seems like a feature no one will ever use. > > > > > > > > > > > > The bikeshed is blue. Let's commit. > > > > > > > > > > This isn't really a bikeshed issue. You guys have greatly complicated > > your > > > > > design to accommodate this one feature. Seems like you might want to > > make > > > > sure > > > > > that feature matters. > > > > > > > > Strongly disagree with this. The HTTP support is primarily to simplify > > > > implementation by leveraging web browsers, rather than create our own > > > > cross-platform UI for it. The fact that we trivially get remote debugging > > > > support out of it is a nice side effect. > > > > > > I'm not saying using a web browser for the UI is a bad idea. I think that's > a > > > great idea. > > > I'm saying that contorting your design to support remote use is a bad idea > if > > no > > > one will use it. > > > > > > HTML and browsers for UI, right there with you. I don't see any need to use > > > HTTP here. > > > If you drop support for remote debugging, your program doesn't need to serve > > > anything. It can just dump out a .html file. > > > > How would you propose handling the interactive features in that case? > > Javascript of course, in a <script> tag in the file. I'm not referring to interactive-within-the-browser features, but rather interacting-with-Skia features. For instance, the design I described would allow you to interactively look at the contents of an SKP file, edit it, and see live updates.
On 2016/01/22 18:15:47, ethannicholas wrote: > On 2016/01/22 18:14:36, mtklein wrote: > > On 2016/01/22 18:13:49, ethannicholas wrote: > > > On 2016/01/22 18:12:19, mtklein wrote: > > > > On 2016/01/22 17:50:14, ethannicholas wrote: > > > > > On 2016/01/22 17:43:22, mtklein wrote: > > > > > > On 2016/01/22 17:41:54, jcgregorio wrote: > > > > > > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > > > > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > > > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > > > > > > That won't work, file:// is not equivalent to > > http://localhost. > > > > > > > > > > > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into > play? > > > > > > > > > > > > > > > > > > See the design doc: > > > > > > > > > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > > > > > > > > > > > You guys are sure you've got people who need to use a debugger > > > remotely? > > > > > > > > > > That > > > > > > > seems like a feature no one will ever use. > > > > > > > > > > > > > > The bikeshed is blue. Let's commit. > > > > > > > > > > > > This isn't really a bikeshed issue. You guys have greatly complicated > > > your > > > > > > design to accommodate this one feature. Seems like you might want to > > > make > > > > > sure > > > > > > that feature matters. > > > > > > > > > > Strongly disagree with this. The HTTP support is primarily to simplify > > > > > implementation by leveraging web browsers, rather than create our own > > > > > cross-platform UI for it. The fact that we trivially get remote > debugging > > > > > support out of it is a nice side effect. > > > > > > > > I'm not saying using a web browser for the UI is a bad idea. I think > that's > > a > > > > great idea. > > > > I'm saying that contorting your design to support remote use is a bad idea > > if > > > no > > > > one will use it. > > > > > > > > HTML and browsers for UI, right there with you. I don't see any need to > use > > > > HTTP here. > > > > If you drop support for remote debugging, your program doesn't need to > serve > > > > anything. It can just dump out a .html file. > > > > > > How would you propose handling the interactive features in that case? > > > > Javascript of course, in a <script> tag in the file. > > I'm not referring to interactive-within-the-browser features, but rather > interacting-with-Skia features. For instance, the design I described would allow > you to interactively look at the contents of an SKP file, edit it, and see live > updates. Holy crap, why are you even proposing using the web as a UI for this? Just write a normal app that links Skia!
On 2016/01/22 at 18:18:07, mtklein wrote: > On 2016/01/22 18:15:47, ethannicholas wrote: > > On 2016/01/22 18:14:36, mtklein wrote: > > > On 2016/01/22 18:13:49, ethannicholas wrote: > > > > On 2016/01/22 18:12:19, mtklein wrote: > > > > > On 2016/01/22 17:50:14, ethannicholas wrote: > > > > > > On 2016/01/22 17:43:22, mtklein wrote: > > > > > > > On 2016/01/22 17:41:54, jcgregorio wrote: > > > > > > > > On 2016/01/22 at 17:37:48, mtklein wrote: > > > > > > > > > On 2016/01/22 17:29:28, jcgregorio wrote: > > > > > > > > > > On 2016/01/22 at 17:25:43, mtklein wrote: > > > > > > > > > > > > That won't work, file:// is not equivalent to > > > http://localhost. > > > > > > > > > > > > > > > > > > > > > > I'm not sure I follow. Where's http://localhost come into > > play? > > > > > > > > > > > > > > > > > > > > See the design doc: > > > > > > > > > > > > > > https://github.com/google/skia-buildbot/blob/master/debugger/DESIGN.md > > > > > > > > > > > > > > > > > > You guys are sure you've got people who need to use a debugger > > > > remotely? > > > > > > > > > > > > That > > > > > > > > seems like a feature no one will ever use. > > > > > > > > > > > > > > > > The bikeshed is blue. Let's commit. > > > > > > > > > > > > > > This isn't really a bikeshed issue. You guys have greatly complicated > > > > your > > > > > > > design to accommodate this one feature. Seems like you might want to > > > > make > > > > > > sure > > > > > > > that feature matters. > > > > > > > > > > > > Strongly disagree with this. The HTTP support is primarily to simplify > > > > > > implementation by leveraging web browsers, rather than create our own > > > > > > cross-platform UI for it. The fact that we trivially get remote > > debugging > > > > > > support out of it is a nice side effect. > > > > > > > > > > I'm not saying using a web browser for the UI is a bad idea. I think > > that's > > > a > > > > > great idea. > > > > > I'm saying that contorting your design to support remote use is a bad idea > > > if > > > > no > > > > > one will use it. > > > > > > > > > > HTML and browsers for UI, right there with you. I don't see any need to > > use > > > > > HTTP here. > > > > > If you drop support for remote debugging, your program doesn't need to > > serve > > > > > anything. It can just dump out a .html file. > > > > > > > > How would you propose handling the interactive features in that case? > > > > > > Javascript of course, in a <script> tag in the file. > > > > I'm not referring to interactive-within-the-browser features, but rather > > interacting-with-Skia features. For instance, the design I described would allow > > you to interactively look at the contents of an SKP file, edit it, and see live > > updates. > > Holy crap, why are you even proposing using the web as a UI for this? Just write a normal app that links Skia! Like I said, bikeshed. If you wanted to have input, Tuesday was the day. Right now we are running a 2 week experiment to see how far we can get with this design, another one of those things discussed on Tuesday.
The CQ bit was checked by joshualitt@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jcgregorio@google.com Link to the patchset: https://codereview.chromium.org/1621753002/#ps20001 (title: "tweaks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1621753002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1621753002/20001
Message was sent while issue was closed.
Description was changed from ========== First rough draft of skiaserve Please excuse the mess while we iterate BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== First rough draft of skiaserve Please excuse the mess while we iterate BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/7f6a1e078641c1acd135da6ad04b9be53b29cf0d ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/7f6a1e078641c1acd135da6ad04b9be53b29cf0d |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
