[Chromoting] Use HostExperimentSessionPlugin in host

[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

BUG=650926
Review-Url: https://codereview.chromium.org/2615113005
Cr-Commit-Position: refs/heads/master@{#448852}
Committed: https://chromium.googlesource.com/chromium/src/+/f38c9720e162e14c8a8fb2f492cbae99fd525ae3

[Hzj_jie, 2017-01-06 20:51:55]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 20:52:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 20:59:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 20:59:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hzj_jie, 2017-01-06 22:58:24]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2017-01-06 22:58:45]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-06 22:58:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 23:07:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 23:07:52]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hzj_jie, 2017-01-06 23:49:21]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 23:49:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-01-07 00:07:49]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-07 00:16:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-07 00:16:43]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-01-07 00:36:07]

Description was changed from

==========
[Chromoting] Use HostExperimentSessionPlugin in host

BUG=650926
==========

to

==========
[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

BUG=650926
==========

[Hzj_jie, 2017-01-07 00:36:07]

zijiehe@chromium.org changed reviewers:
+ jamiewalch@chromium.org, sergeyu@chromium.org

[Hzj_jie, 2017-01-07 00:37:49]

Description was changed from

==========
[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

BUG=650926
==========

to

==========
[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

~50% of this change is for test.

BUG=650926
==========

[Sergey Ulanov, 2017-01-07 01:23:09]

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/client_se...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/client_se...
remoting/host/client_session_unittest.cc:441:
std::unique_ptr<protocol::MockSession> session(new protocol::MockSession());
nit: This line may would be more readable if written as follows:
 auto session = base::MakeUnique<protocol::MockSession>();

Same for the line below.

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/client_se...
remoting/host/client_session_unittest.cc:449:
->options().desktop_capture_options()->allow_directx_capturer());
'git cl format' please

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/host_sess...
File remoting/host/host_session_options.h (right):

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/host_sess...
remoting/host/host_session_options.h:5: #ifndef
REMOTING_PROTOCOL_HOST_SESSION_OPTIONS_H_
s/PROTOCOL/HOST/

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/host_sess...
remoting/host/host_session_options.h:19: class HostSessionOptions final {
Can this class be used on the client side as well?
Then maybe it should be in protocol?

https://codereview.chromium.org/2615113005/diff/40001/remoting/protocol/proto...
File remoting/protocol/protocol_mock_objects.h (right):

https://codereview.chromium.org/2615113005/diff/40001/remoting/protocol/proto...
remoting/protocol/protocol_mock_objects.h:214: void SetAttachment(size_t round,
It's better to avoid mixing non-GMock code into Mock classes. I suggest updating
client_session_unittest.cc to use FakeSession instead of MockSession and
implement this feature in FakeSession.

I think we should get rid of MockSession and use FakeSession everywhere (not in
this CL obviously). That would make tests cleaner.
Beside client_session_unittest.cc MockSession is used only in
chromoting_host_unittest.cc.

[Hzj_jie, 2017-01-08 22:56:19]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-08 22:56:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-08 23:45:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-08 23:45:40]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-01-08 23:47:08]

Description was changed from

==========
[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

~50% of this change is for test.

BUG=650926
==========

to

==========
[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

BUG=650926
==========

[Hzj_jie, 2017-01-08 23:48:06]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-08 23:48:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-01-08 23:48:39]

Patchset #2 (id:60001) has been deleted

[Hzj_jie, 2017-01-08 23:48:49]

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/client_se...
File remoting/host/client_session_unittest.cc (right):

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/client_se...
remoting/host/client_session_unittest.cc:441:
std::unique_ptr<protocol::MockSession> session(new protocol::MockSession());
On 2017/01/07 01:23:09, Sergey Ulanov wrote:
> nit: This line may would be more readable if written as follows:
>  auto session = base::MakeUnique<protocol::MockSession>();
> 
> Same for the line below.

Done.

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/client_se...
remoting/host/client_session_unittest.cc:449:
->options().desktop_capture_options()->allow_directx_capturer());
On 2017/01/07 01:23:09, Sergey Ulanov wrote:
> 'git cl format' please

Done.

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/host_sess...
File remoting/host/host_session_options.h (right):

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/host_sess...
remoting/host/host_session_options.h:5: #ifndef
REMOTING_PROTOCOL_HOST_SESSION_OPTIONS_H_
On 2017/01/07 01:23:09, Sergey Ulanov wrote:
> s/PROTOCOL/HOST/

Done.

https://codereview.chromium.org/2615113005/diff/40001/remoting/host/host_sess...
remoting/host/host_session_options.h:19: class HostSessionOptions final {
On 2017/01/07 01:23:09, Sergey Ulanov wrote:
> Can this class be used on the client side as well?
> Then maybe it should be in protocol?

No, clients won't create or parse the host experiment configuration. Clients
only need to read a string from experiment description file, and pass it as
configuration to the host.

https://codereview.chromium.org/2615113005/diff/40001/remoting/protocol/proto...
File remoting/protocol/protocol_mock_objects.h (right):

https://codereview.chromium.org/2615113005/diff/40001/remoting/protocol/proto...
remoting/protocol/protocol_mock_objects.h:214: void SetAttachment(size_t round,
On 2017/01/07 01:23:09, Sergey Ulanov wrote:
> It's better to avoid mixing non-GMock code into Mock classes. I suggest
updating
> client_session_unittest.cc to use FakeSession instead of MockSession and
> implement this feature in FakeSession.
> 
> I think we should get rid of MockSession and use FakeSession everywhere (not
in
> this CL obviously). That would make tests cleaner.
> Beside client_session_unittest.cc MockSession is used only in
> chromoting_host_unittest.cc.

Done.

[commit-bot: I haz the power, 2017-01-09 00:46:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 00:46:05]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-01-09 23:00:27]

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
File remoting/host/host_experiment_servant.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
remoting/host/host_experiment_servant.h:18: class HostExperimentServant : public
protocol::SessionPlugin {
I don't think it's necessary to rename this class in this CL.
Also "Server" is more commonly used term, not "Servant", but
"HostExperimentServer" would be not a good name either because it doesn't
reflect what this class is responsible for. HostExperimentReceiver might be a
better name, but I think HostExperimentSessionPlugin works as well.

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
File remoting/protocol/fake_session.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_session.h:47: void SetAttachment(size_t round,
int would be more appropriate here than size_t, but I think it would be better
to replace this function with 
SetAttachments(std::vector<std::unique_ptr<buzz::XmlElement>> attachments)
to set all attachments at once.

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_session.h:80: std::deque<JingleMessage> attachments_;
I'd prefer to avoid using std::deque<> anywhere in new code. Depending on impl
it can be rather inefficient, see crbug.com/674287 . This is a test code, so it
doesn't make much difference here, but for consistency I would prefer to avoid
it in test code as well. This class needs only emplace_back() and operator[], so
you could use std::vector<> instead.

[Hzj_jie, 2017-01-10 00:33:41]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 00:34:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-01-10 02:31:10]

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
File remoting/host/host_experiment_servant.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
remoting/host/host_experiment_servant.h:18: class HostExperimentServant : public
protocol::SessionPlugin {
On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> I don't think it's necessary to rename this class in this CL.
> Also "Server" is more commonly used term, not "Servant", but
> "HostExperimentServer" would be not a good name either because it doesn't
> reflect what this class is responsible for. HostExperimentReceiver might be a
> better name, but I think HostExperimentSessionPlugin works as well.

No, HostExperimentServer is definitely not correct. I am OK with current name if
you do not have concern.

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
File remoting/protocol/fake_session.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_session.h:47: void SetAttachment(size_t round,
On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> int would be more appropriate here than size_t, but I think it would be better
> to replace this function with 
> SetAttachments(std::vector<std::unique_ptr<buzz::XmlElement>> attachments)
> to set all attachments at once.

Then it would be complex if the FakeSession has been sent through several
components, and each component needs to set its own SessionPlugin and
attachments.

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_session.h:80: std::deque<JingleMessage> attachments_;
On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> I'd prefer to avoid using std::deque<> anywhere in new code. Depending on impl
> it can be rather inefficient, see crbug.com/674287 . This is a test code, so
it
> doesn't make much difference here, but for consistency I would prefer to avoid
> it in test code as well. This class needs only emplace_back() and operator[],
so
> you could use std::vector<> instead.

It's a little bit complex, and relates to the implementation of vector. Since
vector creates a new array, and copy all the elements from old one when the old
array is full. If we actively provide a destructor, vector will use
copy-assignment or move-constructor, so the destructor can be correctly
involved. But copy-assignment is disabled because of the unique_ptr variables,
and we do not define a move-constructor, so it will trigger a build break. A
simpler sample is @ http://cpp.sh/6hvul.

List does not provide a random access (operator[] here), so it won't support the
scenario.

Deque does not need to move the internal array, so it does not require
copy-assignment or move-contructor.

So we can do one of the following two,
1. Change JingleMessage to remove the destructor, which is not really necessary,
or provide a move constructor by simply add JingleMessage(JingleMessage&&) =
default. Then it should be able to work with vector.
2. Use deque, since it's in test code, which does not really impact the
production performance.

[commit-bot: I haz the power, 2017-01-10 02:49:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 02:49:20]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-01-10 15:11:46]

joedow@chromium.org changed reviewers:
+ joedow@chromium.org

[joedow, 2017-01-10 15:11:46]

joedow@chromium.org changed reviewers:
+ joedow@chromium.org

[joedow, 2017-01-10 15:11:46]

joedow@chromium.org changed reviewers:
+ joedow@chromium.org

[joedow, 2017-01-10 15:11:47]

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
File remoting/host/host_experiment_servant.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
remoting/host/host_experiment_servant.h:18: class HostExperimentServant : public
protocol::SessionPlugin {
On 2017/01/10 02:31:09, Hzj_jie wrote:
> On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > I don't think it's necessary to rename this class in this CL.
> > Also "Server" is more commonly used term, not "Servant", but
> > "HostExperimentServer" would be not a good name either because it doesn't
> > reflect what this class is responsible for. HostExperimentReceiver might be
a
> > better name, but I think HostExperimentSessionPlugin works as well.
> 
> No, HostExperimentServer is definitely not correct. I am OK with current name
if
> you do not have concern.

Drive-by comment.  'Servant' is not standard nomenclature and has negative
connotations, definitely change it.

[joedow, 2017-01-10 15:11:47]

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
File remoting/host/host_experiment_servant.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
remoting/host/host_experiment_servant.h:18: class HostExperimentServant : public
protocol::SessionPlugin {
On 2017/01/10 02:31:09, Hzj_jie wrote:
> On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > I don't think it's necessary to rename this class in this CL.
> > Also "Server" is more commonly used term, not "Servant", but
> > "HostExperimentServer" would be not a good name either because it doesn't
> > reflect what this class is responsible for. HostExperimentReceiver might be
a
> > better name, but I think HostExperimentSessionPlugin works as well.
> 
> No, HostExperimentServer is definitely not correct. I am OK with current name
if
> you do not have concern.

Drive-by comment.  'Servant' is not standard nomenclature and has negative
connotations, definitely change it.

[joedow, 2017-01-10 15:11:47]

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
File remoting/host/host_experiment_servant.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
remoting/host/host_experiment_servant.h:18: class HostExperimentServant : public
protocol::SessionPlugin {
On 2017/01/10 02:31:09, Hzj_jie wrote:
> On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > I don't think it's necessary to rename this class in this CL.
> > Also "Server" is more commonly used term, not "Servant", but
> > "HostExperimentServer" would be not a good name either because it doesn't
> > reflect what this class is responsible for. HostExperimentReceiver might be
a
> > better name, but I think HostExperimentSessionPlugin works as well.
> 
> No, HostExperimentServer is definitely not correct. I am OK with current name
if
> you do not have concern.

Drive-by comment.  'Servant' is not standard nomenclature and has negative
connotations, definitely change it.

[Hzj_jie, 2017-01-10 20:32:31]

On 2017/01/10 15:11:47, joedow wrote:
>
https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
> File remoting/host/host_experiment_servant.h (right):
> 
>
https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
> remoting/host/host_experiment_servant.h:18: class HostExperimentServant :
public
> protocol::SessionPlugin {
> On 2017/01/10 02:31:09, Hzj_jie wrote:
> > On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > > I don't think it's necessary to rename this class in this CL.
> > > Also "Server" is more commonly used term, not "Servant", but
> > > "HostExperimentServer" would be not a good name either because it doesn't
> > > reflect what this class is responsible for. HostExperimentReceiver might
be
> a
> > > better name, but I think HostExperimentSessionPlugin works as well.
> > 
> > No, HostExperimentServer is definitely not correct. I am OK with current
name
> if
> > you do not have concern.
> 
> Drive-by comment.  'Servant' is not standard nomenclature and has negative
> connotations, definitely change it.

Yes, updated.

[Hzj_jie, 2017-01-10 20:32:34]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 20:33:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 21:05:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 21:05:45]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-01-13 19:54:31]

On 2017/01/10 20:32:31, Hzj_jie wrote:
> On 2017/01/10 15:11:47, joedow wrote:
> >
>
https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
> > File remoting/host/host_experiment_servant.h (right):
> > 
> >
>
https://codereview.chromium.org/2615113005/diff/80001/remoting/host/host_expe...
> > remoting/host/host_experiment_servant.h:18: class HostExperimentServant :
> public
> > protocol::SessionPlugin {
> > On 2017/01/10 02:31:09, Hzj_jie wrote:
> > > On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > > > I don't think it's necessary to rename this class in this CL.
> > > > Also "Server" is more commonly used term, not "Servant", but
> > > > "HostExperimentServer" would be not a good name either because it
doesn't
> > > > reflect what this class is responsible for. HostExperimentReceiver might
> be
> > a
> > > > better name, but I think HostExperimentSessionPlugin works as well.
> > > 
> > > No, HostExperimentServer is definitely not correct. I am OK with current
> name
> > if
> > > you do not have concern.
> > 
> > Drive-by comment.  'Servant' is not standard nomenclature and has negative
> > connotations, definitely change it.
> 
> Yes, updated.

Ping ...

[Sergey Ulanov, 2017-01-18 22:21:40]

On 2017/01/13 19:54:31, Hzj_jie wrote:
> Ping ...

Do we still needs parts of this CL related to the DX capturer experiment?

[Hzj_jie, 2017-01-21 02:07:02]

I believe so, we can have a reverse experiment to analyze the impact of DX
capturer. Currently we have only capture latency, but no other user
engagement data.

On Wed, Jan 18, 2017 at 2:21 PM, <sergeyu@chromium.org> wrote:

> On 2017/01/13 19:54:31, Hzj_jie wrote:
> > Ping ...
>
> Do we still needs parts of this CL related to the DX capturer experiment?
>
> https://codereview.chromium.org/2615113005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sergey Ulanov, 2017-01-23 00:51:36]

lgmt when my comment are addressed

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
File remoting/protocol/fake_session.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_session.h:80: std::deque<JingleMessage> attachments_;
On 2017/01/10 02:31:10, Hzj_jie wrote:
> On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > I'd prefer to avoid using std::deque<> anywhere in new code. Depending on
impl
> > it can be rather inefficient, see crbug.com/674287 . This is a test code, so
> it
> > doesn't make much difference here, but for consistency I would prefer to
avoid
> > it in test code as well. This class needs only emplace_back() and
operator[],
> so
> > you could use std::vector<> instead.
> 
> It's a little bit complex, and relates to the implementation of vector. Since
> vector creates a new array, and copy all the elements from old one when the
old
> array is full. If we actively provide a destructor, vector will use
> copy-assignment or move-constructor, so the destructor can be correctly
> involved. But copy-assignment is disabled because of the unique_ptr variables,
> and we do not define a move-constructor, so it will trigger a build break. A
> simpler sample is @ http://cpp.sh/6hvul.
> 
> List does not provide a random access (operator[] here), so it won't support
the
> scenario.
> 
> Deque does not need to move the internal array, so it does not require
> copy-assignment or move-contructor.
> 
> So we can do one of the following two,
> 1. Change JingleMessage to remove the destructor, which is not really
necessary,
> or provide a move constructor by simply add JingleMessage(JingleMessage&&) =
> default. Then it should be able to work with vector.
> 2. Use deque, since it's in test code, which does not really impact the
> production performance.

I don't think you need to store JingleMessages here. Only the attachments field
is used, so this can be std::vector<std::unique_ptr<buzz::XmlElement>> .

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/desktop_...
File remoting/host/desktop_environment_options.h (right):

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/desktop_...
remoting/host/desktop_environment_options.h:43: void Import(const
HostSessionOptions& options);
nit: maybe call it ApplyHostSessionOptions()?

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/host_ses...
File remoting/host/host_session_options.cc (right):

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/host_ses...
remoting/host/host_session_options.cc:38: : HostSessionOptions() {
Don't need to call this constructor explicitly

[Sergey Ulanov, 2017-01-23 00:52:09]

On 2017/01/23 00:51:36, Sergey Ulanov wrote:
> lgmt when my comment are addressed

I meant LGTM when my comments are addressed

> 
>
https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
> File remoting/protocol/fake_session.h (right):
> 
>
https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
> remoting/protocol/fake_session.h:80: std::deque<JingleMessage> attachments_;
> On 2017/01/10 02:31:10, Hzj_jie wrote:
> > On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > > I'd prefer to avoid using std::deque<> anywhere in new code. Depending on
> impl
> > > it can be rather inefficient, see crbug.com/674287 . This is a test code,
so
> > it
> > > doesn't make much difference here, but for consistency I would prefer to
> avoid
> > > it in test code as well. This class needs only emplace_back() and
> operator[],
> > so
> > > you could use std::vector<> instead.
> > 
> > It's a little bit complex, and relates to the implementation of vector.
Since
> > vector creates a new array, and copy all the elements from old one when the
> old
> > array is full. If we actively provide a destructor, vector will use
> > copy-assignment or move-constructor, so the destructor can be correctly
> > involved. But copy-assignment is disabled because of the unique_ptr
variables,
> > and we do not define a move-constructor, so it will trigger a build break. A
> > simpler sample is @ http://cpp.sh/6hvul.
> > 
> > List does not provide a random access (operator[] here), so it won't support
> the
> > scenario.
> > 
> > Deque does not need to move the internal array, so it does not require
> > copy-assignment or move-contructor.
> > 
> > So we can do one of the following two,
> > 1. Change JingleMessage to remove the destructor, which is not really
> necessary,
> > or provide a move constructor by simply add JingleMessage(JingleMessage&&) =
> > default. Then it should be able to work with vector.
> > 2. Use deque, since it's in test code, which does not really impact the
> > production performance.
> 
> I don't think you need to store JingleMessages here. Only the attachments
field
> is used, so this can be std::vector<std::unique_ptr<buzz::XmlElement>> .
> 
>
https://codereview.chromium.org/2615113005/diff/120001/remoting/host/desktop_...
> File remoting/host/desktop_environment_options.h (right):
> 
>
https://codereview.chromium.org/2615113005/diff/120001/remoting/host/desktop_...
> remoting/host/desktop_environment_options.h:43: void Import(const
> HostSessionOptions& options);
> nit: maybe call it ApplyHostSessionOptions()?
> 
>
https://codereview.chromium.org/2615113005/diff/120001/remoting/host/host_ses...
> File remoting/host/host_session_options.cc (right):
> 
>
https://codereview.chromium.org/2615113005/diff/120001/remoting/host/host_ses...
> remoting/host/host_session_options.cc:38: : HostSessionOptions() {
> Don't need to call this constructor explicitly

[Hzj_jie, 2017-02-07 22:07:32]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 22:08:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 22:45:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 22:45:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Hzj_jie, 2017-02-07 23:19:21]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 23:20:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-02-07 23:49:06]

Patchset #7 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-02-07 23:53:00]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Hzj_jie, 2017-02-08 00:21:00]

Patchset #5 (id:140001) has been deleted

[Hzj_jie, 2017-02-08 00:21:14]

Patchset #5 (id:160001) has been deleted

[Hzj_jie, 2017-02-08 00:21:20]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 00:21:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 00:53:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 00:53:08]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-02-08 01:56:29]

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
File remoting/protocol/fake_session.h (right):

https://codereview.chromium.org/2615113005/diff/80001/remoting/protocol/fake_...
remoting/protocol/fake_session.h:80: std::deque<JingleMessage> attachments_;
On 2017/01/23 00:51:35, Sergey Ulanov wrote:
> On 2017/01/10 02:31:10, Hzj_jie wrote:
> > On 2017/01/09 23:00:26, Sergey Ulanov wrote:
> > > I'd prefer to avoid using std::deque<> anywhere in new code. Depending on
> impl
> > > it can be rather inefficient, see crbug.com/674287 . This is a test code,
so
> > it
> > > doesn't make much difference here, but for consistency I would prefer to
> avoid
> > > it in test code as well. This class needs only emplace_back() and
> operator[],
> > so
> > > you could use std::vector<> instead.
> > 
> > It's a little bit complex, and relates to the implementation of vector.
Since
> > vector creates a new array, and copy all the elements from old one when the
> old
> > array is full. If we actively provide a destructor, vector will use
> > copy-assignment or move-constructor, so the destructor can be correctly
> > involved. But copy-assignment is disabled because of the unique_ptr
variables,
> > and we do not define a move-constructor, so it will trigger a build break. A
> > simpler sample is @ http://cpp.sh/6hvul.
> > 
> > List does not provide a random access (operator[] here), so it won't support
> the
> > scenario.
> > 
> > Deque does not need to move the internal array, so it does not require
> > copy-assignment or move-contructor.
> > 
> > So we can do one of the following two,
> > 1. Change JingleMessage to remove the destructor, which is not really
> necessary,
> > or provide a move constructor by simply add JingleMessage(JingleMessage&&) =
> > default. Then it should be able to work with vector.
> > 2. Use deque, since it's in test code, which does not really impact the
> > production performance.
> 
> I don't think you need to store JingleMessages here. Only the attachments
field
> is used, so this can be std::vector<std::unique_ptr<buzz::XmlElement>> .

Done.

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/desktop_...
File remoting/host/desktop_environment_options.h (right):

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/desktop_...
remoting/host/desktop_environment_options.h:43: void Import(const
HostSessionOptions& options);
On 2017/01/23 00:51:35, Sergey Ulanov wrote:
> nit: maybe call it ApplyHostSessionOptions()?

Done.

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/host_ses...
File remoting/host/host_session_options.cc (right):

https://codereview.chromium.org/2615113005/diff/120001/remoting/host/host_ses...
remoting/host/host_session_options.cc:38: : HostSessionOptions() {
On 2017/01/23 00:51:35, Sergey Ulanov wrote:
> Don't need to call this constructor explicitly

Done.

[Hzj_jie, 2017-02-08 01:58:06]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2017-02-08 01:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2615113005/#ps200001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2017-02-08 01:58:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 02:05:43]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486519086094190,
"parent_rev": "af445fb0a75d6811c0a8991becd79986e996d647", "commit_rev":
"f38c9720e162e14c8a8fb2f492cbae99fd525ae3"}

[commit-bot: I haz the power, 2017-02-08 02:06:36]

Description was changed from

==========
[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

BUG=650926
==========

to

==========
[Chromoting] Use HostExperimentSessionPlugin in host

This change adds HostExperimentSessionPlugin in host ClientSession class, and
attaches it to Session through ConnectionToClient. When OnAuthenticated() is
fired, the HostExperimentSessionPlugin::configuration() will be sent to
HostSesionPlugin / DesktopEnvironmentOptions, so DesktopEnvironment can use it
to create DirectX capturer.

This change is more specific for experiment of DirectX capturer.

BUG=650926

Review-Url: https://codereview.chromium.org/2615113005
Cr-Commit-Position: refs/heads/master@{#448852}
Committed:
https://chromium.googlesource.com/chromium/src/+/f38c9720e162e14c8a8fb2f492cb...
==========

[commit-bot: I haz the power, 2017-02-08 02:06:38]

Committed patchset #5 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f38c9720e162e14c8a8fb2f492cb...