Enable dragging of images to desktop (Finder), Preview, etc. (on Mac)....

Enable dragging of images to desktop (Finder), Preview, etc. (on Mac).

This hooks up drag-and-drop of file promises, lazy writing to the drag
pasteboard, drag-sourcing of TIFF images (via Cocoa) and file contents.

Patch Set 5 improvements: Adds asynchronous writing of promised files.

Patch Set 4 improvements: Big refactoring -- drag source stuff is now
handled by the WebDragSource (Cocoa) object, with messages proxied
through the TabContentsViewCocoa object. The WebDragSource object
carries a weak reference to the TCVC, owns the WebDropData, and keeps
track of the drag pasteboard types/promises.

Patch Set 3 improvements over Patch Set 2: It shouldn't crash anymore. Made
drop_data_ reference counted, in anticipation of asynchronous file
writing.

TODO #1: Testing. Still need a unit test, maybe. Should make sure that
dragging by file contents actually works.

TODO #2 (in some other patch): Refactor some of the WebDropData
extraction code out, e.g., file name extraction should be made common
with other platforms.

TODO #3 (in some other patch): We really should make WebDropData cheaper
to copy around and retain. I'm not convinced it's a good idea to push
out the entire thing over IPC, especially since the data pushed could be
very big and may not even be used.

BUG=15640
TEST=drag images to various applications

Patch by viettrungluu@gmail.com

[Avi (use Gerrit), 2009-07-27 18:33:01]

Looks reasonable to me; have to leave it to Pink for the final OK.

http://codereview.chromium.org/160083/diff/23/26
File chrome/browser/cocoa/web_drag_source.h (right):

http://codereview.chromium.org/160083/diff/23/26#newcode19
Line 19: // Our drop data. Should only be initialized once. Strong reference.
scoped_ptr means strong ref; you don't have to note that in the comments.

http://codereview.chromium.org/160083/diff/23/26#newcode27
Line 27: DropData:(const WebDropData*)dropData;
decap the first d in "DropData:"

http://codereview.chromium.org/160083/diff/23/27
File chrome/browser/cocoa/web_drag_source.mm (right):

http://codereview.chromium.org/160083/diff/23/27#newcode69
Line 69: // This class's sole task is to write out
write out what?

http://codereview.chromium.org/160083/diff/23/27#newcode79
Line 79: FileStream* file_stream_;
ownership?

[viettrungluu-gmail, 2009-07-27 18:44:36]

Thanks Avi.

Everything corrected/clarified as per your comments.

http://codereview.chromium.org/160083/diff/23/27
File chrome/browser/cocoa/web_drag_source.mm (right):

http://codereview.chromium.org/160083/diff/23/27#newcode69
Line 69: // This class's sole task is to write out
On 2009/07/27 18:33:02, Avi wrote:
> write out what?

Ummm, promised file data.

http://codereview.chromium.org/160083/diff/23/27#newcode79
Line 79: FileStream* file_stream_;
On 2009/07/27 18:33:02, Avi wrote:
> ownership?

... assumed by this class.

[pink (ping after 24hrs), 2009-07-27 21:12:24]

http://codereview.chromium.org/160083/diff/1025/40
File chrome/browser/cocoa/web_drag_source.h (right):

http://codereview.chromium.org/160083/diff/1025/40#newcode17
Line 17: TabContentsViewCocoa* tcvc_;
when reading the code, nobody would be able to deduce what this resolves to.
Pick a better name.

http://codereview.chromium.org/160083/diff/1025/40#newcode22
Line 22: // Our pasteboard (nil when done). Weak reference (globally owned).
it's not necessarily globally owned, it could be created locally in a unit test.
We should probably hold a reference to it while we're working on it, no?

http://codereview.chromium.org/160083/diff/1025/40#newcode26
Line 26: - (id)initWithTCVC:(TabContentsViewCocoa*)tcvc
need a better name. We don't use TCVC anywhere else in the code. How about
|-initWithContentsView:...|?

http://codereview.chromium.org/160083/diff/1025/40#newcode28
Line 28: - (void)lazyWriteToPasteboard:(NSPasteboard*)pboard
all of these need comments if they're part of a public api.

http://codereview.chromium.org/160083/diff/1025/41
File chrome/browser/cocoa/web_drag_source.mm (right):

http://codereview.chromium.org/160083/diff/1025/41#newcode18
Line 18: #include "net/base/file_stream.h"
alphabetize this include

http://codereview.chromium.org/160083/diff/1025/41#newcode80
Line 80: WebDragSource* drag_source_;
if this is a strong ref, use scoped_nsobject

http://codereview.chromium.org/160083/diff/1025/41#newcode83
Line 83: FileStream* file_stream_;
if this now owns the ptr, use scoped_ptr

