DesktopRect::UnionWith() to extend current rect to cover the input rect

DesktopRect::UnionWith() to extend current rect to cover the input rect

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541
Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed: https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8eb3d346f8b64049

[Hzj_jie, 2017-04-27 21:49:29]

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

[commit-bot: I haz the power, 2017-04-27 21:49:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-27 21:54:49]

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

[commit-bot: I haz the power, 2017-04-27 21:54:50]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16518)

[Hzj_jie, 2017-04-28 23:53:58]

Description was changed from

==========
DesktopRect::JoinWith()

BUG=
==========

to

==========
Add a DesktopRect::JoinWith() function to extend current instance to cover both
instances

This small piece of logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile, the
implementation in desktop_configuration.mm is not safe. So I think a function in
DesktopRect to cover the requirement could be more efficient and safer.

I struggle with the name of this function: it's indeed not the "Join" of two
rectangles. But I cannot find a better choice. Considering we have a JoinRects()
function in desktop_configuration.mm, JoinWith() is still a choice. But I am
happy with other names.

BUG=webrtc:7541
==========

[Hzj_jie, 2017-04-28 23:53:58]

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

[jamiewalch, 2017-04-29 00:44:22]

I think "Union" would be suitable a name for this. Technically, the union of two
rectangles is not typically a rectangle, but given that it operates on a
rectangle, I think the semantics will be pretty clear if you use that name. If
you're uncomfortable with that, "BoundingRectangle" is a more verbose option.

[Hzj_jie, 2017-05-02 18:06:52]

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

[commit-bot: I haz the power, 2017-05-02 18:07:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-02 18:26:12]

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

[commit-bot: I haz the power, 2017-05-02 18:26:13]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-05-02 18:49:55]

On 2017/04/29 00:44:22, jamiewalch wrote:
> I think "Union" would be suitable a name for this. Technically, the union of
two
> rectangles is not typically a rectangle, but given that it operates on a
> rectangle, I think the semantics will be pretty clear if you use that name. If
> you're uncomfortable with that, "BoundingRectangle" is a more verbose option.

Sorry, I have lost tracking this change. Updated.

[jamiewalch, 2017-05-02 22:18:11]

jamiewalch@google.com changed reviewers:
+ jamiewalch@google.com

[jamiewalch, 2017-05-02 22:18:12]

lgtm

https://codereview.webrtc.org/2845213002/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_geometry.h (right):

https://codereview.webrtc.org/2845213002/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.h:142: // replaces |this| with
|rect|.
This is not symmetric w.r.t |this| and |rect|. Specifically, if |rect| is empty,
|this| will be extended to include it. This might be the behaviour you want, but
I think it's worth calling it out explicitly one way or the other here.

Also, since this method is complementary to IntersectionWith(), maybe move it to
immediately after that method in both header and implementation.

[Hzj_jie, 2017-05-02 22:42:32]

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

[commit-bot: I haz the power, 2017-05-02 22:42:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-02 23:01:33]

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

[commit-bot: I haz the power, 2017-05-02 23:01:34]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-05-02 23:28:28]

https://codereview.webrtc.org/2845213002/diff/20001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_geometry.h (right):

https://codereview.webrtc.org/2845213002/diff/20001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.h:142: // replaces |this| with
|rect|.
On 2017/05/02 22:18:11, jamiewalch wrote:
> This is not symmetric w.r.t |this| and |rect|. Specifically, if |rect| is
empty,
> |this| will be extended to include it. This might be the behaviour you want,
but
> I think it's worth calling it out explicitly one way or the other here.
> 
> Also, since this method is complementary to IntersectionWith(), maybe move it
to
> immediately after that method in both header and implementation.

A good point, I failed to notice it. I believe a correct behavior is to ignore
|rect| if it's empty. In DirectX capturer, the |rect| won't be empty. In
desktop_configuration.mm, I do not believe believe pixel_bounds may be empty,
otherwise we would have bug for a long time.

[Hzj_jie, 2017-05-02 23:28:33]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2017-05-02 23:28:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@google.com
Link to the patchset: https://codereview.webrtc.org/2845213002/#ps40001 (title:
"Resolve review comments")

[commit-bot: I haz the power, 2017-05-02 23:28:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-02 23:28:41]

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

