Bug 80802 - Rename OptionsObject to Dictionary
Summary: Rename OptionsObject to Dictionary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on: 80207
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-11 19:15 PDT by Kentaro Hara
Modified: 2012-03-13 01:50 PDT (History)
10 users (show)

See Also:


Attachments
Patch (60.46 KB, patch)
2012-03-11 19:23 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (74.16 KB, patch)
2012-03-12 23:03 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-03-11 19:15:16 PDT
For naming consistency with JSDictionary.{h,cpp}, we can rename OptionObject.{h,cpp} with V8Dictionary.{h,cpp}.
Comment 1 Kentaro Hara 2012-03-11 19:23:15 PDT
Created attachment 131272 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2012-03-11 19:59:22 PDT
Comment on attachment 131272 [details]
Patch

Attachment 131272 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11936673
Comment 3 Build Bot 2012-03-11 21:21:11 PDT
Comment on attachment 131272 [details]
Patch

Attachment 131272 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11940684
Comment 4 Kentaro Hara 2012-03-12 01:58:04 PDT
Comment on attachment 131272 [details]
Patch

I found this renaming is wrong. As you can see in IDBObjectStore.h and IDBKeyRange.h, WebCore is expecting that OptionsObject.h exists both in JSC and in V8. OptionsObject.h should be a common interface between JSC and V8. So the right fix would be

- to implement bindings/js/OptionsObject.h so that it has the same interface as bindings/v8/OptionsObject.h. bindings/js/OptionsObject.h would be just a wrapper of JSDictionary.
- to rename OptionsObject.{h,cpp} to Dictionary.{h,cpp}, for clarification.

The reason why IDBObjectStore.h and IDBKeyRange.h are not currently causing any build errors is that IndexedDB is enabled in Chromium/V8 only.
Comment 5 Ryosuke Niwa 2012-03-12 08:57:17 PDT
(In reply to comment #4)
> (From update of attachment 131272 [details])
> I found this renaming is wrong. As you can see in IDBObjectStore.h and IDBKeyRange.h, WebCore is expecting that OptionsObject.h exists both in JSC and in V8. OptionsObject.h should be a common interface between JSC and V8. So the right fix would be

Can we instead call it "Dictionary.h"? OptionsObject is a bad name for a generic object like this one because it can be used for things other than options.
Comment 6 Joshua Bell 2012-03-12 13:39:42 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 131272 [details] [details])
> > I found this renaming is wrong. As you can see in IDBObjectStore.h and IDBKeyRange.h, WebCore is expecting that OptionsObject.h exists both in JSC and in V8. OptionsObject.h should be a common interface between JSC and V8. So the right fix would be
> 
> Can we instead call it "Dictionary.h"? OptionsObject is a bad name for a generic object like this one because it can be used for things other than options.

FYI: https://bugs.webkit.org/show_bug.cgi?id=80207 adds an empty implementation of OptionsObject (patches in that bug are trying to get IDB to compile w/ JSC)

+1 to a name evocative of WebIDL/Dictionary.
Comment 7 Kentaro Hara 2012-03-12 17:14:34 PDT
(In reply to comment #6)
> > Can we instead call it "Dictionary.h"? OptionsObject is a bad name for a generic object like this one because it can be used for things other than options.
> 
> FYI: https://bugs.webkit.org/show_bug.cgi?id=80207 adds an empty implementation of OptionsObject (patches in that bug are trying to get IDB to compile w/ JSC)

OK, please land the patch for 80207 first, to avoid conflict.
Comment 8 Joshua Bell 2012-03-12 17:16:07 PDT
Adding benjamin@ who is working on 80207
Comment 9 Kentaro Hara 2012-03-12 23:03:21 PDT
Created attachment 131552 [details]
Patch
Comment 10 WebKit Review Bot 2012-03-12 23:06:56 PDT
Attachment 131552 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1421:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Kentaro Hara 2012-03-13 01:49:48 PDT
Ews-bots look fine. Committed manually to avoid style check errors.