Add GrabWindowSnapshot method to mac_util class. This code is due to dmaclac...

Add GrabWindowSnapshot method to mac_util class, and a unit test.

BUG= none
TEST= none

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

[dmac, 2009-10-23 23:12:06]

Is there a unit test for this somewhere?

http://codereview.chromium.org/333018/diff/1/2
File base/mac_util.mm (right):

http://codereview.chromium.org/333018/diff/1/2#newcode178
Line 178: *(const unsigned char*)[data bytes]);
I'm not sure this will work. Unless I'm way off base doesn't this just fill
png_representation with [data length] copies of [data bytes][0]?

I think you should be able to just do png_represetation->assign([data length],
static_cast<const unsigned char*>([data bytes]));

[Miranda Callahan, 2009-10-23 23:27:48]

I need to add a unit test, good call.

http://codereview.chromium.org/333018/diff/1/2
File base/mac_util.mm (right):

http://codereview.chromium.org/333018/diff/1/2#newcode178
Line 178: *(const unsigned char*)[data bytes]);
Hmm -- looking at this again, I think we're both wrong:
http://www.cplusplus.com/reference/stl/vector/assign/

Not sure off the top of my head how to do this most cleverly.

On 2009/10/23 23:12:06, dmac wrote:
> I'm not sure this will work. Unless I'm way off base doesn't this just fill
> png_representation with [data length] copies of [data bytes][0]?
> 
> I think you should be able to just do png_represetation->assign([data length],
> static_cast<const unsigned char*>([data bytes]));

[dmac, 2009-10-23 23:38:12]

On 2009/10/23 23:27:48, Miranda Callahan wrote:
> I need to add a unit test, good call.
> 
> http://codereview.chromium.org/333018/diff/1/2
> File base/mac_util.mm (right):
> 
> http://codereview.chromium.org/333018/diff/1/2#newcode178
> Line 178: *(const unsigned char*)[data bytes]);
> Hmm -- looking at this again, I think we're both wrong:
> http://www.cplusplus.com/reference/stl/vector/assign/
> 
> Not sure off the top of my head how to do this most cleverly.
> 
> On 2009/10/23 23:12:06, dmac wrote:
> > I'm not sure this will work. Unless I'm way off base doesn't this just fill
> > png_representation with [data length] copies of [data bytes][0]?
> > 
> > I think you should be able to just do png_represetation->assign([data
length],
> > static_cast<const unsigned char*>([data bytes]));

Correct you are. Sigh.. just was doing it off the top of my head. I would
probably do:
const unsigned char* buffer = static_cast<const unsigned char*>([data bytes]);
NSUInteger length = [data length];
png_representation->reserve(length);
std::copy(buffer, buffer + length, pnt_representation->begin());

If I remember correctly I think that should do it.

[Miranda Callahan, 2009-10-23 23:38:29]

I'm going to use copy from the algorithm library in the STL, unless you think
there is a better way.

On 2009/10/23 23:27:48, Miranda Callahan wrote:
> I need to add a unit test, good call.
> 
> http://codereview.chromium.org/333018/diff/1/2
> File base/mac_util.mm (right):
> 
> http://codereview.chromium.org/333018/diff/1/2#newcode178
> Line 178: *(const unsigned char*)[data bytes]);
> Hmm -- looking at this again, I think we're both wrong:
> http://www.cplusplus.com/reference/stl/vector/assign/
> 
> Not sure off the top of my head how to do this most cleverly.
> 
> On 2009/10/23 23:12:06, dmac wrote:
> > I'm not sure this will work. Unless I'm way off base doesn't this just fill
> > png_representation with [data length] copies of [data bytes][0]?
> > 
> > I think you should be able to just do png_represetation->assign([data
length],
> > static_cast<const unsigned char*>([data bytes]));

[dmac, 2009-10-23 23:41:51]

On 2009/10/23 23:38:29, Miranda Callahan wrote:
> I'm going to use copy from the algorithm library in the STL, unless you think
> there is a better way.

heh... sounds good to me ;-)

[Miranda Callahan, 2009-10-23 23:48:08]

:-)  All right, new and improved!  But this all proves that it definitely needs
a unit test -- I'll try to get that out either after dinner or later in the
weekend.

On 2009/10/23 23:41:51, dmac wrote:
> On 2009/10/23 23:38:29, Miranda Callahan wrote:
> > I'm going to use copy from the algorithm library in the STL, unless you
think
> > there is a better way.
> 
> heh... sounds good to me ;-)

[dmac, 2009-10-23 23:52:43]

http://codereview.chromium.org/333018/diff/7001/2002
File base/mac_util.mm (right):

http://codereview.chromium.org/333018/diff/7001/2002#newcode171
Line 171: // Make sure to grab the "window frame" view so we get all tabs.
"all the tabs." or "get the tabstrip."

http://codereview.chromium.org/333018/diff/7001/2002#newcode177
Line 177: const unsigned char* buf = static_cast<const unsigned char*>([data
bytes]);
should probably check to make sure that data is non-nil before proceeding into
the stl stuff. Do you care about the failure case?