[commit-bot: I haz the power, 2017-05-02 23:28:42]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-webrtc-committers".
Note that this has nothing to do with OWNERS files.

[Hzj_jie, 2017-05-02 23:32:03]

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

[Hzj_jie, 2017-05-02 23:33:03]

Sergey is OoO during these two weeks. Since this is a tiny and safe change, I
will LGTM for myself.

[Hzj_jie, 2017-05-02 23:33:29]

On 2017/05/02 23:33:03, Hzj_jie wrote:
> Sergey is OoO during these two weeks. Since this is a tiny and safe change, I
> will LGTM for myself.

lgtm

[Hzj_jie, 2017-05-02 23:33:51]

On 2017/05/02 23:33:29, Hzj_jie wrote:
> On 2017/05/02 23:33:03, Hzj_jie wrote:
> > Sergey is OoO during these two weeks. Since this is a tiny and safe change,
I
> > will LGTM for myself.
> 
> lgtm

lgtm

[Hzj_jie, 2017-05-02 23:33:59]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2017-05-02 23:34:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

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

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

[commit-bot: I haz the power, 2017-05-02 23:34:10]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-webrtc-committers".
Note that this has nothing to do with OWNERS files.

[Hzj_jie, 2017-05-02 23:34:28]

zijiehe@chromium.org changed reviewers:
- zijiehe@chromium.org

[Hzj_jie, 2017-05-02 23:37:14]

It looks like the "Add myself as reviewer" option box does not work as my
expectation. I still cannot submit this change without lgtm from Sergey.

[jamiewalch, 2017-05-02 23:38:34]

jamiewalch@google.com changed reviewers:
+ wez@chromium.org

[jamiewalch, 2017-05-02 23:39:08]

On 2017/05/02 23:37:14, Hzj_jie wrote:
> It looks like the "Add myself as reviewer" option box does not work as my
> expectation. I still cannot submit this change without lgtm from Sergey.

You just need someone in OWNERS. I added Wez since Sergey is OOO and it's a
simple change.

[Sergey Ulanov, 2017-05-16 19:53:00]

Please update the description - it still mentions JoinWith(). 

LGTM once my comments are addressed

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_geometry.cc (right):

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.cc:42: left_ = rect.left();
*this = rect;

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.cc:46: } else if
(!rect.is_empty()) {
I'd prefer to format this function as follows:

  if (is_empty()) {
    *this = rect;
    return;
  }

  if (rect.is_empty())
    return;  

  left_ = std::min(left(), rect.left());
  top_ = std::min(top(), rect.top());
  right_ = std::max(right(), rect.right());
  bottom_ = std::max(bottom(), rect.bottom());

This makes the default case obvious and separates it from the cases when one of
the rects is empty

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_geometry.h (right):

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.h:127: // Finds the minimum area
to cover both |this| and |rect|. If |this| is empty,
It's not clear from this comment that the result is stored in the same object.
Maybe reword the first sentence as "Extends the rectangle to cover |rect|."

[Hzj_jie, 2017-05-16 20:34:50]

Description was changed from

==========
Add a DesktopRect::JoinWith() function to extend current instance to cover both
instances

This small piece of logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile, the
implementation in desktop_configuration.mm is not safe. So I think a function in
DesktopRect to cover the requirement could be more efficient and safer.

I struggle with the name of this function: it's indeed not the "Join" of two
rectangles. But I cannot find a better choice. Considering we have a JoinRects()
function in desktop_configuration.mm, JoinWith() is still a choice. But I am
happy with other names.

BUG=webrtc:7541
==========

to

==========
Add a DesktopRect::UnionWith() function to extend current instance to cover both
instances

This small piece of logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile, the
implementation in desktop_configuration.mm is not safe. So I think a function in
DesktopRect to cover the requirement could be more efficient and safer.

BUG=webrtc:7541
==========

[Hzj_jie, 2017-05-16 20:37:37]

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

[commit-bot: I haz the power, 2017-05-16 20:37:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-05-16 20:49:29]

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

[commit-bot: I haz the power, 2017-05-16 20:49:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-05-16 23:04:19]

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_geometry.cc (right):

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.cc:42: left_ = rect.left();
On 2017/05/16 19:53:00, Sergey Ulanov wrote:
> *this = rect;

Done.

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.cc:46: } else if
(!rect.is_empty()) {
On 2017/05/16 19:53:00, Sergey Ulanov wrote:
> I'd prefer to format this function as follows:
> 
>   if (is_empty()) {
>     *this = rect;
>     return;
>   }
> 
>   if (rect.is_empty())
>     return;  
> 
>   left_ = std::min(left(), rect.left());
>   top_ = std::min(top(), rect.top());
>   right_ = std::max(right(), rect.right());
>   bottom_ = std::max(bottom(), rect.bottom());
> 
> This makes the default case obvious and separates it from the cases when one
of
> the rects is empty

Done.

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/desktop_geometry.h (right):

https://codereview.webrtc.org/2845213002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/desktop_geometry.h:127: // Finds the minimum area
to cover both |this| and |rect|. If |this| is empty,
On 2017/05/16 19:53:00, Sergey Ulanov wrote:
> It's not clear from this comment that the result is stored in the same object.
> Maybe reword the first sentence as "Extends the rectangle to cover |rect|."

Done.

[commit-bot: I haz the power, 2017-05-16 23:16:32]

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

[commit-bot: I haz the power, 2017-05-16 23:16:34]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-05-16 23:47:29]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2017-05-16 23:47:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, jamiewalch@google.com
Link to the patchset: https://codereview.webrtc.org/2845213002/#ps80001 (title:
"Sync latest changes")

