Bug 154273

Created attachment 271403 [details] Patch

Created attachment 271426 [details] Patch

Comment on attachment 271426 [details] Patch Please hold off on this. This will interfere with at least one feature patch I have in progress, and also touches "Dead-code-walking" (https://bugs.webkit.org/show_bug.cgi?id=150854) After 150584 is resolved (Soon!), this would be a great task to revisit.

(In reply to comment #3) > Comment on attachment 271426 [details] > Patch > > Please hold off on this. > > This will interfere with at least one feature patch I have in progress, and > also touches "Dead-code-walking" > (https://bugs.webkit.org/show_bug.cgi?id=150854) > > After 150584 is resolved (Soon!), this would be a great task to revisit. I see. Let me update this clean up after landing it.

Created attachment 272324 [details] Patch

Comment on attachment 272324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272324&action=review Thanks for tackling this. > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:94 > + return IDBKeyRange::create(bound.get(), 0, open ? LowerBoundOpen : LowerBoundClosed, UpperBoundOpen); This is a case where we’d like to hand over ownership, which shows that we could get better efficiency if IDBKeyRange::create took RefPtr&& arguments rather than pointers; we would use WTFMove here instead of .get(). Also, when touching this line of code, the 0 should be changed to nullptr. > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:106 > + return IDBKeyRange::create(0, bound.get(), LowerBoundOpen, open ? UpperBoundOpen : UpperBoundClosed); Ditto. > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:128 > + return IDBKeyRange::create(lower.get(), upper.get(), lowerOpen ? LowerBoundOpen : LowerBoundClosed, upperOpen ? UpperBoundOpen : UpperBoundClosed); Same thing again. > Source/WebCore/Modules/indexeddb/IDBKeyRange.h:51 > + static Ref<IDBKeyRange> create(IDBKey* lower, IDBKey* upper, LowerBoundType lowerType, UpperBoundType upperType) The argument type here should be RefPtr&&, not raw pointers. > Source/WebCore/Modules/indexeddb/IDBKeyRange.h:55 > + static Ref<IDBKeyRange> create(IDBKey* prpKey); The argument type here should be RefPtr&&, not a raw pointer. > Source/WebCore/Modules/indexeddb/IDBKeyRange.h:56 > ~IDBKeyRange() { } This line of code should be removed; does no good. > Source/WebCore/Modules/indexeddb/IDBKeyRange.h:59 > + RefPtr<IDBKey> lower() const { return m_lower; } > + RefPtr<IDBKey> upper() const { return m_upper; } The return types here should be raw pointers, not RefPtr. > Source/WebCore/Modules/indexeddb/IDBKeyRange.h:82 > + IDBKeyRange(IDBKey* lower, IDBKey* upper, LowerBoundType lowerType, UpperBoundType upperType); These arguments should be RefPtr&&, not *. > Source/WebCore/Modules/indexeddb/IDBKeyRangeData.cpp:71 > - return IDBKeyRange::create(lowerKey.maybeCreateIDBKey(), upperKey.maybeCreateIDBKey(), lowerOpen ? IDBKeyRange::LowerBoundOpen : IDBKeyRange::LowerBoundClosed, upperOpen ? IDBKeyRange::UpperBoundOpen : IDBKeyRange::UpperBoundClosed); > + return IDBKeyRange::create(lowerKey.maybeCreateIDBKey().get(), upperKey.maybeCreateIDBKey().get(), lowerOpen ? IDBKeyRange::LowerBoundOpen : IDBKeyRange::LowerBoundClosed, upperOpen ? IDBKeyRange::UpperBoundOpen : IDBKeyRange::UpperBoundClosed); This change adds reference count churn. We should not make this change. > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:314 > + return IDBKeyRange::create(idbLower.get(), idbUpper.get(), lowerBoundType, upperBoundType); This should be using WTFMove, not get().

Created attachment 272398 [details] Patch

Comment on attachment 272398 [details] Patch Attachment 272398 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/889060 Number of test failures exceeded the failure limit.

Created attachment 272401 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 272398 [details] Patch Attachment 272398 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/889061 Number of test failures exceeded the failure limit.

Created attachment 272403 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 272398 [details] Patch Attachment 272398 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/889078 Number of test failures exceeded the failure limit.

Created attachment 272404 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 272398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272398&action=review review+ assuming you fix the "double move" mistake in the IDBKeyRange::create and IDBKeyRange::only functions that is causing tests to fail > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:38 > +Ref<IDBKeyRange> IDBKeyRange::create(RefPtr<IDBKey>&& key) I don’t think it’s legal to call this function with null. Might be worth coming back here later and changing this to take Ref instead of RefPtr. > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:40 > - RefPtr<IDBKey> key = prpKey; > - return adoptRef(*new IDBKeyRange(key, key, LowerBoundClosed, UpperBoundClosed)); > + return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed)); This is wrong. Can’t modify something in two different arguments because the order is undefined. One of the two will probably show up as null. I think that’s why the tests are failing on EWS. Need something like this: RefPtr<IDBKey> upperBound = key; return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(upperBound), LowerBoundClosed, UpperBoundClosed)); > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:70 > return IDBKeyRange::create(key, key, LowerBoundClosed, UpperBoundClosed); Better to write: return create(key); Unless we are going to delete the create function above that takes only a single argument, we should use it. > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:82 > + return IDBKeyRange::create(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed); Same problem as above, but since we have an overloaded create function, I think the right way to deal with it is to just write this: return create(WTFMove(key)); Unless we are going to delete the create function above that takes only a single argument, we should use it. I also noticed that many call sites throughout this file call IDBKeyRange::create, but could just call create, since the call sites are inside IDBKeyRange member functions.

