Pepper: Clean up ProgressEvent logic.

components/nacl/renderer/progress_event.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_NACL_RENDERER_NEXE_PROGRESS_EVENT_H_
+#define COMPONENTS_NACL_RENDERER_NEXE_PROGRESS_EVENT_H_
+
+#include <string>
+
+#include "ppapi/c/pp_instance.h"
+#include "ppapi/c/private/ppb_nacl_private.h"
+
+namespace nacl {
+
+// See http://www.w3.org/TR/progress-events/ for more details on progress
+// events.
+struct ProgressEvent {
+  explicit ProgressEvent(PP_NaClEventType event_type_param)
+      : event_type(event_type_param),
+        length_is_computable(false),
+        loaded_bytes(0),
+        total_bytes(0) {
+  }
+
+  ProgressEvent(PP_NaClEventType event_type, const std::string& resource_url,
+                bool length_is_computable,
+                uint64_t loaded_bytes, uint64_t total_bytes)

[dmichael (off chromium), 2014/05/08 16:19:02]

nit: for declarations & definitions, each param should get its own line.

[teravest, 2014/05/08 16:42:03]

Done.

+      : event_type(event_type),

[dmichael (off chromium), 2014/05/08 16:19:02]

nit: for clarity, I prefer if the param names are distinct (I sometimes tack on
_param, like event_type_param).

I'm not sure if other people are bugged by this style in Chromium or if it's a
personal problem of mine.

[teravest, 2014/05/08 16:42:03]

Done.

+        resource_url(resource_url),
+        length_is_computable(length_is_computable),
+        loaded_bytes(loaded_bytes),
+        total_bytes(total_bytes) {
+  }
+
+  PP_NaClEventType event_type;
+  std::string resource_url;
+  bool length_is_computable;
+  uint64_t loaded_bytes;
+  uint64_t total_bytes;
+};
+
+// Dispatches a progress event to the DOM frame corresponding to the specified
+// plugin instance.
+void DispatchProgressEvent(PP_Instance instance, const ProgressEvent& event);

[dmichael (off chromium), 2014/05/08 16:19:02]

If we're not using this one anywhere outside progress_event.cc, maybe it should
only live in the body?

[teravest, 2014/05/08 16:42:03]

I think it's reasonable to provide this interface; you should be able to
dispatch progress events without posting a task.

[dmichael (off chromium), 2014/05/08 16:49:11]

Two reasons not to:
1) We're not using it.
2) Using both can cause events to happen out-of-order.

#2 hadn't occurred to me before for some reason. I think we should have only 1
function, and later, if posting a task appears to cause too much latency from
the main thread, we can talk about making it possible to do those directly and
have a queue+lock or something to hopefully preserve order.

+
+// Like DispatchProgressEvent, but posts a task to do the actual work. Provided
+// as a convenience since this is a very common usage pattern.
+void PostDispatchProgressEvent(PP_Instance instance,

[dmichael (off chromium), 2014/05/08 16:19:02]

optional suggestion: I think making this a static function so it's called like
this:
ProgressEvent::Dispatch(instance, event)
Might make call-sites more obvious. At least, during this review, I wasn't sure
where this function came from until I got here.

It could even be a non-static member (event.Dispatch(instance)), but that
somehow feels a little weird to me.

[teravest, 2014/05/08 16:42:03]

It should be easy enough to find from the include paths; I don't feel the need
to make this a static function.

+                               const ProgressEvent& event);
+
+}  // namespace nacl
+
+#endif  // COMPONENTS_NACL_RENDERER_NEXE_PROGRESS_EVENT_H_