http://codereview.chromium.org/160083/diff/1025/41#newcode140
Line 140: pasteboard_ = [NSPasteboard pasteboardWithName:NSDragPboard];
you should take the pasteboard as a parameter so this can be unit tested with
dependency injection.

http://codereview.chromium.org/160083/diff/1025/41#newcode168
Line 168: arrayWithObject:SysUTF16ToNSString(dropData_->url_title)]];
this should be indented more, it's not the same parameter list as the setURLs
call

http://codereview.chromium.org/160083/diff/1025/41#newcode176
Line 176: NSFileWrapper* file_wrapper =
use a scoped_nsobject

http://codereview.chromium.org/160083/diff/1025/41#newcode191
Line 191: NSImage* image = [[NSImage alloc] initWithData:[NSData
could this possibly crash us? do we need to worry about bad images?

http://codereview.chromium.org/160083/diff/1025/41#newcode273
Line 273: FileStream* file_stream = new FileStream;
use a scoped_ptr which avoids all the cleanup at each early return site.

http://codereview.chromium.org/160083/diff/1025/41#newcode293
Line 293: if(seq > k_max_seq) {
space after if and before paren

http://codereview.chromium.org/160083/diff/1025/41#newcode320
Line 320: // Don't reset dropData_, since our PromiseWriterTask may have a stake
in it.
the promiseWriterTask should copy anything it needs as it'll be over on a
different thread. To use anything in this object between threads w/out locks
would be bad.

[viettrungluu-gmail, 2009-07-28 00:20:59]

Thanks. I made the suggested changes (and a couple more). Say the word, and I'll
disable TIFF dragging (a little unfortunate, but seems hard to avoid presently).

http://codereview.chromium.org/160083/diff/1025/40
File chrome/browser/cocoa/web_drag_source.h (right):

http://codereview.chromium.org/160083/diff/1025/40#newcode17
Line 17: TabContentsViewCocoa* tcvc_;
On 2009/07/27 21:12:24, pink wrote:
> when reading the code, nobody would be able to deduce what this resolves to.
> Pick a better name.

How about contentsView_ (as per below)?

http://codereview.chromium.org/160083/diff/1025/40#newcode22
Line 22: // Our pasteboard (nil when done). Weak reference (globally owned).
On 2009/07/27 21:12:24, pink wrote:
> it's not necessarily globally owned, it could be created locally in a unit
test.
> We should probably hold a reference to it while we're working on it, no?

Fair enough.

http://codereview.chromium.org/160083/diff/1025/40#newcode26
Line 26: - (id)initWithTCVC:(TabContentsViewCocoa*)tcvc
On 2009/07/27 21:12:24, pink wrote:
> need a better name. We don't use TCVC anywhere else in the code. How about
> |-initWithContentsView:...|?

Sold.

http://codereview.chromium.org/160083/diff/1025/40#newcode28
Line 28: - (void)lazyWriteToPasteboard:(NSPasteboard*)pboard
On 2009/07/27 21:12:24, pink wrote:
> all of these need comments if they're part of a public api.

Okay. Will do.

http://codereview.chromium.org/160083/diff/1025/41
File chrome/browser/cocoa/web_drag_source.mm (right):

http://codereview.chromium.org/160083/diff/1025/41#newcode18
Line 18: #include "net/base/file_stream.h"
On 2009/07/27 21:12:24, pink wrote:
> alphabetize this include

Done.

http://codereview.chromium.org/160083/diff/1025/41#newcode80
Line 80: WebDragSource* drag_source_;
On 2009/07/27 21:12:24, pink wrote:
> if this is a strong ref, use scoped_nsobject

Done.

http://codereview.chromium.org/160083/diff/1025/41#newcode83
Line 83: FileStream* file_stream_;
On 2009/07/27 21:12:24, pink wrote:
> if this now owns the ptr, use scoped_ptr

Done.

http://codereview.chromium.org/160083/diff/1025/41#newcode140
Line 140: pasteboard_ = [NSPasteboard pasteboardWithName:NSDragPboard];
On 2009/07/27 21:12:24, pink wrote:
> you should take the pasteboard as a parameter so this can be unit tested with
> dependency injection.

Done.

http://codereview.chromium.org/160083/diff/1025/41#newcode168
Line 168: arrayWithObject:SysUTF16ToNSString(dropData_->url_title)]];
On 2009/07/27 21:12:24, pink wrote:
> this should be indented more, it's not the same parameter list as the setURLs
> call

Done.

http://codereview.chromium.org/160083/diff/1025/41#newcode176
Line 176: NSFileWrapper* file_wrapper =
On 2009/07/27 21:12:24, pink wrote:
> use a scoped_nsobject

Done.

http://codereview.chromium.org/160083/diff/1025/41#newcode191
Line 191: NSImage* image = [[NSImage alloc] initWithData:[NSData
On 2009/07/27 21:12:24, pink wrote:
> could this possibly crash us? do we need to worry about bad images?

(Note to self: also use a scoped_nsobject here.)

We're relying on Apple/Cocoa to be robust here, which I imagine they should be
(it should just return nil on failure). The documentation is very vague about
what data is even accepted. I have faith on its robustness, but it's still not
ideal for many reasons. Ideally, we should this asynchronously, using our own
renderer. (Right now, it could block for a bit, if the image is big, or a
complicated SVG/etc.) But it's not obvious if Cocoa will allow us to call
-[NSPasteboard setData:forType:] later.

For safety/paranoia's sake, it's probably better to turn this off until/unless
we get it to do the processing in a render process.

http://codereview.chromium.org/160083/diff/1025/41#newcode273
Line 273: FileStream* file_stream = new FileStream;
On 2009/07/27 21:12:24, pink wrote:
> use a scoped_ptr which avoids all the cleanup at each early return site.

No can do here, since we have to pass off ownership to the writer task.

http://codereview.chromium.org/160083/diff/1025/41#newcode293
Line 293: if(seq > k_max_seq) {
On 2009/07/27 21:12:24, pink wrote:
> space after if and before paren

Done.

http://codereview.chromium.org/160083/diff/1025/41#newcode320
Line 320: // Don't reset dropData_, since our PromiseWriterTask may have a stake
in it.
On 2009/07/27 21:12:24, pink wrote:
> the promiseWriterTask should copy anything it needs as it'll be over on a
> different thread. To use anything in this object between threads w/out locks
> would be bad.

You're probably right that this is bad design, but some comments:
- I'm trying to not copy data, since that's arbitrarily expensive in terms of
time and space.
- The correct fix is to fix our WebDropData class, I suppose.
- I'm pretty sure it's actually currently thread-safe (before I made the other
changes), but things get complicated with the use of scoped_nsobject's. (Without
them, it's easier to ensure where things like the pasteboard get released, which
may or may not be thread-safe.)

So I'll modify the PromiseWriterTask to take a copy of the WebDropData (in the
current version, it retains the WebDragSource, taking advantage of
NSObject-thread-safeness). But the caveat is that this entails arbitrary-size
memory allocations and copies, which would especially be a problem if we ever
want to run this on a small device (iPhone? "iPad"?).

[pink (ping after 24hrs), 2009-07-30 16:49:28]

LGTM with these addressed.

http://codereview.chromium.org/160083/diff/45/49
File chrome/browser/cocoa/web_drag_source.mm (right):

http://codereview.chromium.org/160083/diff/45/49#newcode45
Line 45: const WebDropData& drop_data, unsigned seq = 0) {
i think default arguments are forbidden by the c++ style guide. might want to
double-check.

http://codereview.chromium.org/160083/diff/45/49#newcode81
Line 81: scoped_nsobject<WebDragSource> drag_source_;
can this be removed now that you're copying the data?

http://codereview.chromium.org/160083/diff/45/49#newcode111
Line 111: } // namespace
2 spaces after brace for same-line comment

http://codereview.chromium.org/160083/diff/45/49#newcode229
Line 229: if (!cancelled) {
probably best to swap these around and make it |if (cancelled)| rather than
checking the negative.

[viettrungluu-gmail, 2009-07-30 17:09:20]

Everything addressed. Could you re^2-review (or should that be re^3-view?) and
land it for me? Thanks!.

http://codereview.chromium.org/160083/diff/45/49
File chrome/browser/cocoa/web_drag_source.mm (right):

http://codereview.chromium.org/160083/diff/45/49#newcode45
Line 45: const WebDropData& drop_data, unsigned seq = 0) {
On 2009/07/30 16:49:28, pink wrote:
> i think default arguments are forbidden by the c++ style guide. might want to
> double-check.

You're right. Changed.

http://codereview.chromium.org/160083/diff/45/49#newcode81
Line 81: scoped_nsobject<WebDragSource> drag_source_;
On 2009/07/30 16:49:28, pink wrote:
> can this be removed now that you're copying the data?

Absolutely. I don't know what it was still doing there!

http://codereview.chromium.org/160083/diff/45/49#newcode111
Line 111: } // namespace
On 2009/07/30 16:49:28, pink wrote:
> 2 spaces after brace for same-line comment

I guess that applies to other same-line comments as well....

http://codereview.chromium.org/160083/diff/45/49#newcode229
Line 229: if (!cancelled) {
On 2009/07/30 16:49:28, pink wrote:
> probably best to swap these around and make it |if (cancelled)| rather than
> checking the negative.

Done.

[viettrungluu-gmail, 2009-08-01 05:18:30]

Pushed by thakis as r22187.