In SQLiteStatement::getColumnBlob(), it calls finalize(), prepare(), and step() unconditionally, which makes user can never get blob data out through this method. It should be changed to match method like getColumnBlobAsString() and getColumnBlobAsVector(), which assumes when m_statement is there then prepare() and step() have been called before this being called. And this will match what bindBlob() does, saving any binary data with defined length.
cc: Joe Mason, Darin, Beidson.
Created attachment 201957 [details] Patch for 116223.
Comment on attachment 201957 [details] Patch for 116223. View in context: https://bugs.webkit.org/attachment.cgi?id=201957&action=review > Source/WebCore/ChangeLog:15 > + No new tests needed as no one use SQLiteStatement::getColumnBlob() yet. Still needs a test, to make sure it's actually working when someone wants to use it.
(In reply to comment #3) > (From update of attachment 201957 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201957&action=review > > > Source/WebCore/ChangeLog:15 > > + No new tests needed as no one use SQLiteStatement::getColumnBlob() yet. > > Still needs a test, to make sure it's actually working when someone wants to use it. So it will be a unit test, instead of layout test?
I'd say that if SQLiteStatement::getColumnBlob() is never called, we should strongly consider removing it completely.
(In reply to comment #5) > I'd say that if SQLiteStatement::getColumnBlob() is never called, we should strongly consider removing it completely. But I am planning to use it, and then found the issue.
Created attachment 201969 [details] Remove it instead of fixing it.
(In reply to comment #7) > Created an attachment (id=201969) [details] > Remove it instead of fixing it. Are you planning to re-add it along with our platform specific patch once we're finished the code that uses it?
(In reply to comment #8) > (In reply to comment #7) > > Created an attachment (id=201969) [details] [details] > > Remove it instead of fixing it. > > Are you planning to re-add it along with our platform specific patch once we're finished the code that uses it? Never mind, I actually looked at the patch and I see your comment now. :)
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > Created an attachment (id=201969) [details] [details] [details] > > > Remove it instead of fixing it. > > > > Are you planning to re-add it along with our platform specific patch once we're finished the code that uses it? > > Never mind, I actually looked at the patch and I see your comment now. :) Yeah, I thought getColumnBlob name more correct than getColumnBlobAsVector, and can avoid unnecessary full copy. But then I think maybe this is not that terrible. At least IconDatabase::getImageDataForIconURLFromSQLDatabase() is doing it this way. (I think it should use SharedBuffer::adoptVector() instead of SharedBuffer::create(), but it is another story), and our password is short.
Comment on attachment 201969 [details] Remove it instead of fixing it. View in context: https://bugs.webkit.org/attachment.cgi?id=201969&action=review > Source/WebCore/ChangeLog:12 > + No new tests needed. Whaaaaat? ;)
Comment on attachment 201969 [details] Remove it instead of fixing it. Let's cq- actually: -Please change the title to reflect what is in the patch.
Comment on attachment 201969 [details] Remove it instead of fixing it. Re-set the commit-queue flag after changing the bug title to reflect the patch. Ben: do you feel the changed title is OK now?
Comment on attachment 201969 [details] Remove it instead of fixing it. Bug 127868 made the same change.
*** This bug has been marked as a duplicate of bug 127868 ***