Pepper: Clean up ProgressEvent logic.

Pepper: Clean up ProgressEvent logic.

This simplifies NexeLoadManager, which is getting pretty big. This also removes
an unused field from ProgressEvent, and adds a convenience function for posting
a task to dispatch a progress event from the main thread, which is often
necessary to ensure that progress events are posted to the DOM after other
state changes.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269362

R=dmichael@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269784

[teravest, 2014-05-08 15:46:21]

ping?

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

lgtm

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:656: event.total_bytes =
total_bytes;
I don't particularly care, but this looks like it could use the constructor like
other places. Or other places could look like this, if you think this is better.
Or is there a reason for this to be different?

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
File components/nacl/renderer/progress_event.h (right):

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:27: uint64_t loaded_bytes, uint64_t
total_bytes)
nit: for declarations & definitions, each param should get its own line.

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:28: : event_type(event_type),
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.

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:44: void
DispatchProgressEvent(PP_Instance instance, const ProgressEvent& event);
If we're not using this one anywhere outside progress_event.cc, maybe it should
only live in the body?

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:48: void
PostDispatchProgressEvent(PP_Instance instance,
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:02]

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:656: event.total_bytes =
total_bytes;
On 2014/05/08 16:19:02, dmichael wrote:
> I don't particularly care, but this looks like it could use the constructor
like
> other places. Or other places could look like this, if you think this is
better.
> Or is there a reason for this to be different?

Done.

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
File components/nacl/renderer/progress_event.h (right):

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:27: uint64_t loaded_bytes, uint64_t
total_bytes)
On 2014/05/08 16:19:02, dmichael wrote:
> nit: for declarations & definitions, each param should get its own line.

Done.

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:28: : event_type(event_type),
On 2014/05/08 16:19:02, dmichael wrote:
> 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.

Done.

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:44: void
DispatchProgressEvent(PP_Instance instance, const ProgressEvent& event);
On 2014/05/08 16:19:02, dmichael wrote:
> If we're not using this one anywhere outside progress_event.cc, maybe it
should
> only live in the body?

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

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:48: void
PostDispatchProgressEvent(PP_Instance instance,
On 2014/05/08 16:19:02, dmichael wrote:
> 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.

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

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

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
File components/nacl/renderer/progress_event.h (right):

https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
components/nacl/renderer/progress_event.h:44: void
DispatchProgressEvent(PP_Instance instance, const ProgressEvent& event);
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.

[dmichael (off chromium), 2014-05-08 16:50:10]

On 2014/05/08 16:49:11, dmichael wrote:
>
https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
> File components/nacl/renderer/progress_event.h (right):
> 
>
https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
> components/nacl/renderer/progress_event.h:44: void
> DispatchProgressEvent(PP_Instance instance, const ProgressEvent& event);
> 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.
Oh, #3:
The non-post version can only be called on the main thread, but doesn't do any
checking or document that fact.

[teravest, 2014-05-08 16:50:46]

On Thu, May 8, 2014 at 10:49 AM,  <dmichael@chromium.org> wrote:
>
>
https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
> File components/nacl/renderer/progress_event.h (right):
>
>
https://codereview.chromium.org/270453004/diff/1/components/nacl/renderer/pro...
> components/nacl/renderer/progress_event.h:44: void
> DispatchProgressEvent(PP_Instance instance, const ProgressEvent& event);
> 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.

Alright, fine, though I'll grumble a bit under my breath.
I think we're going to wastefully post tasks to the main thread, but I
guess it's good to be simple.

>
> https://codereview.chromium.org/270453004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-05-08 18:01:45]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-05-08 18:06:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/270453004/40001

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

Change committed as 269362

[hamaji, 2014-05-12 07:11:56]

A revert of this CL has been created in
https://codereview.chromium.org/275403004/ by hamaji@chromium.org.

The reason for reverting is: This broke our app. We do not understand why the
breakage happened or
why browser_tests could not catch this yet. However, as our app do not
want this in M36, I'd revert this change without checking.

BUG=372163
.

[hamaji, 2014-05-12 14:29:15]

I tried the patch set 4 locally, and it is working for us.

[teravest, 2014-05-12 14:32:30]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-05-12 14:32:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/270453004/60001

[teravest, 2014-05-12 15:30:45]

Committed patchset #4 manually as r269784 (presubmit successful).