Fix leak in ScopedClipboardWriterGlue.

Fix leak in ScopedClipboardWriterGlue.

BUG=123117
TEST=none


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

[dcheng, 2012-04-13 01:29:33]

I changed the member to just be a scoped_ptr. I thought about moving the
SharedMemory handling into ScopedClipboardWriterGlue so that all the clipboard
state could be kept together in ScopedClipboardWriterGlue. However that requires
plumbing ChildThread::AllocateSharedMemory() through ClipboardWriteContext,
which seems a little out of place. Let me know what you think.

[dcheng, 2012-04-13 22:38:25]

On 2012/04/13 01:29:33, dcheng wrote:
> I changed the member to just be a scoped_ptr. I thought about moving the
> SharedMemory handling into ScopedClipboardWriterGlue so that all the clipboard
> state could be kept together in ScopedClipboardWriterGlue. However that
requires
> plumbing ChildThread::AllocateSharedMemory() through ClipboardWriteContext,
> which seems a little out of place. Let me know what you think.

I /think/ that the try job failures are unrelated. If you prefer though, it's
fairly easy to add a Dispose() or Destroy() method to ClipboardWriteContext;
then we can use scoped_generic_object.h to destroy it when it goes out of scope.
What do you think?

[piman, 2012-04-13 22:47:06]

On 2012/04/13 22:38:25, dcheng wrote:
> On 2012/04/13 01:29:33, dcheng wrote:
> > I changed the member to just be a scoped_ptr. I thought about moving the
> > SharedMemory handling into ScopedClipboardWriterGlue so that all the
clipboard
> > state could be kept together in ScopedClipboardWriterGlue. However that
> requires
> > plumbing ChildThread::AllocateSharedMemory() through ClipboardWriteContext,
> > which seems a little out of place. Let me know what you think.
> 
> I /think/ that the try job failures are unrelated. If you prefer though, it's
> fairly easy to add a Dispose() or Destroy() method to ClipboardWriteContext;
> then we can use scoped_generic_object.h to destroy it when it goes out of
scope.
> What do you think?

I think they are unrelated indeed. I'm fine with this as is (with the fix to
webkit/tools/test_shell/simple_clipboard_impl.cc as discussed).

[dcheng, 2012-04-13 22:55:35]

On 2012/04/13 22:47:06, piman wrote:
> On 2012/04/13 22:38:25, dcheng wrote:
> > On 2012/04/13 01:29:33, dcheng wrote:
> > > I changed the member to just be a scoped_ptr. I thought about moving the
> > > SharedMemory handling into ScopedClipboardWriterGlue so that all the
> clipboard
> > > state could be kept together in ScopedClipboardWriterGlue. However that
> > requires
> > > plumbing ChildThread::AllocateSharedMemory() through
ClipboardWriteContext,
> > > which seems a little out of place. Let me know what you think.
> > 
> > I /think/ that the try job failures are unrelated. If you prefer though,
it's
> > fairly easy to add a Dispose() or Destroy() method to ClipboardWriteContext;
> > then we can use scoped_generic_object.h to destroy it when it goes out of
> scope.
> > What do you think?
> 
> I think they are unrelated indeed. I'm fine with this as is (with the fix to
> webkit/tools/test_shell/simple_clipboard_impl.cc as discussed).

I don't think simple_clipboard_impl needs an update, as we just return NULL when
asked to create a context there.

[piman, 2012-04-14 00:16:13]

I see, OK.
LGTM

[commit-bot: I haz the power, 2012-04-14 00:18:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10078001/1

[commit-bot: I haz the power, 2012-04-14 00:18:29]

Presubmit check for 10078001-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

[dcheng, 2012-04-16 18:05:15]

+tony for OWNERS approval.

[dcheng, 2012-04-25 00:05:13]

+darin for OWNERS approval.

[darin (slow to review), 2012-04-25 06:35:11]

OK, LGTM

[commit-bot: I haz the power, 2012-04-25 09:17:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10078001/1

[commit-bot: I haz the power, 2012-04-25 10:31:04]

Change committed as 133890