[John Grabowski, 2009-10-26 03:35:02]

Drive by

Why do you prefer to return the data in a vector?  Won't most callers
immediately call [NSBitmapImageRep imageRepWithData:] right after this?  If so,
perhaps this can return an NSBitmapImageRep instead?

If this instead is the Mac impl of an x-plat API (e.g. create a SkiaBitmap*),
perhaps you could hook that up in this CL?

[Amanda Walker, 2009-10-26 12:43:29]

Driving by jrg's drive-by:
This is for the "report a buggy web page" feature (the data will go into an HTTP
POST or the like); it's specifically for the "I need a vector of bytes
containing a PNG screen shot" case, comparable to win_util::GrabWindowSnapshot.

[John Grabowski, 2009-10-26 13:56:32]

Perhaps then it should be called

  void GrabWindowSnapshot(gfx::NativeWindow, ...)

And #ifdefs removed from the call site?

jrg

On Mon, Oct 26, 2009 at 5:43 AM, <amanda@chromium.org> wrote:

> Driving by jrg's drive-by:
> This is for the "report a buggy web page" feature (the data will go into an
> HTTP
> POST or the like); it's specifically for the "I need a vector of bytes
> containing a PNG screen shot" case, comparable to
> win_util::GrabWindowSnapshot.
>
>
> http://codereview.chromium.org/333018
>

[pink (ping after 24hrs), 2009-10-26 14:12:09]

more drive-by's

http://codereview.chromium.org/333018/diff/7001/2002
File base/mac_util.mm (right):

