Bug 110759 – [chromium] Flatten LazyDecodingPixelRef to SkData

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Hin-Chung Lam]

LazyDecodingPixelRef should be serialized to SkData such that it can be recorded as SKP.

Created attachment 193378 [details]
Implement onRefEncodedData

[Hin-Chung Lam]

Comment on attachment 193378 [details]
Implement onRefEncodedData

View in context: https://bugs.webkit.org/attachment.cgi?id=193378&action=review

> platform/graphics/chromium/ImageFrameGenerator.cpp:72
> +    m_data.data(data, allDataReceived);

Call m_data, make a copy and pass it out.

> platform/graphics/chromium/ImageFrameGenerator.h:68
> +    void data(SharedBuffer**, bool* allDataReceived);

Please change this to RefPtr<SharedBuffer>*, bool*. I would like to make a new copy of SharedBuffer, the data is really an internal data structure and shouldn't be given out directly.

> platform/graphics/chromium/LazyDecodingPixelRef.cpp:61
> +    SharedBuffer* buffer = 0;

Change this to RefPtr<SharedBuffer> buffer. It will catch the new object from ImageFrameGenerator.

> platform/graphics/chromium/LazyDecodingPixelRef.cpp:63
> +    m_mutex.lock();

There's no need to lock here. m_frameGenerator is always valid and it gives us a new copy of SharedBuffer.

[Hin-Chung Lam]

The patch also has a merge problem. Please merge and upload.

Created attachment 193617 [details]
patch implementing onRefEncodedData

Comment on attachment 193378 [details]
Implement onRefEncodedData

View in context: https://bugs.webkit.org/attachment.cgi?id=193378&action=review

>> platform/graphics/chromium/ImageFrameGenerator.cpp:72
>> +    m_data.data(data, allDataReceived);
> 
> Call m_data, make a copy and pass it out.

Done.

>> platform/graphics/chromium/ImageFrameGenerator.h:68
>> +    void data(SharedBuffer**, bool* allDataReceived);
> 
> Please change this to RefPtr<SharedBuffer>*, bool*. I would like to make a new copy of SharedBuffer, the data is really an internal data structure and shouldn't be given out directly.

Done

>> platform/graphics/chromium/LazyDecodingPixelRef.cpp:61
>> +    SharedBuffer* buffer = 0;
> 
> Change this to RefPtr<SharedBuffer> buffer. It will catch the new object from ImageFrameGenerator.

Done.

>> platform/graphics/chromium/LazyDecodingPixelRef.cpp:63
>> +    m_mutex.lock();
> 
> There's no need to lock here. m_frameGenerator is always valid and it gives us a new copy of SharedBuffer.

Done

[Hin-Chung Lam]

Comment on attachment 193617 [details]
patch implementing onRefEncodedData

LGTM.

Created attachment 193649 [details]
Implement onRefEncodedData.

Fixes some style violations from the last patch.

[Stephen White]

Comment on attachment 193649 [details]
Implement onRefEncodedData.

View in context: https://bugs.webkit.org/attachment.cgi?id=193649&action=review

OK.  r=me

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:70
> +void ImageFrameGenerator::data(RefPtr<SharedBuffer>* data, bool* allDataReceived)

Naming nit:  IWBN if this method had a verb in its name.  It looks kind of like a getter, but it seems to have side-effects.

I actually agree, but I named it data() to be consistent with ThreadSafeDataTransport::data(), which it calls (and to which it behaves similarly). Do you have a suggestion? getData()? (That does not make it clear that it has a potential side effect though.)

[WebKit Review Bot]

Comment on attachment 193649 [details]
Implement onRefEncodedData.

Rejecting attachment 193649 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 193649, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp
Hunk #1 FAILED at 29.
Hunk #2 succeeded at 56 with fuzz 1 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp.rej
patching file Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Stephen White']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17209444

Created attachment 193866 [details]
Implement onRefEncodedData

Changed data() to copyData() and merged with latest.

[Stephen White]

Comment on attachment 193866 [details]
Implement onRefEncodedData

Thanks for the name change.  Looks good (bots willing).  r=me

[WebKit Review Bot]

Comment on attachment 193866 [details]
Implement onRefEncodedData

Clearing flags on attachment: 193866

Committed r146228: <http://trac.webkit.org/changeset/146228>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.