[Presentation API] Limit the number of pending Start/JoinSession

[Presentation API] Limit the number of pending Start/JoinSession
requests.

For StartSession, PSImpl keeps a queue of StartSession requests to be
processed. (It can only process one at a time). If the queue has
reached maximum allowed size (10), subsequent incoming requests will
be immediately rejected.

For Joinsession, PSImpl keeps a map of currently JoinSession requests
that are currently processed. If the map reached maximum allowed size
(10), subsequent incoming requests will be immediately rejected.

Also, tried to simplify some of the cleanup flow for
NewSessionMojoCallbacks by introducing a wrapper class that ensures
callbacks are not dropped. Currently the cleanup logic is sprinkled
all over the file so hopefully this makes things cleaner.

We could generalize the callback wrapper pattern in the future if
it makes necessary.

BUG=412331
Committed: https://crrev.com/27e2b56b9acea83000624bea149bb7154e916d8b
Cr-Commit-Position: refs/heads/master@{#330203}

[imcheng (use chromium acct), 2015-05-13 01:51:37]

imcheng@google.com changed reviewers:
+ avayvod@chromium.org, mfoltz@chromium.org

[whywhat, 2015-05-13 11:11:33]

lgtm

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.cc:228: } else {
nit: consider returning after InvokeNewSessionMojoCallbackWithError and remove
the else clause for better readability.

or maybe even

if (start_session_request_id_ == kInvalidRequestSessionId) {
  DoStartSession(...);
  return;
}

if (queued_start_session_requests_.size() >= kMaxNumQueuedSessionRequests) {
  InvokeNewSessionMojoCallbackWithError(callback);
  return;
}

queued_start_session_requests_.push_back(...);

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.h:321: // Indicates that
a StartSession request is currently being processed.
nit: update comment?

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.h:330: int
next_request_session_id_;
not clear from the comment how this is used vs start_session_request_id_

[imcheng (use chromium acct), 2015-05-13 17:07:45]

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.cc:228: } else {
On 2015/05/13 11:11:32, whywhat wrote:
> nit: consider returning after InvokeNewSessionMojoCallbackWithError and remove
> the else clause for better readability.
> 
> or maybe even
> 
> if (start_session_request_id_ == kInvalidRequestSessionId) {
>   DoStartSession(...);
>   return;
> }
> 
> if (queued_start_session_requests_.size() >= kMaxNumQueuedSessionRequests) {
>   InvokeNewSessionMojoCallbackWithError(callback);
>   return;
> }
> 
> queued_start_session_requests_.push_back(...);

Done.

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.h:321: // Indicates that
a StartSession request is currently being processed.
On 2015/05/13 11:11:33, whywhat wrote:
> nit: update comment?

Done.

https://codereview.chromium.org/1141683002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.h:330: int
next_request_session_id_;
On 2015/05/13 11:11:33, whywhat wrote:
> not clear from the comment how this is used vs start_session_request_id_

Updated comment.

[imcheng (use chromium acct), 2015-05-14 17:44:54]

Ping for mark.

[mark a. foltz, 2015-05-14 23:03:02]

I don't have time before my trip to do a proper review so a few drive-by
comments.  Suggest pinging another content/ OWNER or wez@ for further comments.

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:145: // before it goes
out of scope.
Is this necessary for production code or is it just for debugging?

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:146: class
NewSessionMojoCallbackWrapper {
Can this inherit from NewSessionMojoCallback so the rest of the code doesn't
have to know about it?

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:150:
~NewSessionMojoCallbackWrapper();
virtual

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:237: 
Does everything from here on down have to be public:?
The rest seem to be implementation details.

[imcheng (use chromium acct), 2015-05-15 18:01:16]

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:145: // before it goes
out of scope.
On 2015/05/14 23:03:01, mfoltz_ooo_until_5-26 wrote:
> Is this necessary for production code or is it just for debugging?

This is an attempt to clean up the logic preventing dropped callbacks. It
replaces what we have in production code currently.

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:146: class
NewSessionMojoCallbackWrapper {
On 2015/05/14 23:03:01, mfoltz_ooo_until_5-26 wrote:
> Can this inherit from NewSessionMojoCallback so the rest of the code doesn't
> have to know about it?

It looks like inheritance would work here as well but unless there's a strong
reason, I prefer composition over inheritance.

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:150:
~NewSessionMojoCallbackWrapper();
On 2015/05/14 23:03:01, mfoltz_ooo_until_5-26 wrote:
> virtual 

Acknowledged.

https://codereview.chromium.org/1141683002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.h:237: 
On 2015/05/14 23:03:01, mfoltz_ooo_until_5-26 wrote:
> Does everything from here on down have to be public:?
> The rest seem to be implementation details.

Not sure what you mean. Everything starting from L61 is private.

[imcheng (use chromium acct), 2015-05-15 18:17:14]

imcheng@google.com changed reviewers:
+ avi@chromium.org

[imcheng (use chromium acct), 2015-05-15 18:17:14]

R += avi

Avi, since mfoltz@ is OOO, do you mind taking a look at this patch?

[Avi (use Gerrit), 2015-05-15 18:30:55]

lgtm

[imcheng (use chromium acct), 2015-05-15 18:34:44]

Thanks! submitting now.

[imcheng (use chromium acct), 2015-05-15 20:06:14]

The CQ bit was checked by imcheng@google.com

[imcheng (use chromium acct), 2015-05-15 20:06:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1141683002/#ps80001
(title: "rm unused function declaration")

[commit-bot: I haz the power, 2015-05-15 20:07:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1141683002/80001

[commit-bot: I haz the power, 2015-05-15 21:39:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-18 11:27:48]

Patchset 5 (id:??) landed as
https://crrev.com/27e2b56b9acea83000624bea149bb7154e916d8b
Cr-Commit-Position: refs/heads/master@{#330203}