Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
127243002:
Pepper: Change ResourceMessageTestSink API for getting multiple messages. (Closed)
|
Created:
6 years, 11 months ago by bbudge Modified:
6 years, 11 months ago Reviewers:
raymes CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPepper: Change ResourceMessageTestSink API for getting multiple messages.
- Remove GetNext* methods.
- Add GetAll* methods, returning a vector of Params/Msg pairs.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243907
Patch Set 1 #
Total comments: 2
Patch Set 2 : Implement GetFirst* methods in terms of GetAll* ones. #
Messages
Total messages: 11 (0 generated)
Hmmm, not as bad as I thought. I didn't know about IPC::TestSink::ClearMessages().
Much better! Thanks. https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... File ppapi/proxy/resource_message_test_sink.h (right): https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... ppapi/proxy/resource_message_test_sink.h:43: bool GetFirstResourceReplyMatching( Can we get rid of these two methods now? Can't we just use GetAllResourceCallsMatching(..)[0]?
https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... File ppapi/proxy/resource_message_test_sink.h (right): https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... ppapi/proxy/resource_message_test_sink.h:43: bool GetFirstResourceReplyMatching( On 2014/01/08 02:21:22, raymes wrote: > Can we get rid of these two methods now? Can't we just use > GetAllResourceCallsMatching(..)[0]? It won't be so simple for callers, since the vector could be empty. I think it's a nice API for the general case where we expect only one message. I can implement the GetFirst* methods in terms of the GetAll* ones though, to reduce the amount of code.
But the callers have to check whether true is returned from the existing function. So it shouldn't be any more complex should it? On Wed, Jan 8, 2014 at 2:05 PM, <bbudge@chromium.org> wrote: > > https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... > File ppapi/proxy/resource_message_test_sink.h (right): > > https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... > ppapi/proxy/resource_message_test_sink.h:43: bool > GetFirstResourceReplyMatching( > On 2014/01/08 02:21:22, raymes wrote: >> >> Can we get rid of these two methods now? Can't we just use >> GetAllResourceCallsMatching(..)[0]? > > > It won't be so simple for callers, since the vector could be empty. I > think it's a nice API for the general case where we expect only one > message. > > I can implement the GetFirst* methods in terms of the GetAll* ones > though, to reduce the amount of code. > > https://codereview.chromium.org/127243002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/09 02:41:09, raymes wrote: > But the callers have to check whether true is returned from the > existing function. So it shouldn't be any more complex should it? > > On Wed, Jan 8, 2014 at 2:05 PM, <mailto:bbudge@chromium.org> wrote: > > > > > https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... > > File ppapi/proxy/resource_message_test_sink.h (right): > > > > > https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... > > ppapi/proxy/resource_message_test_sink.h:43: bool > > GetFirstResourceReplyMatching( > > On 2014/01/08 02:21:22, raymes wrote: > >> > >> Can we get rid of these two methods now? Can't we just use > >> GetAllResourceCallsMatching(..)[0]? > > > > > > It won't be so simple for callers, since the vector could be empty. I > > think it's a nice API for the general case where we expect only one > > message. > > > > I can implement the GetFirst* methods in terms of the GetAll* ones > > though, to reduce the amount of code. > > > > https://codereview.chromium.org/127243002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. The problem would be that instead of an ASSERT failing, we would be indexing into an empty array and likely causing an uncontrolled crash. Or am I missing something?
I'm think that before we would have called:
if (GetFirst...(...)) { do something; }
Now we will be calling:
std::vector<...> results = GetAll...(...);
if (results.size() > 0) { do something; }
On Thu, Jan 9, 2014 at 1:50 PM, <bbudge@chromium.org> wrote:
> On 2014/01/09 02:41:09, raymes wrote:
>>
>> But the callers have to check whether true is returned from the
>> existing function. So it shouldn't be any more complex should it?
>
>
>> On Wed, Jan 8, 2014 at 2:05 PM, <mailto:bbudge@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message...
>>
>> > File ppapi/proxy/resource_message_test_sink.h (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message...
>>
>> > ppapi/proxy/resource_message_test_sink.h:43: bool
>> > GetFirstResourceReplyMatching(
>> > On 2014/01/08 02:21:22, raymes wrote:
>> >>
>> >> Can we get rid of these two methods now? Can't we just use
>> >> GetAllResourceCallsMatching(..)[0]?
>> >
>> >
>> > It won't be so simple for callers, since the vector could be empty. I
>> > think it's a nice API for the general case where we expect only one
>> > message.
>> >
>> > I can implement the GetFirst* methods in terms of the GetAll* ones
>> > though, to reduce the amount of code.
>> >
>> > https://codereview.chromium.org/127243002/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> The problem would be that instead of an ASSERT failing, we would be indexing
> into an empty array and likely causing an uncontrolled crash. Or am I
> missing
> something?
>
> https://codereview.chromium.org/127243002/
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/09 02:57:39, raymes wrote:
> I'm think that before we would have called:
> if (GetFirst...(...)) { do something; }
>
> Now we will be calling:
> std::vector<...> results = GetAll...(...);
> if (results.size() > 0) { do something; }
>
> On Thu, Jan 9, 2014 at 1:50 PM, <mailto:bbudge@chromium.org> wrote:
> > On 2014/01/09 02:41:09, raymes wrote:
> >>
> >> But the callers have to check whether true is returned from the
> >> existing function. So it shouldn't be any more complex should it?
> >
> >
> >> On Wed, Jan 8, 2014 at 2:05 PM, <mailto:bbudge@chromium.org> wrote:
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message...
> >>
> >> > File ppapi/proxy/resource_message_test_sink.h (right):
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message...
> >>
> >> > ppapi/proxy/resource_message_test_sink.h:43: bool
> >> > GetFirstResourceReplyMatching(
> >> > On 2014/01/08 02:21:22, raymes wrote:
> >> >>
> >> >> Can we get rid of these two methods now? Can't we just use
> >> >> GetAllResourceCallsMatching(..)[0]?
> >> >
> >> >
> >> > It won't be so simple for callers, since the vector could be empty. I
> >> > think it's a nice API for the general case where we expect only one
> >> > message.
> >> >
> >> > I can implement the GetFirst* methods in terms of the GetAll* ones
> >> > though, to reduce the amount of code.
> >> >
> >> > https://codereview.chromium.org/127243002/
> >
> >
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >
> > email
> >>
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> > The problem would be that instead of an ASSERT failing, we would be indexing
> > into an empty array and likely causing an uncontrolled crash. Or am I
> > missing
> > something?
> >
> > https://codereview.chromium.org/127243002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
I see many call sites like this one:
ASSERT_TRUE(sink().GetFirstResourceCallMatching(id, ¶ms, &msg));
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/talk_r...
That would have to become:
std::vector<...> results = GetAll...(...);
ASSERT_LT(0u, results.size());
params = results[0].first;
msg = results[0].second;
...
lgtm I still think having the single function makes for a better interface, but not strongly enough to keep pushing back, especially being test-only code. :) On Thu, Jan 9, 2014 at 2:14 PM, <bbudge@chromium.org> wrote: > On 2014/01/09 02:57:39, raymes wrote: >> >> I'm think that before we would have called: >> if (GetFirst...(...)) { do something; } > > >> Now we will be calling: >> std::vector<...> results = GetAll...(...); >> if (results.size() > 0) { do something; } > > >> On Thu, Jan 9, 2014 at 1:50 PM, <mailto:bbudge@chromium.org> wrote: >> > On 2014/01/09 02:41:09, raymes wrote: >> >> >> >> But the callers have to check whether true is returned from the >> >> existing function. So it shouldn't be any more complex should it? >> > >> > >> >> On Wed, Jan 8, 2014 at 2:05 PM, <mailto:bbudge@chromium.org> wrote: >> >> > >> >> > >> > >> > >> > > > > https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... >> >> >> >> >> > File ppapi/proxy/resource_message_test_sink.h (right): >> >> > >> >> > >> > >> > >> > > > > https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message... >> >> >> >> >> > ppapi/proxy/resource_message_test_sink.h:43: bool >> >> > GetFirstResourceReplyMatching( >> >> > On 2014/01/08 02:21:22, raymes wrote: >> >> >> >> >> >> Can we get rid of these two methods now? Can't we just use >> >> >> GetAllResourceCallsMatching(..)[0]? >> >> > >> >> > >> >> > It won't be so simple for callers, since the vector could be empty. I >> >> > think it's a nice API for the general case where we expect only one >> >> > message. >> >> > >> >> > I can implement the GetFirst* methods in terms of the GetAll* ones >> >> > though, to reduce the amount of code. >> >> > >> >> > https://codereview.chromium.org/127243002/ >> > >> > >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > >> > email >> >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > >> > >> > The problem would be that instead of an ASSERT failing, we would be >> > indexing >> > into an empty array and likely causing an uncontrolled crash. Or am I >> > missing >> > something? >> > >> > https://codereview.chromium.org/127243002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > I see many call sites like this one: > > ASSERT_TRUE(sink().GetFirstResourceCallMatching(id, ¶ms, &msg)); > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/talk_r... > > That would have to become: > > > std::vector<...> results = GetAll...(...); > ASSERT_LT(0u, results.size()); > params = results[0].first; > msg = results[0].second; > ... > > https://codereview.chromium.org/127243002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/01/09 03:20:09, raymes wrote:
> lgtm
>
> I still think having the single function makes for a better interface,
> but not strongly enough to keep pushing back, especially being
> test-only code. :)
>
> On Thu, Jan 9, 2014 at 2:14 PM, <mailto:bbudge@chromium.org> wrote:
> > On 2014/01/09 02:57:39, raymes wrote:
> >>
> >> I'm think that before we would have called:
> >> if (GetFirst...(...)) { do something; }
> >
> >
> >> Now we will be calling:
> >> std::vector<...> results = GetAll...(...);
> >> if (results.size() > 0) { do something; }
> >
> >
> >> On Thu, Jan 9, 2014 at 1:50 PM, <mailto:bbudge@chromium.org> wrote:
> >> > On 2014/01/09 02:41:09, raymes wrote:
> >> >>
> >> >> But the callers have to check whether true is returned from the
> >> >> existing function. So it shouldn't be any more complex should it?
> >> >
> >> >
> >> >> On Wed, Jan 8, 2014 at 2:05 PM, <mailto:bbudge@chromium.org> wrote:
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message...
> >>
> >> >>
> >> >> > File ppapi/proxy/resource_message_test_sink.h (right):
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/127243002/diff/1/ppapi/proxy/resource_message...
> >>
> >> >>
> >> >> > ppapi/proxy/resource_message_test_sink.h:43: bool
> >> >> > GetFirstResourceReplyMatching(
> >> >> > On 2014/01/08 02:21:22, raymes wrote:
> >> >> >>
> >> >> >> Can we get rid of these two methods now? Can't we just use
> >> >> >> GetAllResourceCallsMatching(..)[0]?
> >> >> >
> >> >> >
> >> >> > It won't be so simple for callers, since the vector could be empty. I
> >> >> > think it's a nice API for the general case where we expect only one
> >> >> > message.
> >> >> >
> >> >> > I can implement the GetFirst* methods in terms of the GetAll* ones
> >> >> > though, to reduce the amount of code.
> >> >> >
> >> >> > https://codereview.chromium.org/127243002/
> >> >
> >> >
> >> >> To unsubscribe from this group and stop receiving emails from it, send
> >> >> an
> >> >
> >> > email
> >> >>
> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >> >
> >> >
> >> > The problem would be that instead of an ASSERT failing, we would be
> >> > indexing
> >> > into an empty array and likely causing an uncontrolled crash. Or am I
> >> > missing
> >> > something?
> >> >
> >> > https://codereview.chromium.org/127243002/
> >
> >
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >
> > email
> >>
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> > I see many call sites like this one:
> >
> > ASSERT_TRUE(sink().GetFirstResourceCallMatching(id, ¶ms, &msg));
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/talk_r...
> >
> > That would have to become:
> >
> >
> > std::vector<...> results = GetAll...(...);
> > ASSERT_LT(0u, results.size());
> > params = results[0].first;
> > msg = results[0].second;
> > ...
> >
> > https://codereview.chromium.org/127243002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
The GetFirst* API is awfully convenient, and consistent with the base class,
IPC::TestSink, which has a GetFirstMessageMatching method.
Thanks for all the reviews.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/127243002/130001
Message was sent while issue was closed.
Change committed as 243907 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