[commit-bot: I haz the power, 2017-05-16 23:47:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-16 23:50:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494978449829480,
"parent_rev": "9756238084707787f735e1294e896e462e459717", "commit_rev":
"73c1234f6aee9ec75074348b8eb3d346f8b64049"}

[commit-bot: I haz the power, 2017-05-16 23:50:37]

Description was changed from

==========
Add a DesktopRect::UnionWith() function to extend current instance to cover both
instances

This small piece of logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile, the
implementation in desktop_configuration.mm is not safe. So I think a function in
DesktopRect to cover the requirement could be more efficient and safer.

BUG=webrtc:7541
==========

to

==========
Add a DesktopRect::UnionWith() function to extend current instance to cover both
instances

This small piece of logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile, the
implementation in desktop_configuration.mm is not safe. So I think a function in
DesktopRect to cover the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

[commit-bot: I haz the power, 2017-05-16 23:50:39]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...

[Wez, 2017-05-17 13:16:51]

BTW: nit about the CL description, for future reference:

Short description is "Add a DesktopRect::UnionWith() function to extend current
instance to cover both instances" - it isn't clear from context what you mean by
"instance", and you use the term twice, without explanation.

Similarly, the "detailed" description doesn't add any actually detail beyond
mentioning that two code files duplicate "the logic" - there's no description of
_which_ logic.

In general the CL description provides any additional context for a change that
wouldn't make sense as a comment on the actual code (e.g. if we change the code
from mechanism A to mechanism B, the code might simply explain how B works, and
the CL description might clarify why we're changing it)

[zijiehe, 2017-05-17 17:10:10]

Description was changed from

==========
Add a DesktopRect::UnionWith() function to extend current instance to cover both
instances

This small piece of logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile, the
implementation in desktop_configuration.mm is not safe. So I think a function in
DesktopRect to cover the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

to

==========
Add a DesktopRect::UnionWith() function to extend current rectangle to cover the
input rectangle

The UnionWith() logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile the
implementation in desktop_configuration.mm is not safe: if the input rectangle
is empty, it will extend current rectangle to cover (0, 0). So a function in
DesktopRect to cover the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

[zijiehe, 2017-05-17 17:11:59]

Description was changed from

==========
Add a DesktopRect::UnionWith() function to extend current rectangle to cover the
input rectangle

The UnionWith() logic is duplicated in DxgiDuplicatorController,
DxgiAdapterDuplicator and desktop_configuration.mm. Meanwhile the
implementation in desktop_configuration.mm is not safe: if the input rectangle
is empty, it will extend current rectangle to cover (0, 0). So a function in
DesktopRect to cover the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

to

==========
Add a DesktopRect::UnionWith() function to extend current rectangle to cover the
input rectangle

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if the either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

[Hzj_jie, 2017-05-17 17:13:42]

Thank you Wiz for the feedback. I have updated the description according to your
suggestion. Is it better?

[Hzj_jie, 2017-05-17 17:23:13]

Description was changed from

