IndexedDB: Refactor cursor iteration to remove duplicate code
Created attachment 135871 [details] Patch
dgrogan@ mind taking a look - this is in preparation for my first actual fix..
Comment on attachment 135871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135871&action=review > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1018 > + bool checkCurrentBoundaries() const; Boundaries don't change, so is "Current" needed? Maybe rename to "isInBounds"? But it also only checks the end boundary - can that be captured in the name? > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1042 > + // in the very first seek, we're already in the right I don't think this comment is necessary; the behavior it's describing is implied by the code. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1060 > + // check if we've advanced past the range Unnecessary comment, if the method name can be clarified. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1064 > + // check if haven't yet *entered* the range Nit: Capitalization, grammar, and punctuation. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1079 > + // we'll need more information from the current row to continue Looking at loadCurrentRow(), it returns false if the row fails to load. So the behavior is to keep iterating if the data is invalid. Can this comment be improved? > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1102 > +// return true if we're still within the upper/lower bounds of this range Don't need this comment. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1107 > + // high key not included in range Nit: capitalize, add period. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1116 > + // low key not included in range Nit: capitalize, add period.
Created attachment 135896 [details] Patch
Comment on attachment 135871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135871&action=review >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1018 >> + bool checkCurrentBoundaries() const; > > Boundaries don't change, so is "Current" needed? Maybe rename to "isInBounds"? But it also only checks the end boundary - can that be captured in the name? I reversed it to isPastBounds() and added a clarifying comment (hoping that will make it by the webkit guidelines) >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1079 >> + // we'll need more information from the current row to continue > > Looking at loadCurrentRow(), it returns false if the row fails to load. So the behavior is to keep iterating if the data is invalid. Can this comment be improved? The semantics of this aren't great, but this is what was there before - It looks like it's not really fatal, because sometimes indexes have stale entries and you just have to keep iterating until you find a non-stale entry.
LGTM
tony@ - r? cq?
Comment on attachment 135896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135896&action=review > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1034 > + return continueFunction(0, true); Please declare and use an enum rather than a bool. There are other examples of this in the code base.
Created attachment 136042 [details] Patch
Comments addressed, enum added. tony@ - r? cq?
Comment on attachment 136042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136042&action=review > Source/WebCore/Modules/indexeddb/IDBBackingStore.h:105 > + Nit: Extra newline? > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:983 > + virtual bool continueFunction(const IDBKey*, IteratorState nextState); Remove the argument name |nextState|. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1048 > + } else > + nextState = Seek; Why do we set nextState here? It doesn't look like we use it later on. > Tools/Scripts/webkitpy/style/checkers/cpp.py:1565 > + print "parameter: ", canonical_parameter_name > + print "Acronym: ", acronym > + print "In text: ", canonical_text Did you mean to include this change?
Created attachment 136132 [details] Patch
Comment on attachment 136042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136042&action=review >> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:105 >> + > > Nit: Extra newline? Done. >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:983 >> + virtual bool continueFunction(const IDBKey*, IteratorState nextState); > > Remove the argument name |nextState|. Done. >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1048 >> + nextState = Seek; > > Why do we set nextState here? It doesn't look like we use it later on. Added a comment to explain - it's for the next iteration >> Tools/Scripts/webkitpy/style/checkers/cpp.py:1565 >> + print "In text: ", canonical_text > > Did you mean to include this change? oops! good ol' -a flag :) Removed.
Comments addressed tony@ - r? cq?
tony@ - ping? r? cq?
oops.. tony is on vacation ojan@ - r? cr?
Comment on attachment 136132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136132&action=review Mostly a bunch of style nits. I know some of this is copy-pasted from the old code, but I think deleting comments is a small enough change from the old code that it's OK. :) > Source/WebCore/Modules/indexeddb/IDBBackingStore.h:93 > + Ready = 0, // Already in position, don't advance. > + Seek // Seek into position before use. Nit: Typically we don't include "what" comments in WebKit code. More "why" comments, e.g. an explanation of why we're doing something non-obvious. "what" comments tend to get stale and inaccurate and can usually be easily understood by reading the code. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1060 > + // Make sure we have *entered* the range. Instead of adding a comment, you could add a helper function, e.g., haveEnteredRange. Then this code would be: if (!haveEnteredRange(...)) continue; Which is self-describing. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1063 > + // lowKey not included in the range. Ditto re "what" comments here and below. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1076 > + // The row may not load because there's a stale entry in the > + // index. This is not fatal. Good "why" comment. :) > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1100 > +// Returns true if the cursor has gone beyond the bounds, but only in > +// the direction of the iterator. "what" comment. > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1119 > + // High key not included in range. > + if (compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) >= 0) > + return true; > + } else { > + if (compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) > 0) > + return true; > + } > + } else { > + if (m_cursorOptions.lowOpen) { > + // Low key not included in range. > + if (compareIndexKeys(m_iterator->key(), m_cursorOptions.lowKey) <= 0) > + return true; > + } else { > + if (compareIndexKeys(m_iterator->key(), m_cursorOptions.lowKey) < 0) > + return true; This is a stylistic thing, but I'd prefer if you returned the comparisons instead of true, e.g., if (m_cursorOptions.forward) { if (m_cursorPoptions.highOpen) return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) >= 0; else return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) > 0; } else { etc. Makes the logic a bit more clear (and concise) since you don't have to keep track of what happens after each return.
Created attachment 136934 [details] Patch
Comment on attachment 136132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136132&action=review >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1060 >> + // Make sure we have *entered* the range. > > Instead of adding a comment, you could add a helper function, e.g., haveEnteredRange. Then this code would be: > if (!haveEnteredRange(...)) > continue; > > Which is self-describing. that is nicer. done. >> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1119 >> + return true; > > This is a stylistic thing, but I'd prefer if you returned the comparisons instead of true, e.g., > > if (m_cursorOptions.forward) { > if (m_cursorPoptions.highOpen) > return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) >= 0; > else > return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) > 0; > } else { > > etc. > > Makes the logic a bit more clear (and concise) since you don't have to keep track of what happens after each return. Not sure I agree with the clarity, but I've made the change and applied the same style in the new haveEnteredRange() All other comments addressed... new patch coming up...
ojan@ can you take another shot at this one r? cq?
Comment on attachment 136934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136934&action=review Nice cleanup! > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1099 > + > + return compareIndexKeys(m_iterator->key(), m_cursorOptions.lowKey) >= 0; > + } > + if (m_cursorOptions.highOpen) > + return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) < 0; > + > + return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) <= 0; nit: typically, webkit code wouldn't have these extra line-breaks. there's no official style here though.
Comment on attachment 136934 [details] Patch Clearing flags on attachment: 136934 Committed r114041: <http://trac.webkit.org/changeset/114041>
All reviewed patches have been landed. Closing bug.