Bug 70743 – IndexedDB: Replace bool args in IDBKeyRange private methods with enum

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

Per [e-mail]'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.

[Joshua Bell]

Created attachment 116389 [details]
Patch

[Joshua Bell]

Trivial change to better align with WebKit style, at tony@'s suggestion.

[WebKit Review Bot]

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

[Joshua Bell]

Oops, that was embarrassing.

[Joshua Bell]

Created attachment 116418 [details]
Patch

[Joshua Bell]

Comment on attachment 116418 [details]
Patch

This time for real.

[Tony Chang]

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?

[Joshua Bell]

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

[Joshua Bell]

Created attachment 118237 [details]
Patch

[Joshua Bell]

Still only a minimal readability improvement over the bools.

[Tony Chang]

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.

[Joshua Bell]

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

[Tony Chang]

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

[Joshua Bell]

Comment on attachment 118237 [details]
Patch

Okay then. cq?

[WebKit Review Bot]

Comment on attachment 118237 [details]
Patch

Clearing flags on attachment: 118237

Committed r102289: <http://trac.webkit.org/changeset/102289>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.