| Summary: | IndexedDB custom bindings missing String type | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | George Staikos <staikos> | ||||||||
| Component: | JavaScriptCore | Assignee: | George Staikos <staikos> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | charles.wei, jsbell, levin, rwlbuis, tonikitoo, webkit.review.bot, yong.li.webkit, zimmermann | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
George Staikos
2012-07-01 14:22:39 PDT
Created attachment 150338 [details]
Patch to implement String
Comment on attachment 150338 [details]
Patch to implement String
LGTM.
Comment on attachment 150338 [details]
Patch to implement String
commit.
Comment on attachment 150338 [details] Patch to implement String Rejecting attachment 150338 [details] from review queue. charles.wei@torchmobile.com.cn does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights. Adding some indexeddb gurus for thoughts on this two-line patch :) Can anyone review this? It's a two line patch. Comment on attachment 150338 [details]
Patch to implement String
Doesn't this need a test? (Or is it untestable at present?)
(In reply to comment #7) > (From update of attachment 150338 [details]) > Doesn't this need a test? (Or is it untestable at present?) Not sure. Maybe Charles can comment. I'd hope the tests already are present. Is the stringToUstring call necessary? The JSIDBKeyCustom impl doesn't have it. This will be covered by existing IDB key path tests when enabled for a port. IDBAny::StringType was a recent addition and I failed to account for the JSC version when I added it. Looks good from an IDB perspective. (In reply to comment #9) > Is the stringToUstring call necessary? The JSIDBKeyCustom impl doesn't have it. > > This will be covered by existing IDB key path tests when enabled for a port. IDBAny::StringType was a recent addition and I failed to account for the JSC version when I added it. > > Looks good from an IDB perspective. Seems to be the case. Updating the patch now. Created attachment 151152 [details]
Patch to implement String
Comment on attachment 151152 [details] Patch to implement String View in context: https://bugs.webkit.org/attachment.cgi?id=151152&action=review rs=me, please modify the ChangeLog before landing: > Source/WebCore/ChangeLog:7 > + Joshuas explanation should be added to the ChangeLog. It's missing the "why" answers, as discussed recently on webkit-dev. Just include a line saying that this is covered by existing tests for ports that use JSC and have IndexDB available - at least that's how I understand it. Created attachment 151574 [details]
Patch to implement String
Comment on attachment 151574 [details] Patch to implement String Clearing flags on attachment: 151574 Committed r122295: <http://trac.webkit.org/changeset/122295> All reviewed patches have been landed. Closing bug. |