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.
Created attachment 116389 [details] Patch
Trivial change to better align with WebKit style, at tony@'s suggestion.
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
Oops, that was embarrassing.
Created attachment 116418 [details] Patch
Comment on attachment 116418 [details] Patch This time for real.
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?
(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.
Created attachment 118237 [details] Patch
Still only a minimal readability improvement over the bools.
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.
(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.
(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 on attachment 118237 [details] Patch Okay then. cq?
Comment on attachment 118237 [details] Patch Clearing flags on attachment: 118237 Committed r102289: <http://trac.webkit.org/changeset/102289>
All reviewed patches have been landed. Closing bug.