Bug 116223 – Remove the incorrectly implemented and unused SQLiteStatement::getColumnBlob().

• Component: Platform
• Status: RESOLVED DUPLICATE

[Lyon Chen]

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.

[Lyon Chen]

cc: Joe Mason, Darin, Beidson.

[Lyon Chen]

Created attachment 201957 [details]
Patch for 116223.

[Joe Mason]

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.

[Lyon Chen]

(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?

[Alexey Proskuryakov]

I'd say that if SQLiteStatement::getColumnBlob() is never called, we should strongly consider removing it completely.

[Lyon Chen]

(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.

[Lyon Chen]

Created attachment 201969 [details]
Remove it instead of fixing it.

[Joe Mason]

(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?

[Joe Mason]

(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. :)

[Lyon Chen]

(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.

[Benjamin Poulain]

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? ;)

[Benjamin Poulain]

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.

[Lyon Chen]

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?

[Tim Nguyen (:ntim)]

Comment on attachment 201969 [details]
Remove it instead of fixing it.

Bug 127868 made the same change.

[Tim Nguyen (:ntim)]

*** This bug has been marked as a duplicate of bug 127868 ***