Switch over to using SkRegions to calculate dirty areas.

Switch over to using SkRegions to calculate dirty areas.


BUG=91619
TEST=Set up a remoting sesssion and make sure it works.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96327

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96632

[dmac, 2011-08-05 23:29:15]

PTAL.

Thakis, I added you due to change to skia/ext. Any reason why we were picking
from within ApplicationServices?

[Nico, 2011-08-05 23:54:33]

skia change lgtm

[Wez, 2011-08-08 20:49:34]

http://codereview.chromium.org/7491070/diff/4001/remoting/base/codec_test.cc
File remoting/base/codec_test.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/base/codec_test.cc#...
remoting/base/codec_test.cc:143: decoder_->GetUpdatedRects(&update_rects_);
Shouldn't this change to GetUpdatedRegion()?

http://codereview.chromium.org/7491070/diff/4001/remoting/base/codec_test.cc#...
remoting/base/codec_test.cc:242: uint8** memory) {
nit: Making this a scoped_array<uint8>* seems cleaner.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer.h
File remoting/host/capturer.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer.h#new...
remoting/host/capturer.h:58: virtual void InvalidateRegion(const SkRegion&
inval_region) = 0;
nit: This should be |region|, |dirty_region| or |invalid_region|, to fit the
style guide.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer.h#new...
remoting/host/capturer.h:72: // call the |callback| routine.
nit: These two sentences could be combined, i.e. "... even if the dirty region
is empty".

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_fake.h
File remoting/host/capturer_fake.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_fake....
remoting/host/capturer_fake.h:27: virtual void InvalidateRegion(const SkRegion&
inval_region) OVERRIDE;
nit: Parameter name.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
File remoting/host/capturer_helper.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.cc:27:
inval_region_.setRect(SkIRect::MakeWH(size.width(), size.height()));
nit: Technically this should be a union operation.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.cc:39: bounds.width() == size.width() &&
bounds.height() == size.height();
This will return true if the capture consists of two small rectangles at
opposite corners of the desktop.  Need to check that there is exactly one
rectangle in the region.

On the other hand, the IsCaptureFullScreen and InvalidateFullScreen logic looks
like it can just be got-rid of entirely.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.cc:39: bounds.width() == size.width() &&
bounds.height() == size.height();
See above.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helper.h
File remoting/host/capturer_helper.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:42: // common.SwapInvalidRegion(inval_region);
Does this example actually add anything?  How else would one expect to use it?

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:47: void SwapInvalidRegion(SkRegion&
inval_region);
Pass by pointer, not reference.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:57: SkRegion inval_region_;
Style: Rename |dirty_region| or |invalid_region|.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:60: base::Lock inval_region_lock_;
Rename to match region above.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux.cc
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux...
remoting/host/capturer_linux.cc:151: // with the previous buffer used.
nit: This comment doesn't parse.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux...
remoting/host/capturer_linux.cc:244: helper_.SwapInvalidRegion(region);
Since |region| isn't used after this call, isn't it just a clear operation?

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux...
remoting/host/capturer_linux.cc:389: // |rects| from |last_invalid_rects_|.
Now that we're using regions, perhaps implement that suggestion?

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.cc
File remoting/host/capturer_mac.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.c...
remoting/host/capturer_mac.cc:246: helper_.SwapInvalidRegion(region);
Should be clear, not swap?

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.c...
remoting/host/capturer_mac.cc:348: // the invalid region from the last capture
in the current buffer.
Same optimization Alpha suggests for the Linux code applies here.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.c...
remoting/host/capturer_mac.cc:463: InvalidateRegion(region);
Do we need to invalidate the source region for the copy here?  Surely only the
destination changes, and the source will be notified via a separate refresh
operation?

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac_u...
File remoting/host/capturer_mac_unittest.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac_u...
remoting/host/capturer_mac_unittest.cc:105: std::ostream&
operator<<(std::ostream& out, const SkRegion& region) {
Is there not already such an override?

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ.h
File remoting/host/differ.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ.h#newco...
remoting/host/differ.h:52: void MergeBlocks(SkRegion* region);
You decided not to optimize out these steps in this CL?

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
File remoting/host/differ_unittest.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:57: int RegionRectCount(const SkRegion& region)
{
SkRegion really doesn't have a method to query the number of rectangles? :(

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:146: // |x|, |y|, |width| and |height| are
specified in block (not pixel) units.
Update this comment, and the method name, to reflect what it actually does.  It
seems to check that the dirty region contains one rectangle matching the
dimensions given?

Given the usage below, it looks like it should instead be checking that the
SkRegion has exactly one rectangle, of the desired dimensions, though.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:169: bool is_good = RegionRectCount(dirty) ==
1;
Simpler to use isComplex() and get rid of RegionRectCount().

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:374: EXPECT_EQ(0, RegionRectCount(dirty));
EXPECT_TRUE(dirty.isEmpty());

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:549: ASSERT_TRUE(CheckDirtyRegion(dirty, 2, 2,
1, 1));
Clearer to restructure this to iterate over dirty three times, checking each
rect, then assert empty?

If you don't want to make assumptions about the order the SkRegion will have the
rects in, then for each rect check that it is completely contained by the
region, then subtract it from it, for an equivalent effect.

http://codereview.chromium.org/7491070/diff/4001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/remoting.gyp#newcode99
remoting/remoting.gyp:99: '../third_party/skia/include/config',
Do we need these if our targets have appropriate Skia dependencies?

http://codereview.chromium.org/7491070/diff/4001/remoting/remoting.gyp#newcod...
remoting/remoting.gyp:135: # crbug.com/74951
Now that we have a real dependency on Skia, is this comment valid?

http://codereview.chromium.org/7491070/diff/4001/remoting/remoting.gyp#newcod...
remoting/remoting.gyp:274: 'dependencies': [
Doesn't base depend on skia now, since EncoderVp8 uses it internally?

[dmac, 2011-08-10 20:30:35]

I think I dealt with all of these.

http://codereview.chromium.org/7491070/diff/4001/remoting/base/codec_test.cc
File remoting/base/codec_test.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/base/codec_test.cc#...
remoting/base/codec_test.cc:143: decoder_->GetUpdatedRects(&update_rects_);
On 2011/08/08 20:49:34, Wez wrote:
> Shouldn't this change to GetUpdatedRegion()?

I didn't want to switch over the decoder on this CL. Have added a comment and a
bug to decoder.h

http://codereview.chromium.org/7491070/diff/4001/remoting/base/codec_test.cc#...
remoting/base/codec_test.cc:242: uint8** memory) {
On 2011/08/08 20:49:34, Wez wrote:
> nit: Making this a scoped_array<uint8>* seems cleaner.

http://crbug.com/92086

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer.h
File remoting/host/capturer.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer.h#new...
remoting/host/capturer.h:58: virtual void InvalidateRegion(const SkRegion&
inval_region) = 0;
On 2011/08/08 20:49:34, Wez wrote:
> nit: This should be |region|, |dirty_region| or |invalid_region|, to fit the
> style guide.

Seriously? Fixed all over :-)

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer.h#new...
remoting/host/capturer.h:72: // call the |callback| routine.
On 2011/08/08 20:49:34, Wez wrote:
> nit: These two sentences could be combined, i.e. "... even if the dirty region
> is empty".

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_fake.h
File remoting/host/capturer_fake.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_fake....
remoting/host/capturer_fake.h:27: virtual void InvalidateRegion(const SkRegion&
inval_region) OVERRIDE;
On 2011/08/08 20:49:34, Wez wrote:
> nit: Parameter name.

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
File remoting/host/capturer_helper.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.cc:27:
inval_region_.setRect(SkIRect::MakeWH(size.width(), size.height()));
On 2011/08/08 20:49:34, Wez wrote:
> nit: Technically this should be a union operation.

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.cc:39: bounds.width() == size.width() &&
bounds.height() == size.height();
On 2011/08/08 20:49:34, Wez wrote:
> This will return true if the capture consists of two small rectangles at
> opposite corners of the desktop.  Need to check that there is exactly one
> rectangle in the region.
> 
> On the other hand, the IsCaptureFullScreen and InvalidateFullScreen logic
looks
> like it can just be got-rid of entirely.

Added bug and comment.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helper.h
File remoting/host/capturer_helper.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:42: // common.SwapInvalidRegion(inval_region);
On 2011/08/08 20:49:34, Wez wrote:
> Does this example actually add anything?  How else would one expect to use it?

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:47: void SwapInvalidRegion(SkRegion&
inval_region);
On 2011/08/08 20:49:34, Wez wrote:
> Pass by pointer, not reference.

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:57: SkRegion inval_region_;
On 2011/08/08 20:49:34, Wez wrote:
> Style: Rename |dirty_region| or |invalid_region|.

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_helpe...
remoting/host/capturer_helper.h:60: base::Lock inval_region_lock_;
On 2011/08/08 20:49:34, Wez wrote:
> Rename to match region above.

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux.cc
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux...
remoting/host/capturer_linux.cc:151: // with the previous buffer used.
On 2011/08/08 20:49:34, Wez wrote:
> nit: This comment doesn't parse.

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux...
remoting/host/capturer_linux.cc:244: helper_.SwapInvalidRegion(region);
On 2011/08/08 20:49:34, Wez wrote:
> Since |region| isn't used after this call, isn't it just a clear operation?

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_linux...
remoting/host/capturer_linux.cc:389: // |rects| from |last_invalid_rects_|.
On 2011/08/08 20:49:34, Wez wrote:
> Now that we're using regions, perhaps implement that suggestion?
Added bug

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.cc
File remoting/host/capturer_mac.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.c...
remoting/host/capturer_mac.cc:246: helper_.SwapInvalidRegion(region);
On 2011/08/08 20:49:34, Wez wrote:
> Should be clear, not swap?

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.c...
remoting/host/capturer_mac.cc:348: // the invalid region from the last capture
in the current buffer.
On 2011/08/08 20:49:34, Wez wrote:
> Same optimization Alpha suggests for the Linux code applies here.

Added comment

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac.c...
remoting/host/capturer_mac.cc:463: InvalidateRegion(region);
On 2011/08/08 20:49:34, Wez wrote:
> Do we need to invalidate the source region for the copy here?  Surely only the
> destination changes, and the source will be notified via a separate refresh
> operation?

Good point. Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac_u...
File remoting/host/capturer_mac_unittest.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/capturer_mac_u...
remoting/host/capturer_mac_unittest.cc:105: std::ostream&
operator<<(std::ostream& out, const SkRegion& region) {
On 2011/08/08 20:49:34, Wez wrote:
> Is there not already such an override?

Not that I am aware of. There is a "tostring"  but is defined for Android only.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ.h
File remoting/host/differ.h (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ.h#newco...
remoting/host/differ.h:52: void MergeBlocks(SkRegion* region);
On 2011/08/08 20:49:34, Wez wrote:
> You decided not to optimize out these steps in this CL?

Added todo at the top of this file, and have now logged a bug to track it as
well.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
File remoting/host/differ_unittest.cc (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:57: int RegionRectCount(const SkRegion& region)
{
On 2011/08/08 20:49:34, Wez wrote:
> SkRegion really doesn't have a method to query the number of rectangles? :(

Nope

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:146: // |x|, |y|, |width| and |height| are
specified in block (not pixel) units.
On 2011/08/08 20:49:34, Wez wrote:
> Update this comment, and the method name, to reflect what it actually does. 
It
> seems to check that the dirty region contains one rectangle matching the
> dimensions given?
> 
> Given the usage below, it looks like it should instead be checking that the
> SkRegion has exactly one rectangle, of the desired dimensions, though.

It is used in different ways below, but I did update name and comment.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:169: bool is_good = RegionRectCount(dirty) ==
1;
On 2011/08/08 20:49:34, Wez wrote:
> Simpler to use isComplex() and get rid of RegionRectCount().

RegionRectCount is used elsewhere, but I did get rid of this case.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:374: EXPECT_EQ(0, RegionRectCount(dirty));
On 2011/08/08 20:49:34, Wez wrote:
> EXPECT_TRUE(dirty.isEmpty());

Done.

http://codereview.chromium.org/7491070/diff/4001/remoting/host/differ_unittes...
remoting/host/differ_unittest.cc:549: ASSERT_TRUE(CheckDirtyRegion(dirty, 2, 2,
1, 1));
On 2011/08/08 20:49:34, Wez wrote:
> Clearer to restructure this to iterate over dirty three times, checking each
> rect, then assert empty?
> 
> If you don't want to make assumptions about the order the SkRegion will have
the
> rects in, then for each rect check that it is completely contained by the
> region, then subtract it from it, for an equivalent effect.

I'm going to leave as is for right now. It keeps all the test cases looking very
similar so you don't need to "learn" a new pattern.

http://codereview.chromium.org/7491070/diff/4001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/7491070/diff/4001/remoting/remoting.gyp#newcode99
remoting/remoting.gyp:99: '../third_party/skia/include/config',
On 2011/08/08 20:49:34, Wez wrote:
> Do we need these if our targets have appropriate Skia dependencies?

Tightened up the includes properly.

[Wez, 2011-08-10 22:05:43]

A few more nits, but otherwise LGTM.

http://codereview.chromium.org/7491070/diff/11001/remoting/base/capture_data.cc
File remoting/base/capture_data.cc (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/base/capture_data....
remoting/base/capture_data.cc:20: dirty_region_(),
nit: Do we need to explicitly default-ctor |dirty_region_|?

http://codereview.chromium.org/7491070/diff/11001/remoting/base/capture_data.h
File remoting/base/capture_data.h (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/base/capture_data....
remoting/base/capture_data.h:37: // Get the dirty region from the last capture.
nit: "last" -> "previous", to avoid ambiguity...

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer.h
File remoting/host/capturer.h (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer.h#ne...
remoting/host/capturer.h:58: virtual void InvalidateRegion(const SkRegion&
inval_region) = 0;
|inval_region|?  ORLY?

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer.h#ne...
remoting/host/capturer.h:69: // When the capture is complete, |callback| is
called even If the dirty region
nit: "If" -> "if".

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer.h#ne...
remoting/host/capturer.h:73: // data of the last capture.
nit: "last"->"previous".

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_fake.h
File remoting/host/capturer_fake.h (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_fake...
remoting/host/capturer_fake.h:27: virtual void InvalidateRegion(const SkRegion&
inval_region) OVERRIDE;
|invalid_region|?

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_fake...
File remoting/host/capturer_fake_ascii.h (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_fake...
remoting/host/capturer_fake_ascii.h:28: virtual void InvalidateRegion(const
SkRegion& inval_region) OVERRIDE;
|inval_region|

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
File remoting/host/capturer_helper.cc (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
remoting/host/capturer_helper.cc:40: // http://crbug.com/92346
The bug is introduced by this CL, though.  Can be fixed by testing
invalid_region.isComplex(), I think (returns true for 0 or 1 rectangle regions).

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
File remoting/host/capturer_helper.h (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
remoting/host/capturer_helper.h:26: void InvalidateRegion(const SkRegion&
inval_region);
|inval_region|

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
remoting/host/capturer_helper.h:39: void SwapInvalidRegion(SkRegion*
inval_region);
|inval_region|

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_linu...
File remoting/host/capturer_linux.cc (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_linu...
remoting/host/capturer_linux.cc:151: // current with the last buffer used.
nit: Previous.

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_linu...
remoting/host/capturer_linux.cc:152: SkRegion last_invalid_region_;
nit: |previous_invalid_region_|

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_mac.cc
File remoting/host/capturer_mac.cc (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_mac....
remoting/host/capturer_mac.cc:185: // Contains an invalid region from the last
capture.
nit: "last"->"previous"

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_mac....
remoting/host/capturer_mac.cc:186: SkRegion last_invalid_region_;
nit: |previous...|

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_mac....
remoting/host/capturer_mac.cc:347: // the invalid region from the last capture
in the current buffer.
nit: previous.

[Wez, 2011-08-10 22:06:22]

Oh, but please take a look at the comment regarding the IsFullScreen bug.

[Lambros, 2011-08-10 22:17:09]

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
File remoting/host/capturer_helper.cc (right):

http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
remoting/host/capturer_helper.cc:40: // http://crbug.com/92346
On 2011/08/10 22:05:43, Wez wrote:
> The bug is introduced by this CL, though.  Can be fixed by testing
> invalid_region.isComplex(), I think (returns true for 0 or 1 rectangle
regions).

Drive-by: If you want to test whether |invalid_region_| is full-screen, why not
construct a temporary region with the screen rectangle, then compare for
equality with |invalid_region| ?  Why mess with internals (bounds,
rectangle-counting) when you don't have to? :)

[Wez, 2011-08-10 22:19:59]

On 2011/08/10 22:17:09, Lambros wrote:
>
http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
> File remoting/host/capturer_helper.cc (right):
> 
>
http://codereview.chromium.org/7491070/diff/11001/remoting/host/capturer_help...
> remoting/host/capturer_helper.cc:40: // http://crbug.com/92346
> On 2011/08/10 22:05:43, Wez wrote:
> > The bug is introduced by this CL, though.  Can be fixed by testing
> > invalid_region.isComplex(), I think (returns true for 0 or 1 rectangle
> regions).
> 
> Drive-by: If you want to test whether |invalid_region_| is full-screen, why
not
> construct a temporary region with the screen rectangle, then compare for
> equality with |invalid_region| ?  Why mess with internals (bounds,
> rectangle-counting) when you don't have to? :)

A very good point...