Bug 151675 - Modern IDB: "prevunique" cursors should point at the lowest primary key that matches, not the highest
Summary: Modern IDB: "prevunique" cursors should point at the lowest primary key that ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117 150882 151684
  Show dependency tree / graph
 
Reported: 2015-11-30 14:30 PST by Brady Eidson
Modified: 2015-11-30 22:23 PST (History)
5 users (show)

See Also:

Attachments
Patch v1 (22.08 KB, patch)
2015-11-30 15:43 PST, Brady Eidson 		darin: review+
beidson: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (21.67 KB, patch)
2015-11-30 16:42 PST, Brady Eidson 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2015-11-30 14:30:16 PST 
Modern IDB: "prevunique" cursors should point at the lowest primary key that matches, not the highest.

This is so that "nextunique" and "prevunique" both point at the same primary key record for an index entry.
Comment 1 Brady Eidson 2015-11-30 15:43:13 PST 
Created attachment 266291 [details]
Patch v1
Comment 2 Darin Adler 2015-11-30 16:00:30 PST 
Comment on attachment 266291 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp:222
> -    auto iterator = std::set<IDBKeyData>::reverse_iterator(m_orderedKeys->upper_bound(key));
> +
> +    std::set<IDBKeyData>::reverse_iterator iterator = std::set<IDBKeyData>::reverse_iterator(m_orderedKeys->upper_bound(key));

Why the change here? Is it useful to repeat std::set<IDBKeyData>::reverse_iterator twice?

> Source/WebCore/Modules/indexeddb/server/IndexValueEntry.h:38
> +enum class CursorDuplicity;

Is this kind of forward declaration of an enum class sufficient? If so, that’s great and I was not aware until now.
Comment 3 Brady Eidson 2015-11-30 16:39:18 PST 
(In reply to comment #2)
> Comment on attachment 266291 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266291&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp:222
> > -    auto iterator = std::set<IDBKeyData>::reverse_iterator(m_orderedKeys->upper_bound(key));
> > +
> > +    std::set<IDBKeyData>::reverse_iterator iterator = std::set<IDBKeyData>::reverse_iterator(m_orderedKeys->upper_bound(key));
> 
> Why the change here? Is it useful to repeat
> std::set<IDBKeyData>::reverse_iterator twice?

The patch was more complicated originally and required a fully-typed local variable to play with.

Then it got easier again and I forgot to make it auto.

Will-do!

> 
> > Source/WebCore/Modules/indexeddb/server/IndexValueEntry.h:38
> > +enum class CursorDuplicity;
> 
> Is this kind of forward declaration of an enum class sufficient? If so,
> that’s great and I was not aware until now.

Yes, it is! It's one of the understated great things about enum class!
Comment 4 Brady Eidson 2015-11-30 16:42:06 PST 
Created attachment 266300 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2015-11-30 17:28:27 PST 
Comment on attachment 266300 [details]
Patch for landing

Clearing flags on attachment: 266300

Committed r192847: <http://trac.webkit.org/changeset/192847>