Bug 57507 - Clean up IndexedDB WebKit API
Summary: Clean up IndexedDB WebKit API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on: 57684
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-30 14:55 PDT by Jeremy Orlow
Modified: 2011-04-06 16:34 PDT (History)
8 users (show)

See Also:


Attachments
Patch (68.74 KB, patch)
2011-03-30 14:58 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
fixed patch (68.69 KB, patch)
2011-04-04 05:59 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
manual upload (96.71 KB, patch)
2011-04-04 06:14 PDT, Hans Wennborg
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2011-03-30 14:55:30 PDT
Clean up IndexedDB WebKit API
Comment 1 Jeremy Orlow 2011-03-30 14:58:00 PDT
Created attachment 87623 [details]
Patch
Comment 2 Jeremy Orlow 2011-03-30 14:58:36 PDT
100% mechanical cleanup...no logic changes
Comment 3 David Grogan 2011-03-30 15:11:29 PDT
LGTM
Comment 4 WebKit Review Bot 2011-03-30 15:32:55 PDT
Attachment 87623 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8309019
Comment 5 WebKit Review Bot 2011-03-30 23:45:05 PDT
Attachment 87623 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8154154
Comment 6 Jeremy Orlow 2011-04-01 16:12:24 PDT
Committed r82738: <http://trac.webkit.org/changeset/82738>
Comment 7 Adam Barth 2011-04-01 17:05:58 PDT
This appears to have broken Broke storage/indexeddb/objectstore-autoincrement.html and InjectIDBKey.
Comment 8 Hans Wennborg 2011-04-04 05:55:20 PDT
Re-opening since this got reverted.
Comment 9 Hans Wennborg 2011-04-04 05:59:54 PDT
Created attachment 88053 [details]
fixed patch

Uploading patch again, with fix the should make all tests pass this time.
Comment 10 WebKit Review Bot 2011-04-04 06:05:16 PDT
Attachment 88053 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8247991
Comment 11 Hans Wennborg 2011-04-04 06:14:23 PDT
Created attachment 88054 [details]
manual upload

webkit-patch upload fail; doing it manually.
Comment 12 WebKit Review Bot 2011-04-04 06:19:40 PDT
Attachment 88053 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8185934
Comment 13 Adam Barth 2011-04-04 13:19:28 PDT
Comment on attachment 88054 [details]
manual upload

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

Thanks for picking up this change.  What was the bug with the old patch?

> Source/WebKit/chromium/ChangeLog:14
> +        Second try: added WebCore:: qualifier to
> +        injectIDBKeyIntoSerializedValue in WebIDBKey.cpp.

Usually we don't put this kind of information in ChangeLogs.  We usually put it in bug comments because it's information about how the patch was produced, not why the change is being made.

> Source/WebKit/chromium/src/WebIDBTransactionImpl.cpp:2
> - * Copyright (C) 2010 Google Inc. All rights reserved.
> + * Copyright (C) 2011 Google Inc. All rights reserved.

We probably shouldn't change just the copyright in this file.

> Source/WebKit/chromium/src/WebIDBTransactionImpl.h:2
> - * Copyright (C) 2010 Google Inc. All rights reserved.
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Or this one.
Comment 14 Hans Wennborg 2011-04-05 02:32:56 PDT
(In reply to comment #13)
> (From update of attachment 88054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88054&action=review
> 
> Thanks for picking up this change.  What was the bug with the old patch?

Jeremy added a "using namespace WebCore;" directive to WebIDBkey when he moved it into the WebKit namespace, and removed the WebCore:: qualifier from the call to injectIDBKeyIntoSerializedValue. This made it call the wrong thing, so I put the WebCore qualifier back on that call.

> 
> > Source/WebKit/chromium/ChangeLog:14
> > +        Second try: added WebCore:: qualifier to
> > +        injectIDBKeyIntoSerializedValue in WebIDBKey.cpp.
> 
> Usually we don't put this kind of information in ChangeLogs.  We usually put it in bug comments because it's information about how the patch was produced, not why the change is being made.
Ok, removed.

> 
> > Source/WebKit/chromium/src/WebIDBTransactionImpl.cpp:2
> > - * Copyright (C) 2010 Google Inc. All rights reserved.
> > + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> We probably shouldn't change just the copyright in this file.
Fixed.

> 
> > Source/WebKit/chromium/src/WebIDBTransactionImpl.h:2
> > - * Copyright (C) 2010 Google Inc. All rights reserved.
> > + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> Or this one.
Fixed.
Comment 15 Hans Wennborg 2011-04-05 02:38:31 PDT
Committed r82917: <http://trac.webkit.org/changeset/82917>
Comment 16 WebKit Review Bot 2011-04-05 03:50:50 PDT
http://trac.webkit.org/changeset/82917 might have broken GTK Linux 32-bit Debug
Comment 17 Jeremy Orlow 2011-04-06 16:34:33 PDT
Thanks Hans!  Glad we had tests.  :-)