The SQLitePersistentCookieStore should use a non-partial index on iOS.

The SQLitePersistentCookieStore should use a non-partial index on iOS.

I recently added the "transient" partial index to the
SQLitePersistentCookieStore. iOS 8.1 and older doesn't have access to SQLite
3.8+, so it can't use partial indices. Instead, use a full index on
SQLitePersistentCookieStore on iOS.

BUG=478346
Committed: https://crrev.com/4417ab30e74fa234a5cc0623ef7d13bc609802fc
Cr-Commit-Position: refs/heads/master@{#327130}

[erikchen, 2015-04-24 17:15:49]

erikchen@chromium.org changed reviewers:
+ rohitrao@chromium.org

[erikchen, 2015-04-24 17:15:50]

rohitrao: Please review.

[rohitrao (ping after 24h), 2015-04-24 17:22:28]

https://codereview.chromium.org/1098883005/diff/20001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1098883005/diff/20001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:994: "CREATE INDEX IF NOT
EXISTS is_transient ON cookies(persistent) "
Also here?

[erikchen, 2015-04-24 17:30:19]

rohitrao: PTAL

https://codereview.chromium.org/1098883005/diff/20001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1098883005/diff/20001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:994: "CREATE INDEX IF NOT
EXISTS is_transient ON cookies(persistent) "
On 2015/04/24 17:22:28, rohitrao wrote:
> Also here?

Done.

[rohitrao (ping after 24h), 2015-04-24 17:36:29]

lgtm

https://codereview.chromium.org/1098883005/diff/40001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1098883005/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:435: #if defined(OS_IOS)
Is it worth adding a comment saying why iOS is different, and which iOS version
is the first to support partial indexes?  (Put the comment in both places, if
you add it?)

https://codereview.chromium.org/1098883005/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:990: "CREATE INDEX IF NOT
EXISTS is_transient ON cookies(persistent) ")) {
Extra trailing whitespace in the SQL command.

[erikchen, 2015-04-24 17:42:57]

https://codereview.chromium.org/1098883005/diff/40001/content/browser/net/sql...
File content/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1098883005/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:435: #if defined(OS_IOS)
On 2015/04/24 17:36:29, rohitrao wrote:
> Is it worth adding a comment saying why iOS is different, and which iOS
version
> is the first to support partial indexes?  (Put the comment in both places, if
> you add it?)

Done.

https://codereview.chromium.org/1098883005/diff/40001/content/browser/net/sql...
content/browser/net/sqlite_persistent_cookie_store.cc:990: "CREATE INDEX IF NOT
EXISTS is_transient ON cookies(persistent) ")) {
On 2015/04/24 17:36:29, rohitrao wrote:
> Extra trailing whitespace in the SQL command.

Done.

[erikchen, 2015-04-24 20:12:55]

erikchen@chromium.org changed reviewers:
+ erikwright@chromium.org

[erikchen, 2015-04-24 20:12:55]

erikwright: Please review.

[erikwright (departed), 2015-04-27 18:54:21]

Can you just ping shess@ to confirm that there's no better way to query for the
SQLite version and/or presence of this feature? I mean, once we upgrade SQLite
on iOS this check will be incorrect...

LGTM if due diligence confirms that there is no other more palatable option.

[erikchen, 2015-04-27 19:13:47]

On 2015/04/27 18:54:21, erikwright wrote:
> Can you just ping shess@ to confirm that there's no better way to query for
the
> SQLite version and/or presence of this feature? I mean, once we upgrade SQLite
> on iOS this check will be incorrect...
> 
> LGTM if due diligence confirms that there is no other more palatable option.

I'm intentionally not querying for the availability of partial indices.

Some iOS users are already on 8.2. Most are not. If we queried for the
availability of partial indices, then some iOS users would have a database with
a partial index, and some would have one with a full-index. Then when the latter
users upgraded to iOS 8.2, they'd still have a full index, even though they were
on iOS 8.2. Then we would have many users on iOS 8.2, some with a partial index,
and some with a full. We could add more logic to upgrade users with a
non-partial index to a partial index if the feature is available, but this adds
complexity for unclear gains.

It's much simpler to have all iOS users use a non-partial index.

[erikwright (departed), 2015-04-27 19:18:25]

SGTM. LGTM confirmed.

[erikchen, 2015-04-27 19:19:06]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-04-27 19:19:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/1098883005/#ps60001
(title: "Comments from rohit round two.")

[commit-bot: I haz the power, 2015-04-27 19:19:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1098883005/60001

[commit-bot: I haz the power, 2015-04-27 21:30:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-27 21:32:11]

Patchset 4 (id:??) landed as
https://crrev.com/4417ab30e74fa234a5cc0623ef7d13bc609802fc
Cr-Commit-Position: refs/heads/master@{#327130}