Created attachment 272441 [details] Patch

Created attachment 272446 [details] Patch

(In reply to comment #14) > Comment on attachment 272398 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272398&action=review > > review+ assuming you fix the "double move" mistake in the > IDBKeyRange::create and IDBKeyRange::only functions that is causing tests to > fail > > > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:38 > > +Ref<IDBKeyRange> IDBKeyRange::create(RefPtr<IDBKey>&& key) > > I don’t think it’s legal to call this function with null. Might be worth > coming back here later and changing this to take Ref instead of RefPtr. > > > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:40 > > - RefPtr<IDBKey> key = prpKey; > > - return adoptRef(*new IDBKeyRange(key, key, LowerBoundClosed, UpperBoundClosed)); > > + return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed)); > > This is wrong. Can’t modify something in two different arguments because the > order is undefined. One of the two will probably show up as null. I think > that’s why the tests are failing on EWS. Need something like this: > > RefPtr<IDBKey> upperBound = key; > return adoptRef(*new IDBKeyRange(WTFMove(key), WTFMove(upperBound), > LowerBoundClosed, UpperBoundClosed)); > > > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:70 > > return IDBKeyRange::create(key, key, LowerBoundClosed, UpperBoundClosed); > > Better to write: > > return create(key); > > Unless we are going to delete the create function above that takes only a > single argument, we should use it. > > > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:82 > > + return IDBKeyRange::create(WTFMove(key), WTFMove(key), LowerBoundClosed, UpperBoundClosed); > > Same problem as above, but since we have an overloaded create function, I > think the right way to deal with it is to just write this: > > return create(WTFMove(key)); > > Unless we are going to delete the create function above that takes only a > single argument, we should use it. > > I also noticed that many call sites throughout this file call > IDBKeyRange::create, but could just call create, since the call sites are > inside IDBKeyRange member functions. Yes, I noticed to miss that *key* was already passed to the create() through first WTFMove(). Fixed all. Thanks.

Comment on attachment 272446 [details] Patch Clearing flags on attachment: 272446 Committed r197306: <http://trac.webkit.org/changeset/197306>

All reviewed patches have been landed. Closing bug.