==========
Add a DesktopRect::UnionWith() function to extend current rectangle to cover the
input rectangle

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if the either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

to

==========
Add a DesktopRect::UnionWith() function to extend current rectangle to cover the
input rectangle

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

[Sergey Ulanov, 2017-05-17 17:27:35]

On 2017/05/17 17:13:42, Hzj_jie wrote:
> Thank you Wiz for the feedback. I have updated the description according to
your
> suggestion. Is it better?

Thank you. You could also make the summary line shorter (e.g. it could be "Add a
DesktopRect::UnionWith()" for this CL).
There is no hard limit. This document says it should be below 50 characters:
https://chris.beams.io/posts/git-commit/  (linked from
https://www.chromium.org/developers/contributing-code). This may be too strict,
summaries in chromium are often longer. But it's best to keep it shorter than 
description lines, i.e. 72 chars

[Hzj_jie, 2017-05-17 17:32:11]

Description was changed from

==========
Add a DesktopRect::UnionWith() function to extend current rectangle to cover the
input rectangle

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

to

==========
Add a DesktopRect::UnionWith() to extend current rect to cover the input rect

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

[Hzj_jie, 2017-05-17 17:33:24]

Description was changed from

==========
Add a DesktopRect::UnionWith() to extend current rect to cover the input rect

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

to

==========
DesktopRect::UnionWith() to extend current rect to cover the input rect

The logic to calculate a smallest rectangle to cover two rectangles is
duplicated in DxgiDuplicatorController, DxgiAdapterDuplicator and
desktop_configuration.mm. Meanwhile the implementation in
desktop_configuration.mm is not safe: if either rectangle is empty, it will
extend current rectangle to cover (0, 0). So a function in DesktopRect to cover
the requirement could be more efficient and safer.

BUG=webrtc:7541

Review-Url: https://codereview.webrtc.org/2845213002
Cr-Commit-Position: refs/heads/master@{#18171}
Committed:
https://chromium.googlesource.com/external/webrtc/+/73c1234f6aee9ec75074348b8...
==========

[Hzj_jie, 2017-05-17 17:35:34]

On 2017/05/17 17:27:35, Sergey Ulanov wrote:
> On 2017/05/17 17:13:42, Hzj_jie wrote:
> > Thank you Wiz for the feedback. I have updated the description according to
> your
> > suggestion. Is it better?
> 
> Thank you. You could also make the summary line shorter (e.g. it could be "Add
a
> DesktopRect::UnionWith()" for this CL).
> There is no hard limit. This document says it should be below 50 characters:
> https://chris.beams.io/posts/git-commit/  (linked from
> https://www.chromium.org/developers/contributing-code). This may be too
strict,
> summaries in chromium are often longer. But it's best to keep it shorter than 
> description lines, i.e. 72 chars

I still prefer to keep a little bit detail, i.e. the functionality of the newly
added function. As Jamie and I discussed in the code review, typically
Rectangle.UnionWith() should generate a DesktopRegion but not a DesktopRect.
Without a little bit more description, it may be a little bit confusing.

[Do not use (sergeyu), 2017-05-17 17:38:35]

On 2017/05/17 17:35:34, Hzj_jie wrote:
> On 2017/05/17 17:27:35, Sergey Ulanov wrote:
> > On 2017/05/17 17:13:42, Hzj_jie wrote:
> > > Thank you Wiz for the feedback. I have updated the description according
to
> > your
> > > suggestion. Is it better?
> > 
> > Thank you. You could also make the summary line shorter (e.g. it could be
"Add
> a
> > DesktopRect::UnionWith()" for this CL).
> > There is no hard limit. This document says it should be below 50 characters:
> > https://chris.beams.io/posts/git-commit/  (linked from
> > https://www.chromium.org/developers/contributing-code). This may be too
> strict,
> > summaries in chromium are often longer. But it's best to keep it shorter
than 
> > description lines, i.e. 72 chars
> 
> I still prefer to keep a little bit detail, i.e. the functionality of the
newly
> added function. As Jamie and I discussed in the code review, typically
> Rectangle.UnionWith() should generate a DesktopRegion but not a DesktopRect.
> Without a little bit more description, it may be a little bit confusing.

Yes you should put these details in the CL description, but they don't need to
be in the summary line (i.e. the first line). It's almost never possible to put
all relevant details in one line and it's not what summary line is for.