LazyDecodingPixelRef should be serialized to SkData such that it can be recorded as SKP.
Created attachment 193378 [details] Implement 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. > 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.
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
Comment on attachment 193617 [details] patch implementing onRefEncodedData LGTM.
Created attachment 193649 [details] Implement onRefEncodedData. Fixes some style violations from the last patch.
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.)
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.
Comment on attachment 193866 [details] Implement onRefEncodedData Thanks for the name change. Looks good (bots willing). r=me
Comment on attachment 193866 [details] Implement onRefEncodedData Clearing flags on attachment: 193866 Committed r146228: <http://trac.webkit.org/changeset/146228>
All reviewed patches have been landed. Closing bug.