Fix FakePortAllocator to work with standard ICE.

Fix FakePortAllocator to work with standard ICE.

Moved flags initialization from FakePortAllocatorSession into FakePortAllocator,
so that they are passed to the PortAllocatorSession ctor rather than being set via
set_flags() in the FakePortAllocatorSession ctor. This is required because some
flags (specifically PORTALLOCATOR_ENABLE_SHARED_UFRAG) are ignored if applied to
PortAllocatorSession via set_flags().

Committed: https://crrev.com/6805fb18758a97b11b044c52feb885fedb8f69d2
Cr-Commit-Position: refs/heads/master@{#330875}

[Sergey Ulanov, 2015-05-19 22:00:03]

sergeyu@chromium.org changed reviewers:
+ wez@chromium.org

[Sergey Ulanov, 2015-05-19 22:00:04]

This should fix remoting_perftests

[Wez, 2015-05-20 18:32:37]

Can you clean up the description to be more concise? The first sentence is very
confusing. I think you're basically saying that BasicPortAllocatorSession picks
up a default set of flags and passes then to PortAllocatorSession, which just
ignores them, but I'm not sure?

[Wez, 2015-05-20 18:34:25]

On 2015/05/20 18:32:37, Wez wrote:
> Can you clean up the description to be more concise? The first sentence is
very
> confusing. I think you're basically saying that BasicPortAllocatorSession
picks
> up a default set of flags and passes then to PortAllocatorSession, which just
> ignores them, but I'm not sure?

Yeah, based on the CL description it's not clear to me why setting the flags in
the session ctor differs in behaviour from setting them on the factory - surely
setting the flags should always have the same effect, or we shouldn't let them
be set after the ctor...?

[Sergey Ulanov, 2015-05-20 19:19:56]

On 2015/05/20 18:34:25, Wez wrote:
> On 2015/05/20 18:32:37, Wez wrote:
> > Can you clean up the description to be more concise? The first sentence is
> very
> > confusing. I think you're basically saying that BasicPortAllocatorSession
> picks
> > up a default set of flags and passes then to PortAllocatorSession, which
just
> > ignores them, but I'm not sure?

BasicPortAllocatorSession takes the flags from the PortAllocator (which acts as
PortAllocatorSession factory) and passes them to PortAllocatorSession, but this
code was never setting flags for PortAllocator.

> 
> Yeah, based on the CL description it's not clear to me why setting the flags
in
> the session ctor differs in behaviour from setting them on the factory -
surely
> setting the flags should always have the same effect, or we shouldn't let them
> be set after the ctor...?

Right. PortAllocatorSession has flags parameter in the constructor and also
set_flags() method. PORTALLOCATOR_ENABLE_SHARED_UFRAG must be passed to the
constructor and has no effect when passed to set_flags().
PortAllocatorSession::set_flags() can and should be removed - I'll make a
separate CL for that.

[Wez, 2015-05-20 21:32:25]

On 2015/05/20 19:19:56, Sergey Ulanov wrote:
> On 2015/05/20 18:34:25, Wez wrote:
> > On 2015/05/20 18:32:37, Wez wrote:
> > > Can you clean up the description to be more concise? The first sentence is
> > very
> > > confusing. I think you're basically saying that BasicPortAllocatorSession
> > picks
> > > up a default set of flags and passes then to PortAllocatorSession, which
> just
> > > ignores them, but I'm not sure?
> 
> BasicPortAllocatorSession takes the flags from the PortAllocator (which acts
as
> PortAllocatorSession factory) and passes them to PortAllocatorSession, but
this
> code was never setting flags for PortAllocator.
> 
> > 
> > Yeah, based on the CL description it's not clear to me why setting the flags
> in
> > the session ctor differs in behaviour from setting them on the factory -
> surely
> > setting the flags should always have the same effect, or we shouldn't let
them
> > be set after the ctor...?
> 
> Right. PortAllocatorSession has flags parameter in the constructor and also
> set_flags() method. PORTALLOCATOR_ENABLE_SHARED_UFRAG must be passed to the
> constructor and has no effect when passed to set_flags().
> PortAllocatorSession::set_flags() can and should be removed - I'll make a
> separate CL for that.

OK - can you update the CL description to be clearer?  Basically just saying
that UFRAG option needs to be applied at construction time, which means we need
to set the flags on the factory, and that by the way that will be the only way
to set flags, in future.

[Sergey Ulanov, 2015-05-20 21:39:02]

On 2015/05/20 21:32:25, Wez wrote:
> OK - can you update the CL description to be clearer?  Basically just saying
> that UFRAG option needs to be applied at construction time, which means we
need
> to set the flags on the factory, and that by the way that will be the only way
> to set flags, in future.

I did updated it. Is it still not clear enough?

[Wez, 2015-05-20 22:29:59]

On 2015/05/20 21:39:02, Sergey Ulanov wrote:
> On 2015/05/20 21:32:25, Wez wrote:
> > OK - can you update the CL description to be clearer?  Basically just saying
> > that UFRAG option needs to be applied at construction time, which means we
> need
> > to set the flags on the factory, and that by the way that will be the only
way
> > to set flags, in future.
> 
> I did updated it. Is it still not clear enough?

The first two sentences are confusing; the first provides detail that seems
irrelevant - basically this can be expressed much more concisely, e.g: "Move
initialization of PortAllocatorSession flags into FakePortAllocator, so that
they are passed to the PortAllocatorSession ctor rather than being set via
set_flags() in the FakePortAllocatorSession ctor. This is required because some
flags (specifically PORTALLOCATOR_ENABLE_SHARED_UFRAG) are ignored if applied to
PortAllocatorSession via set_flags()."

[Sergey Ulanov, 2015-05-21 00:50:48]

On 2015/05/20 22:29:59, Wez wrote:
> On 2015/05/20 21:39:02, Sergey Ulanov wrote:
> > On 2015/05/20 21:32:25, Wez wrote:
> > > OK - can you update the CL description to be clearer?  Basically just
saying
> > > that UFRAG option needs to be applied at construction time, which means we
> > need
> > > to set the flags on the factory, and that by the way that will be the only
> way
> > > to set flags, in future.
> > 
> > I did updated it. Is it still not clear enough?
> 
> The first two sentences are confusing; the first provides detail that seems
> irrelevant - basically this can be expressed much more concisely, e.g: "Move
> initialization of PortAllocatorSession flags into FakePortAllocator, so that
> they are passed to the PortAllocatorSession ctor rather than being set via
> set_flags() in the FakePortAllocatorSession ctor. This is required because
some
> flags (specifically PORTALLOCATOR_ENABLE_SHARED_UFRAG) are ignored if applied
to
> PortAllocatorSession via set_flags()."

Done, reworded the first sentence slightly - technically these are "port
allocator flags" and not "port allocator session flags".

[Wez, 2015-05-21 00:51:34]

The CQ bit was checked by wez@chromium.org

[Wez, 2015-05-21 00:51:35]

lgtm

[commit-bot: I haz the power, 2015-05-21 00:52:26]

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

[commit-bot: I haz the power, 2015-05-21 01:59:08]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-21 02:00:00]

Patchset 1 (id:??) landed as
https://crrev.com/6805fb18758a97b11b044c52feb885fedb8f69d2
Cr-Commit-Position: refs/heads/master@{#330875}