Bug 70743 - IndexedDB: Replace bool args in IDBKeyRange private methods with enum
Summary: IndexedDB: Replace bool args in IDBKeyRange private methods with enum
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-24 11:00 PDT by Joshua Bell
Modified: 2011-12-07 17:33 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.71 KB, patch)
2011-11-23 11:56 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (5.73 KB, patch)
2011-11-23 13:52 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2011-12-07 11:19 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2011-10-24 11:00:57 PDT
Per tony@chromium.org's comment in https://bugs.webkit.org/show_bug.cgi?id=70065:

WebKit style prefers using enums rather than bools for arguments.  It makes it more clear at the caller what the second param is.  Since the code was already using bools, this is just a suggestion for a future cleanup.  This would only apply to the private methods, not the idl methods.
Comment 1 Joshua Bell 2011-11-23 11:56:17 PST
Created attachment 116389 [details]
Patch
Comment 2 Joshua Bell 2011-11-23 11:57:47 PST
Trivial change to better align with WebKit style, at tony@'s suggestion.
Comment 3 WebKit Review Bot 2011-11-23 12:22:35 PST
Comment on attachment 116389 [details]
Patch

Attachment 116389 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10636028

New failing tests:
storage/indexeddb/keyrange.html
storage/indexeddb/objectstore-cursor.html
storage/indexeddb/index-cursor.html
storage/indexeddb/mozilla/indexes.html
storage/indexeddb/mozilla/cursors.html
Comment 4 Joshua Bell 2011-11-23 12:23:36 PST
Oops, that was embarrassing.
Comment 5 Joshua Bell 2011-11-23 13:52:38 PST
Created attachment 116418 [details]
Patch
Comment 6 Joshua Bell 2011-11-23 13:53:04 PST
Comment on attachment 116418 [details]
Patch

This time for real.
Comment 7 Tony Chang 2011-11-24 09:01:56 PST
Comment on attachment 116418 [details]
Patch

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

> Source/WebCore/storage/IDBKeyRange.cpp:52
> -    return IDBKeyRange::create(key, key, false, false);
> +    return IDBKeyRange::create(key, key, Closed, Closed);

This isn't much better than bools and doesn't really improve the readability of the code.  Values like MinBoundOpen, MinBoundClosed might be better (you could have separate enums for min and max).  What is meant by Open and Closed in this context?  Do you mean inclusive/exclusive or bounded/unbounded?
Comment 8 Joshua Bell 2011-11-28 10:33:20 PST
(In reply to comment #7)
> 
> This isn't much better than bools and doesn't really improve the readability of the code.  

Yeah, I didn't think so either.

> Values like MinBoundOpen, MinBoundClosed might be better (you could have separate enums for min and max).  

I'll consider that.

> What is meant by Open and Closed in this context?  Do you mean inclusive/exclusive or bounded/unbounded?

It's inclusive/exclusive, which would be better names except the spec uses Open/Closed in reference to interval terminology and I thought spec alignment was preferable.
Comment 9 Joshua Bell 2011-12-07 11:19:44 PST
Created attachment 118237 [details]
Patch
Comment 10 Joshua Bell 2011-12-07 11:20:49 PST
Still only a minimal readability improvement over the bools.
Comment 11 Tony Chang 2011-12-07 11:38:08 PST
Comment on attachment 118237 [details]
Patch

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

The ternaries aren't great, but it seems like we just have to push the enum usage further out and they will go away.

> Source/WebCore/storage/IDBKeyRange.cpp:52
> -    return IDBKeyRange::create(key, key, false, false);
> +    return IDBKeyRange::create(key, key, LowerBoundClosed, UpperBoundClosed);

This is definitely easier to read.

> Source/WebCore/storage/IDBKeyRange.cpp:55
>  PassRefPtr<IDBKeyRange> IDBKeyRange::lowerBound(PassRefPtr<IDBKey> bound, bool open, ExceptionCode& ec)

Using LowerBoundType here would be nice.  Could be a follow up change.

> Source/WebCore/storage/IDBKeyRange.cpp:65
>  PassRefPtr<IDBKeyRange> IDBKeyRange::upperBound(PassRefPtr<IDBKey> bound, bool open, ExceptionCode& ec)

And UpperBoundType here.
Comment 12 Joshua Bell 2011-12-07 11:42:45 PST
(In reply to comment #11)
> (From update of attachment 118237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118237&action=review
> 
> The ternaries aren't great, but it seems like we just have to push the enum usage further out and they will go away.

Let me pull them out into separate lines, which should improve things...

> > Source/WebCore/storage/IDBKeyRange.cpp:52
> > -    return IDBKeyRange::create(key, key, false, false);
> > +    return IDBKeyRange::create(key, key, LowerBoundClosed, UpperBoundClosed);
> 
> This is definitely easier to read.
> 
> > Source/WebCore/storage/IDBKeyRange.cpp:55
> >  PassRefPtr<IDBKeyRange> IDBKeyRange::lowerBound(PassRefPtr<IDBKey> bound, bool open, ExceptionCode& ec)
> 
> Using LowerBoundType here would be nice.  Could be a follow up change.

Unfortunately, that's a public method exposed in the IDL, with bool in the signature, so we can't change that.
 
> > Source/WebCore/storage/IDBKeyRange.cpp:65
> >  PassRefPtr<IDBKeyRange> IDBKeyRange::upperBound(PassRefPtr<IDBKey> bound, bool open, ExceptionCode& ec)
> 
> And UpperBoundType here.

Ditto.
Comment 13 Tony Chang 2011-12-07 11:44:15 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 118237 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118237&action=review
> > 
> > The ternaries aren't great, but it seems like we just have to push the enum usage further out and they will go away.
> 
> Let me pull them out into separate lines, which should improve things...

Oh, I would still keep them inline.  It's just verbose, not a big deal.
Comment 14 Joshua Bell 2011-12-07 11:45:30 PST
Comment on attachment 118237 [details]
Patch

Okay then. cq?
Comment 15 WebKit Review Bot 2011-12-07 17:33:44 PST
Comment on attachment 118237 [details]
Patch

Clearing flags on attachment: 118237

Committed r102289: <http://trac.webkit.org/changeset/102289>
Comment 16 WebKit Review Bot 2011-12-07 17:33:49 PST
All reviewed patches have been landed.  Closing bug.