Mojo content handler shebang support

mojo/common/data_pipe_utils.cc

Index: mojo/common/data_pipe_utils.cc
diff --git a/mojo/common/data_pipe_utils.cc b/mojo/common/data_pipe_utils.cc
index 428a5897b05074ebcae62029659adbace64314c7..6d9cf11aba815fef9561b027588cf04784514e5e 100644
--- a/mojo/common/data_pipe_utils.cc
+++ b/mojo/common/data_pipe_utils.cc
@@ -11,6 +11,7 @@
 #include "base/files/scoped_file.h"
 #include "base/message_loop/message_loop.h"
 #include "base/task_runner_util.h"
+#include "base/threading/platform_thread.h"
 
 namespace mojo {
 namespace common {
@@ -58,9 +59,139 @@ size_t CopyToFileHelper(FILE* fp, const void* buffer, uint32_t num_bytes) {
   return fwrite(buffer, 1, num_bytes, fp);
 }
 
+// Sleep for as long as max_sleep_micros if the deadline hasn't been reached
+// and the number of bytes read is still increasing. Returns true if sleep
+// was actually called.
+//
+// This class is a substitute for being able to wait until N bytes are available
+// from a data pipe. The MaybeSleep method is called when num_bytes_read are
+// available but more are needed by the Peek operation. If a second
+// Peek operation finds the same number of bytes after sleeping we assume
+// that there's no point in trying again.

[viettrungluu, 2014/10/31 23:30:23]

Please add a TODO for yourself, since this is pretty hacky.

(Maybe also file a bug, in which case link to the bug in your TODO.)

[hansmuller, 2014/11/01 00:06:04]

Done.

+

[viettrungluu, 2014/10/31 23:30:22]

nit: no blank line here

[hansmuller, 2014/11/01 00:06:05]

Done.

+class PeekSleeper {

[viettrungluu, 2014/10/31 23:30:22]

For some reason, having this helper class seems like overkill to me.

[hansmuller, 2014/11/01 00:06:05]

I initially wrote the code without it. Moving the sleep code to a helper class
made BlockingPeekHelper much easier to read.

+public:

[viettrungluu, 2014/10/31 23:30:23]

indent one space

(consider running clang-format)

[hansmuller, 2014/11/01 00:06:05]

Done.

+  PeekSleeper(MojoTimeTicks deadline)

[viettrungluu, 2014/10/31 23:30:23]

explicit

[hansmuller, 2014/11/01 00:06:05]

Done.

+      : deadline_(deadline),
+        last_number_bytes_read_(0),
+        max_sleep_micros_(1000 * 10) {

[viettrungluu, 2014/10/31 23:30:22]

This is a constant, so it should be declared as such.

[hansmuller, 2014/11/01 00:06:05]

Done.

+  }
+
+  bool MaybeSleep(uint32 num_bytes_read) {
+    if (num_bytes_read > 0 && last_number_bytes_read_ >= num_bytes_read)
+      return false;
+    last_number_bytes_read_ = num_bytes_read;
+
+    MojoTimeTicks now(GetTimeTicksNow());
+    if (now > deadline_)
+      return false;
+
+    MojoTimeTicks sleep_time = (deadline_ == 0)
+        ? max_sleep_micros_
+        : std::min<int64>(deadline_ - now, max_sleep_micros_);
+    base::PlatformThread::Sleep(base::TimeDelta::FromMicroseconds(sleep_time));
+    return true;
+  }
+
+private:

[viettrungluu, 2014/10/31 23:30:23]

indent one space

[hansmuller, 2014/11/01 00:06:05]

Done.

+  MojoTimeTicks deadline_; // 0 => MOJO_DEADLINE_INDEFINITE
+  uint32 last_number_bytes_read_;
+  MojoTimeTicks max_sleep_micros_; // microseconds
+};
+
+enum PeekStatus {kSuccess, kFail, kKeepReading};

[viettrungluu, 2014/10/31 23:30:23]

space after {, before }

[hansmuller, 2014/11/01 00:06:05]

Done.

+typedef PeekStatus (*PeekFunc)(const void*, uint32_t, size_t, std::string*);

[viettrungluu, 2014/10/31 23:30:23]

I find the fact that the size_t argument has varying meaning rather disturbing.

Possibly, you should use base::Callback() and then base::Bind() (partially
binding the size_t argument) instead, though admittedly that's more heavyweight.

[hansmuller, 2014/11/03 23:24:24]

Done.

+
+// When data is available on source, call peek_func and then either return true
+// and value, continue waiting for enough data to satisfy peek_func, or fail
+// and return false. Fail if the timeout is exceeded.
+

[viettrungluu, 2014/10/31 23:30:23]

no blank line

[hansmuller, 2014/11/01 00:06:05]

Done.

+bool BlockingPeekHelper(DataPipeConsumerHandle source,
+                        std::string* value,
+                        size_t value_length,
+                        MojoDeadline timeout,
+                        PeekFunc peek_func) {
+  CHECK(value);

[viettrungluu, 2014/10/31 23:30:23]

Probably a DCHECK is sufficient.

[hansmuller, 2014/11/01 00:06:04]

Done.

+  value->clear();
+
+  MojoTimeTicks deadline = (timeout == MOJO_DEADLINE_INDEFINITE) ? 0
+      : 1 + GetTimeTicksNow() + static_cast<MojoTimeTicks>(timeout);
+  PeekSleeper sleeper(deadline);
+  MojoResult result = MOJO_RESULT_OK;
+
+  while(result == MOJO_RESULT_OK) {

[viettrungluu, 2014/10/31 23:30:23]

Maybe write this as a do-while instead?

[viettrungluu, 2014/11/03 18:36:56]

You missed this.

[hansmuller, 2014/11/03 23:24:24]

Done.

+    const void* buffer;
+    uint32_t num_bytes;
+    result =
+        BeginReadDataRaw(source, &buffer, &num_bytes, MOJO_READ_DATA_FLAG_NONE);

[viettrungluu, 2014/10/31 23:30:23]

So if this is called not at the beginning of the buffer, it may never get the
data that you want, even if it's available. :/

[viettrungluu, 2014/11/03 18:36:56]

And this....

+
+    if (result == MOJO_RESULT_OK) {
+      PeekStatus status = peek_func(buffer, num_bytes, value_length, value);
+      if (EndReadDataRaw(source, 0) != MOJO_RESULT_OK)

[viettrungluu, 2014/10/31 23:30:23]

Probably just do a CHECK_NE.

[viettrungluu, 2014/11/03 18:36:56]

...

[hansmuller, 2014/11/03 23:24:24]

Done.

+        return false;
+      switch (status) {
+        case PeekStatus::kSuccess: return true;
+        case PeekStatus::kFail: return false;
+        case PeekStatus::kKeepReading: break;
+      }
+      if (!sleeper.MaybeSleep(num_bytes))
+        return false;
+

[viettrungluu, 2014/10/31 23:30:23]

no blank line

[hansmuller, 2014/11/01 00:06:05]

Done.

+    } else if (result == MOJO_RESULT_SHOULD_WAIT) {
+      MojoTimeTicks now(GetTimeTicksNow());
+      if (timeout == MOJO_DEADLINE_INDEFINITE || now < deadline)
+        result = Wait(source, MOJO_HANDLE_SIGNAL_READABLE, deadline - now);
+    }
+  }
+
+  return false;
+}
+
+PeekStatus PeekLine(const void* buffer,
+                        uint32 buffer_num_bytes,
+                        size_t max_line_length,
+                        std::string* line) {
+  const char* p = static_cast<const char*>(buffer);
+  std::string s(p, buffer_num_bytes);

[viettrungluu, 2014/10/31 23:30:23]

It seems quite painful to build an entire string just so you can do s.find() on
it.

Possibly you should use StringPiece, for maybe you should just write a loop:

for (size_t i = 0; i < std::min(buffer_num_bytes, max_line_length); i++) {
  if (p[i] == '\n') {
    *line = std::string(p, n);
    return PeekStatus::kSuccess;
  }
}
return (i > max_line_length) ? PeekStatus::kFail : PeekStatus:kKeepReading;

[hansmuller, 2014/11/01 00:06:04]

Since you went to the trouble of writing a loop, I've just used it. 

+  size_t n = s.find("\n");
+  if (n != std::string::npos && n <= max_line_length) {
+    *line = s.substr(0, n);

[viettrungluu, 2014/10/31 23:30:23]

This won't include the '\n', will it? I find that slightly odd.

[hansmuller, 2014/11/01 00:06:05]

Yes he line return value doesn't include the terminating newline. It didn't seem
odd to me but I don't have a strong feeling about it. I will include it.

[viettrungluu, 2014/11/03 18:36:56]

I guess for "peek", it's less odd, but you should have it to be parallel to a
hypothetical "read" (which would either include the \n or at least dispose of
it).

+    return PeekStatus::kSuccess;
+  }
+  if (s.size() > max_line_length)
+    return PeekStatus::kFail;
+  return PeekStatus::kKeepReading;
+}
+
+PeekStatus PeekNBytes(const void* buffer,
+                          uint32 buffer_num_bytes,

[viettrungluu, 2014/10/31 23:30:23]

fix indentation

[hansmuller, 2014/11/01 00:06:05]

Done.

+                          size_t bytes_length,
+                          std::string* bytes) {
+  if (buffer_num_bytes >= bytes_length) {
+    const char* p = static_cast<const char*>(buffer);
+    *bytes = std::string(p, bytes_length);
+    return PeekStatus::kSuccess;
+  }
+  return PeekStatus::kKeepReading;
+}
+
 } // namespace
 
 

[viettrungluu, 2014/10/31 23:30:23]

nit: one fewer blank line

[hansmuller, 2014/11/01 00:06:04]

Done.

+bool BlockingPeekNBytes(DataPipeConsumerHandle source,
+                        std::string* bytes,
+                        size_t bytes_length,
+                        MojoDeadline timeout) {
+  return BlockingPeekHelper(source, bytes, bytes_length, timeout, &PeekNBytes);
+}
+
+bool BlockingPeekLine(DataPipeConsumerHandle source,
+                      std::string* line,
+                      size_t max_line_length,
+                      MojoDeadline timeout) {
+  return BlockingPeekHelper(source, line, max_line_length, timeout, &PeekLine);
+}
+
 // TODO(hansmuller): Add a max_size parameter.
 bool BlockingCopyToString(ScopedDataPipeConsumerHandle source,
                           std::string* result) {