Initial code for webtry, a web application for allowing users to try out Skia.

Initial code for webtry, a web application for allowing users to try out Skia.

Currently running at http://108.170.220.126:8000/

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=14114

[mtklein, 2014-04-08 19:00:22]

Man, this is so cool.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
File experimental/webtry/main.cpp (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:20: static int installSyscallFilter(void) {
Usually static methods are named_like_this

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:39: (unsigned
short)(sizeof(filter)/sizeof(filter[0])),
SK_ARRAY_COUNT?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:43: if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
This function could use a bit of comment narration.  I can guess what's going on
up until here, but it's not clear to me what the PR_SET_NO_NEW_PRIVS call does.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:59: void setLimits() {
static void set_limits?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:77: void draw(SkCanvas* canvas);
Can't hurt to remind us that this comes from outside with an explicit "extern"?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:81: SkForceLinking(false);
Typically I use the __SK_FORCE_IMAGE_DECODER_LINKING macro up top somewhere, if
only so you don't have to think about what false means.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:84: SkFILEWStream stream(FLAGS_out[0]);
Bail if FLAGS_out.count() == 0?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:92: if (FLAGS_logSyscall &&
install_syscall_reporter()) {
Ordinarily I'd prefer true, false over 0, 1.  I guess you're writing
installSyscallFilter() to be similar to install_sycall_reporter(), which returns
an int because it's C?  I'm on board with that if so.  Can you note
installSyscallFilter() returns 0 on success, and check != 0 for both of these
calls?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:102: SkAutoTUnref<SkImage>
image(surface->newImageSnapshot());
I know it's sort of notional, but I can't help but notice that this code doesn't
quite match up with what we show on the page.  Think it's worth making it linked
to the same source, or is it inevitable that we make little white lies about
what support code's running?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
File experimental/webtry/seccomp_bpf.h (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
experimental/webtry/seccomp_bpf.h:11: */
I gather this is pulled from chromium/src somewhere?  Can you provide a pointer
so that it's clear where to look if we want to update, or if for some reason
someone actually wanted to look at LICENSE?

Actually, can we grab these couple files and the LICENSE and put them in their
own dir in third_party with a note about how they got there?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
experimental/webtry/seccomp_bpf.h:66: #define VALIDATE_ARCHITECTURE \
Is it important that I (or we, you?) understand what's going on in here?  I'm
willing to learn, of course, but just as willing to glaze over magic if this is
unmodified working code.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
File experimental/webtry/syscall_names.h (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_names.h:6: syscall_names[247] = "waitid";
Dare I ask why this doesn't go in order, or why this is not static const char*
kSyscallNames[] = { ... }

Again, pulled verbatim from somewhere?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
File experimental/webtry/syscall_reporter.cpp (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_reporter.cpp:20: /* Since "sprintf" is technically
not signal-safe, reimplement %d here. */
Badass

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_reporter.cpp:39: #define SIZE 100
I'm getting the impression that this file was just pulled wholesale, but man is
it weird and ugly code.  Having compared the code you wrote yourself and this
code, I'd sort of prefer it if you or I rewrote this from scratch rather than
use this as is.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_reporter.cpp:62: {
This code has some of the oddest formatting I've ever seen.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_reporter.cpp:85: int ret = write(STDOUT_FILENO, buf,
strlen(buf));
WTF do these next 4 lines do?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_reporter.cpp:87: ret = ret;
Particularly WTF.  Is there some implicit register loading going on here?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
File experimental/webtry/webtry.go (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:35: useChroot = flag.Bool("use_chroot", false,
"Run the compiled code in the schroot jail.")
My paranoia wants this to default to true now that it's an option.  I guess we
support --nouse_chroot only for local testing?  Can we make local testing work
smoothly with a chroot?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:50: // Change the current working directory to be
where the directory of the executable.
Funky wording?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:69: type userCode struct {
I've got no problem with the struct if you like it, but sure seems like you
could get by with just string for now.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:85: func ExpandCode(code string) (string, error) {
expandCode?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:92: err :=
expandToFile(fmt.Sprintf("../../../cache/%s.cpp", hash), content, codeTemplate)
Could be helpful to remind the reader where we are the the hierarchy right now
(or will be when it's used).  ../../.. is sort of terrifying without a map.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:97: type Response struct {
response?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:103: func doCmd(commandLine string, moveToDebug
bool) (string, error) {
Not clear here what it returns?  can this be (stdout string, error)?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:118: } else if *useChroot {
?

if moveToDebug {

} else if !*useChroot {

}
?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:120: abs, err := filepath.Abs("../../../inout")
Might help readability to make some of these literal relative paths constants
(or flags).

[mtklein, 2014-04-08 19:15:07]

On 2014/04/08 19:00:22, mtklein wrote:
> Man, this is so cool.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> File experimental/webtry/main.cpp (right):
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:20: static int installSyscallFilter(void) {
> Usually static methods are named_like_this
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:39: (unsigned
> short)(sizeof(filter)/sizeof(filter[0])),
> SK_ARRAY_COUNT?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:43: if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
> This function could use a bit of comment narration.  I can guess what's going
on
> up until here, but it's not clear to me what the PR_SET_NO_NEW_PRIVS call
does.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:59: void setLimits() {
> static void set_limits?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:77: void draw(SkCanvas* canvas);
> Can't hurt to remind us that this comes from outside with an explicit
"extern"?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:81: SkForceLinking(false);
> Typically I use the __SK_FORCE_IMAGE_DECODER_LINKING macro up top somewhere,
if
> only so you don't have to think about what false means.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:84: SkFILEWStream stream(FLAGS_out[0]);
> Bail if FLAGS_out.count() == 0?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:92: if (FLAGS_logSyscall &&
> install_syscall_reporter()) {
> Ordinarily I'd prefer true, false over 0, 1.  I guess you're writing
> installSyscallFilter() to be similar to install_sycall_reporter(), which
returns
> an int because it's C?  I'm on board with that if so.  Can you note
> installSyscallFilter() returns 0 on success, and check != 0 for both of these
> calls?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
> experimental/webtry/main.cpp:102: SkAutoTUnref<SkImage>
> image(surface->newImageSnapshot());
> I know it's sort of notional, but I can't help but notice that this code
doesn't
> quite match up with what we show on the page.  Think it's worth making it
linked
> to the same source, or is it inevitable that we make little white lies about
> what support code's running?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
> File experimental/webtry/seccomp_bpf.h (right):
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
> experimental/webtry/seccomp_bpf.h:11: */
> I gather this is pulled from chromium/src somewhere?  Can you provide a
pointer
> so that it's clear where to look if we want to update, or if for some reason
> someone actually wanted to look at LICENSE?
> 
> Actually, can we grab these couple files and the LICENSE and put them in their
> own dir in third_party with a note about how they got there?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
> experimental/webtry/seccomp_bpf.h:66: #define VALIDATE_ARCHITECTURE \
> Is it important that I (or we, you?) understand what's going on in here?  I'm
> willing to learn, of course, but just as willing to glaze over magic if this
is
> unmodified working code.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> File experimental/webtry/syscall_names.h (right):
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> experimental/webtry/syscall_names.h:6: syscall_names[247] = "waitid";
> Dare I ask why this doesn't go in order, or why this is not static const char*
> kSyscallNames[] = { ... }
> 
> Again, pulled verbatim from somewhere?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> File experimental/webtry/syscall_reporter.cpp (right):
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> experimental/webtry/syscall_reporter.cpp:20: /* Since "sprintf" is technically
> not signal-safe, reimplement %d here. */
> Badass
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> experimental/webtry/syscall_reporter.cpp:39: #define SIZE 100
> I'm getting the impression that this file was just pulled wholesale, but man
is
> it weird and ugly code.  Having compared the code you wrote yourself and this
> code, I'd sort of prefer it if you or I rewrote this from scratch rather than
> use this as is.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> experimental/webtry/syscall_reporter.cpp:62: {
> This code has some of the oddest formatting I've ever seen.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> experimental/webtry/syscall_reporter.cpp:85: int ret = write(STDOUT_FILENO,
buf,
> strlen(buf));
> WTF do these next 4 lines do?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
> experimental/webtry/syscall_reporter.cpp:87: ret = ret;
> Particularly WTF.  Is there some implicit register loading going on here?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> File experimental/webtry/webtry.go (right):
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:35: useChroot = flag.Bool("use_chroot", false,
> "Run the compiled code in the schroot jail.")
> My paranoia wants this to default to true now that it's an option.  I guess we
> support --nouse_chroot only for local testing?  Can we make local testing work
> smoothly with a chroot?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:50: // Change the current working directory to
be
> where the directory of the executable.
> Funky wording?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:69: type userCode struct {
> I've got no problem with the struct if you like it, but sure seems like you
> could get by with just string for now.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:85: func ExpandCode(code string) (string, error)
{
> expandCode?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:92: err :=
> expandToFile(fmt.Sprintf("../../../cache/%s.cpp", hash), content,
codeTemplate)
> Could be helpful to remind the reader where we are the the hierarchy right now
> (or will be when it's used).  ../../.. is sort of terrifying without a map.
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:97: type Response struct {
> response?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:103: func doCmd(commandLine string, moveToDebug
> bool) (string, error) {
> Not clear here what it returns?  can this be (stdout string, error)?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:118: } else if *useChroot {
> ?
> 
> if moveToDebug {
> 
> } else if !*useChroot {
> 
> }
> ?
> 
>
https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
> experimental/webtry/webtry.go:120: abs, err := filepath.Abs("../../../inout")
> Might help readability to make some of these literal relative paths constants
> (or flags).

As a user, I've got some feature requests:
   1) functions
   2) compile logs on error
:)

[jcgregorio, 2014-04-09 14:10:31]

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
File experimental/webtry/main.cpp (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:20: static int installSyscallFilter(void) {
On 2014/04/08 19:00:23, mtklein wrote:
> Usually static methods are named_like_this

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:39: (unsigned
short)(sizeof(filter)/sizeof(filter[0])),
On 2014/04/08 19:00:23, mtklein wrote:
> SK_ARRAY_COUNT?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:43: if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
On 2014/04/08 19:00:23, mtklein wrote:
> This function could use a bit of comment narration.  I can guess what's going
on
> up until here, but it's not clear to me what the PR_SET_NO_NEW_PRIVS call
does.

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:59: void setLimits() {
On 2014/04/08 19:00:23, mtklein wrote:
> static void set_limits?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:77: void draw(SkCanvas* canvas);
On 2014/04/08 19:00:23, mtklein wrote:
> Can't hurt to remind us that this comes from outside with an explicit
"extern"?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:81: SkForceLinking(false);
On 2014/04/08 19:00:23, mtklein wrote:
> Typically I use the __SK_FORCE_IMAGE_DECODER_LINKING macro up top somewhere,
if
> only so you don't have to think about what false means. 

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:84: SkFILEWStream stream(FLAGS_out[0]);
On 2014/04/08 19:00:23, mtklein wrote:
> Bail if FLAGS_out.count() == 0?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:92: if (FLAGS_logSyscall &&
install_syscall_reporter()) {
On 2014/04/08 19:00:23, mtklein wrote:
> Ordinarily I'd prefer true, false over 0, 1.  I guess you're writing
> installSyscallFilter() to be similar to install_sycall_reporter(), which
returns
> an int because it's C?  I'm on board with that if so.  Can you note
> installSyscallFilter() returns 0 on success, and check != 0 for both of these
> calls?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/main...
experimental/webtry/main.cpp:102: SkAutoTUnref<SkImage>
image(surface->newImageSnapshot());
Yeah, I'm torn between making the code look exactly like what's executed, or the
code that's here, which you could in theory cut and paste as a complete program
and compile into a working program.

For now I'm fine with the little white lie.

On 2014/04/08 19:00:23, mtklein wrote:
> I know it's sort of notional, but I can't help but notice that this code
doesn't
> quite match up with what we show on the page.  Think it's worth making it
linked
> to the same source, or is it inevitable that we make little white lies about
> what support code's running?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
File experimental/webtry/seccomp_bpf.h (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
experimental/webtry/seccomp_bpf.h:11: */
Stripped the file down to its absolute minimum and added a pointer to the
original file.

On 2014/04/08 19:00:23, mtklein wrote:
> I gather this is pulled from chromium/src somewhere?  Can you provide a
pointer
> so that it's clear where to look if we want to update, or if for some reason
> someone actually wanted to look at LICENSE?
> 
> Actually, can we grab these couple files and the LICENSE and put them in their
> own dir in third_party with a note about how they got there?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/secc...
experimental/webtry/seccomp_bpf.h:66: #define VALIDATE_ARCHITECTURE \
See above.

On 2014/04/08 19:00:23, mtklein wrote:
> Is it important that I (or we, you?) understand what's going on in here?  I'm
> willing to learn, of course, but just as willing to glaze over magic if this
is
> unmodified working code.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
File experimental/webtry/syscall_names.h (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_names.h:6: syscall_names[247] = "waitid";
Deleting this file.

On 2014/04/08 19:00:23, mtklein wrote:
> Dare I ask why this doesn't go in order, or why this is not static const char*
> kSyscallNames[] = { ... }
> 
> Again, pulled verbatim from somewhere?

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
File experimental/webtry/syscall_reporter.cpp (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/sysc...
experimental/webtry/syscall_reporter.cpp:1: /*
Deleting this file.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
File experimental/webtry/webtry.go (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:35: useChroot = flag.Bool("use_chroot", false,
"Run the compiled code in the schroot jail.")
On 2014/04/08 19:00:23, mtklein wrote:
> My paranoia wants this to default to true now that it's an option.  I guess we
> support --nouse_chroot only for local testing?

Not just local testing, but anyone can run this locally, and I wanted to make
that easy.

>  Can we make local testing work
> smoothly with a chroot?

There's weirdness with compiling under goobuntu and then running in wheezy,
which maybe we can fix at a later date by statically linking. For now I think
running locally w/o the chroot is fine.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:50: // Change the current working directory to be
where the directory of the executable.
On 2014/04/08 19:00:23, mtklein wrote:
> Funky wording?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:69: type userCode struct {
On 2014/04/08 19:00:23, mtklein wrote:
> I've got no problem with the struct if you like it, but sure seems like you
> could get by with just string for now.

I think that would make the template for expansion {{.}}, and I'm not even sure
that works, so I'd like to stick with the struct.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:85: func ExpandCode(code string) (string, error) {
On 2014/04/08 19:00:23, mtklein wrote:
> expandCode?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:92: err :=
expandToFile(fmt.Sprintf("../../../cache/%s.cpp", hash), content, codeTemplate)
On 2014/04/08 19:00:23, mtklein wrote:
> Could be helpful to remind the reader where we are the the hierarchy right now
> (or will be when it's used).  ../../.. is sort of terrifying without a map.

Done. Also added a TODO to make into flags.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:97: type Response struct {
On 2014/04/08 19:00:23, mtklein wrote:
> response?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:103: func doCmd(commandLine string, moveToDebug
bool) (string, error) {
On 2014/04/08 19:00:23, mtklein wrote:
> Not clear here what it returns?  can this be (stdout string, error)?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:118: } else if *useChroot {
On 2014/04/08 19:00:23, mtklein wrote:
> ?
> 
> if moveToDebug {
> 
> } else if !*useChroot {
> 
> }
> ?

Done.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:120: abs, err := filepath.Abs("../../../inout")
On 2014/04/08 19:00:23, mtklein wrote:
> Might help readability to make some of these literal relative paths constants
> (or flags).

Added as TODO above.

[mtklein, 2014-04-09 15:02:59]

lgtm

Please at least add {} around that if statement body before submitting.

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
File experimental/webtry/webtry.go (right):

https://codereview.chromium.org/228693002/diff/40001/experimental/webtry/webt...
experimental/webtry/webtry.go:69: type userCode struct {
On 2014/04/09 14:10:31, jcgregorio wrote:
> On 2014/04/08 19:00:23, mtklein wrote:
> > I've got no problem with the struct if you like it, but sure seems like you
> > could get by with just string for now.
> 
> I think that would make the template for expansion {{.}}, and I'm not even
sure
> that works, so I'd like to stick with the struct.
> 
Yeah, it would and it does work.  I think it's even idiomatic.  I see a lot of
templates that seem to use {{.}} exclusively for values, and use {{with .Foo}}
to move around in the tree.

That said, I'm switching sides and saying it's clearer in the template to see
{{.UserCode}} instead of {{.}}.  So this LGTM.

One thing I do sometimes is make the struct anonymous, just existing for the
template instantiation.  You can wait until the last minute to plug the members
in however you like:

return t.Execute(f, struct{
    UserCode string
}{
    UserCode: userCode,
})

(Think that works.  I'm freewheeling here.)

https://codereview.chromium.org/228693002/diff/60001/experimental/webtry/main...
File experimental/webtry/main.cpp (right):

https://codereview.chromium.org/228693002/diff/60001/experimental/webtry/main...
experimental/webtry/main.cpp:20: static bool install_syscall_filter(void) {
Just FYI, you can legally write () instead of (void) in C++.

https://codereview.chromium.org/228693002/diff/60001/experimental/webtry/main...
experimental/webtry/main.cpp:58: if (errno == EINVAL)
Wrap with {}?  I've seen too many
    if (errno == EINVAL)
        LOG(ERROR) << "Huh, how did this happen?";
        do_some_serious_error_handling();

Personally, I don't mind
    if (errno == EINVAL) fprintf(stderr, ...);
but that's emphatically not the Skia consensus.  We like our new lines _and_
braces.

[jcgregorio, 2014-04-09 15:23:07]

https://codereview.chromium.org/228693002/diff/60001/experimental/webtry/main...
File experimental/webtry/main.cpp (right):

https://codereview.chromium.org/228693002/diff/60001/experimental/webtry/main...
experimental/webtry/main.cpp:58: if (errno == EINVAL)
On 2014/04/09 15:03:00, mtklein wrote:
> Wrap with {}?  I've seen too many
>     if (errno == EINVAL)
>         LOG(ERROR) << "Huh, how did this happen?";
>         do_some_serious_error_handling();
> 
> Personally, I don't mind
>     if (errno == EINVAL) fprintf(stderr, ...);
> but that's emphatically not the Skia consensus.  We like our new lines _and_
> braces.

Done.

[jcgregorio, 2014-04-09 15:23:26]

The CQ bit was checked by jcgregorio@google.com

[commit-bot: I haz the power, 2014-04-09 15:23:30]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jcgregorio@google.com/228693002/80001

[commit-bot: I haz the power, 2014-04-09 17:05:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 17:05:30]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildEverything
http://skia-tree-status.appspot.com/redirect/compile-buildbots/buildstatus?bu...

[jcgregorio, 2014-04-09 17:43:32]

Fixed buildbot failure by removing webtry from everything.gyp. Updated scripts
and instructions accordingly.

[mtklein, 2014-04-09 17:51:27]

On 2014/04/09 17:43:32, jcgregorio wrote:
> Fixed buildbot failure by removing webtry from everything.gyp. Updated scripts
> and instructions accordingly.

:(  SGTM

[jcgregorio, 2014-04-09 17:58:56]

The CQ bit was checked by jcgregorio@google.com

[commit-bot: I haz the power, 2014-04-09 17:59:05]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/jcgregorio@google.com/228693002/100001

[commit-bot: I haz the power, 2014-04-09 19:00:05]

Change committed as 14114

[tfarina, 2014-09-20 03:55:20]

https://codereview.chromium.org/228693002/diff/100001/experimental/webtry/DESIGN
File experimental/webtry/DESIGN (right):

https://codereview.chromium.org/228693002/diff/100001/experimental/webtry/DES...
experimental/webtry/DESIGN:93: By sysv init only handles starting and stopping a
program once, so we use
s/By/But ?

[tfarina, 2014-09-25 16:14:50]

https://codereview.chromium.org/228693002/diff/100001/experimental/webtry/set...
File experimental/webtry/setup/webtry_setup.sh (right):

https://codereview.chromium.org/228693002/diff/100001/experimental/webtry/set...
experimental/webtry/setup/webtry_setup.sh:32: sudo monit -t
should we reload monit here as well to get the new changes?

sudo monit reload

[jcgregorio, 2014-09-25 17:05:59]

https://codereview.chromium.org/228693002/diff/100001/experimental/webtry/set...
File experimental/webtry/setup/webtry_setup.sh (right):

https://codereview.chromium.org/228693002/diff/100001/experimental/webtry/set...
experimental/webtry/setup/webtry_setup.sh:32: sudo monit -t
Yes, that would be great.

On 2014/09/25 16:14:50, tfarina wrote:
> should we reload monit here as well to get the new changes?
> 
> sudo monit reload