http://codereview.chromium.org/333018/diff/7001/2002#newcode165
Line 165: void GrabWindowSnapshot(const NSWindow* window_handle,
mac programmers almost never refer to these as "window handles". It's ok to just
call this |window|.

http://codereview.chromium.org/333018/diff/7001/2002#newcode167
Line 167: // Images are big.  Let's get the rep off our stack as soon as
possible in
This comment doesn't make sense. What here is on the stack? The pool? That's for
heap-allocated objects.

http://codereview.chromium.org/333018/diff/7001/2002#newcode180
Line 180: std::copy(buf, buf + length, png_representation->begin());
ensure length > 0?

[Miranda Callahan, 2009-10-26 19:46:06]

Ok -- I tried to address everybody's comments, and I added a unit test.  Please
take another look!

http://codereview.chromium.org/333018/diff/7001/2002
File base/mac_util.mm (right):

http://codereview.chromium.org/333018/diff/7001/2002#newcode165
Line 165: void GrabWindowSnapshot(const NSWindow* window_handle,
On 2009/10/26 14:12:09, pink wrote:
> mac programmers almost never refer to these as "window handles". It's ok to
just
> call this |window|.

Done.

http://codereview.chromium.org/333018/diff/7001/2002#newcode167
Line 167: // Images are big.  Let's get the rep off our stack as soon as
possible in
On 2009/10/26 14:12:09, pink wrote:
> This comment doesn't make sense. What here is on the stack? The pool? That's
for
> heap-allocated objects.

Artifact from when I took the code from dmac.  Removed.

http://codereview.chromium.org/333018/diff/7001/2002#newcode171
Line 171: // Make sure to grab the "window frame" view so we get all tabs.
On 2009/10/23 23:52:43, dmac wrote:
> "all the tabs." or "get the tabstrip."

Done.

http://codereview.chromium.org/333018/diff/7001/2002#newcode177
Line 177: const unsigned char* buf = static_cast<const unsigned char*>([data
bytes]);
On 2009/10/23 23:52:43, dmac wrote:
> should probably check to make sure that data is non-nil before proceeding into
> the stl stuff. Do you care about the failure case?

Done.

http://codereview.chromium.org/333018/diff/7001/2002#newcode180
Line 180: std::copy(buf, buf + length, png_representation->begin());
On 2009/10/26 14:12:09, pink wrote:
> ensure length > 0?

Done.

[Miranda Callahan, 2009-10-26 20:14:46]

As I just discussed with jrg in person, I couldn't make the change of the
signature of GrabWindowScreenshot to take a gfx::NativeWindow, because it breaks
dependencies requirements.

[John Grabowski, 2009-10-26 21:04:27]

http://codereview.chromium.org/333018/diff/8/1006
File base/base.gyp (right):

http://codereview.chromium.org/333018/diff/8/1006#newcode666
Line 666: 'mac_util_unittest.mm',
Unnecessary; I believe the unit test target is smart enough to remove .mm files
on non-Mac platforms.

http://codereview.chromium.org/333018/diff/8/1005
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/333018/diff/8/1005#newcode52
Line 52: [[NSData alloc] initWithBytes:&(*png_representation)[0]
   &(*png)[0] 
is hard to read since you're using 2 overloaded operators at once.  How about
 &(png.get()[0])
Not sure that is more clear to anyone other than me tho.

http://codereview.chromium.org/333018/diff/8/1005#newcode55
Line 55: EXPECT_TRUE([rep isKindOfClass:[NSBitmapImageRep class]]);
Confirm the data isn't garbage, or all zeros?  It's supposed to be all white
which is easy to spot.
Confirm the data returned reflects a 400x400 window and isn't just a single
char?

[dmac, 2009-10-26 21:45:21]

http://codereview.chromium.org/333018/diff/8/1005
File base/mac_util_unittest.mm (left):

http://codereview.chromium.org/333018/diff/8/1005#oldcode7
Line 7: #include <ApplicationServices/ApplicationServices.h>
do you need cocoa and app services?

http://codereview.chromium.org/333018/diff/8/1005
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/333018/diff/8/1005#newcode18
Line 18: typedef PlatformTest MacUtilTest;
can you use the new CocoaTest that I just added?

http://codereview.chromium.org/333018/diff/8/1005#newcode36
Line 36: [NSApplication sharedApplication];
if you use CocoaTest you can get rid of initing the app and you can just use
"test_window()" to get your window. Remember to close it at the end though.

[Miranda Callahan, 2009-10-27 00:13:17]

Addressed new concerns, please take another look.  Thanks for all of your help!

http://codereview.chromium.org/333018/diff/8/1006
File base/base.gyp (right):

http://codereview.chromium.org/333018/diff/8/1006#newcode666
Line 666: 'mac_util_unittest.mm',
On 2009/10/26 21:04:28, John Grabowski wrote:
> Unnecessary; I believe the unit test target is smart enough to remove .mm
files
> on non-Mac platforms.

Done.

http://codereview.chromium.org/333018/diff/8/1005
File base/mac_util_unittest.mm (left):

http://codereview.chromium.org/333018/diff/8/1005#oldcode7
Line 7: #include <ApplicationServices/ApplicationServices.h>
On 2009/10/26 21:45:21, dmac wrote:
> do you need cocoa and app services?

Removed app services.

http://codereview.chromium.org/333018/diff/8/1005
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/333018/diff/8/1005#newcode18
Line 18: typedef PlatformTest MacUtilTest;
On 2009/10/26 21:45:21, dmac wrote:
> can you use the new CocoaTest that I just added?

Hmmm. I would like to do this, but this would mean either having
base/mac_util_unittest.mm depend on something in the chrome/browser directory,
or moving the test for this specific mac_util function out of base -- which
doesn't seem right either. Unless you think it's important enough to make this
change, I'm going to remain with this least of many evils, just creating a test
window to grab a snapshot of.

http://codereview.chromium.org/333018/diff/8/1005#newcode36
Line 36: [NSApplication sharedApplication];
On 2009/10/26 21:45:21, dmac wrote:
> if you use CocoaTest you can get rid of initing the app and you can just use
> "test_window()" to get your window. Remember to close it at the end though.

I wish I could use it without reaching across to chrome! :-(

http://codereview.chromium.org/333018/diff/8/1005#newcode52
Line 52: [[NSData alloc] initWithBytes:&(*png_representation)[0]
On 2009/10/26 21:04:28, John Grabowski wrote:
>    &(*png)[0] 
> is hard to read since you're using 2 overloaded operators at once.  How about
>  &(png.get()[0])
> Not sure that is more clear to anyone other than me tho.  

Unfortunately, the get doesn't help -- we still have to do 

&(*png_representation.get())[0]

I don't think there's a way to remove indirection -- we have to
1) get the vector which is pointed to, so we can use the [] operator
2) use [0] to get the first element of the vector
3) use & to get the address of the first element
and because vectors store memory contiguously, 
4) profit!

http://codereview.chromium.org/333018/diff/8/1005#newcode55
Line 55: EXPECT_TRUE([rep isKindOfClass:[NSBitmapImageRep class]]);
On 2009/10/26 21:04:28, John Grabowski wrote:
> Confirm the data isn't garbage, or all zeros?  It's supposed to be all white
> which is easy to spot.
> Confirm the data returned reflects a 400x400 window and isn't just a single
> char?

Done.

[John Grabowski, 2009-10-27 00:28:28]

LGTM

http://codereview.chromium.org/333018/diff/10009/3006
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/333018/diff/10009/3006#newcode54
Line 54: EXPECT_TRUE(CGImageGetWidth([rep CGImage]) == 400);
I don't see any confirmation the data isn't bogus.  How about adding this:

NSColor* color = [rep colorAtX:200 y:200];
EXPECT_GE([color whiteComponent], 0.8);

[Miranda Callahan, 2009-10-27 15:55:00]

Added it -- I had to do some fooling around, as whiteComponent expects a
calibrated color -- but the testing for color works.

On 2009/10/27 00:28:28, John Grabowski wrote:
> LGTM
> 
> http://codereview.chromium.org/333018/diff/10009/3006
> File base/mac_util_unittest.mm (right):
> 
> http://codereview.chromium.org/333018/diff/10009/3006#newcode54
> Line 54: EXPECT_TRUE(CGImageGetWidth([rep CGImage]) == 400);
> I don't see any confirmation the data isn't bogus.  How about adding this:
> 
> NSColor* color = [rep colorAtX:200 y:200];
> EXPECT_GE([color whiteComponent], 0.8);

[dmac, 2009-10-27 17:35:05]

LGTM