Bug 106135

Summary: IndexedDB: Provide LevelDB with IDBEnv instead of Env::Default
Product: WebKit Reporter: David Grogan <dgrogan>
Component: New BugsAssignee: David Grogan <dgrogan>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dglazkov, jsbell, michael, mrobinson, peter+ews, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 106360    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description David Grogan 2013-01-04 14:58:24 PST
IndexedDB: Provide LevelDB with IDBEnv instead of Env::Default
Comment 1 David Grogan 2013-01-04 15:00:05 PST
Created attachment 181382 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-05 11:49:24 PST
Attachment 181382 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2013-01-05 11:52:32 PST
Comment on attachment 181382 [details]
Patch

Attachment 181382 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15739178
Comment 4 Peter Beverloo (cr-android ews) 2013-01-05 11:55:10 PST
Comment on attachment 181382 [details]
Patch

Attachment 181382 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15732218
Comment 5 David Grogan 2013-01-07 10:54:20 PST
Webkit's chromium deps will have to be rolled when https://codereview.chromium.org/11776009/ lands.
Comment 6 David Grogan 2013-01-08 12:12:30 PST
Created attachment 181724 [details]
Patch
Comment 7 David Grogan 2013-01-08 16:00:04 PST
Josh, could you take a look at this?
Comment 8 Joshua Bell 2013-01-08 16:34:23 PST
lgtm but implies work for other ports to ensure their leveldb copy exports IDBEnv.

CC'ing some folks working on GTK.
Comment 9 David Grogan 2013-01-08 17:12:13 PST
Tony, could you review this?

If it makes it to r+ it'll be the 75th patch of mine that you've granted review. Appreciated.
Comment 10 WebKit Review Bot 2013-01-08 18:29:49 PST
Comment on attachment 181724 [details]
Patch

Clearing flags on attachment: 181724

Committed r139143: <http://trac.webkit.org/changeset/139143>
Comment 11 WebKit Review Bot 2013-01-08 18:29:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Dimitri Glazkov (Google) 2013-01-09 10:00:32 PST
Could this patch be causing these failures on try bots?

http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/7888/steps/content_browsertests/logs/stdio
Comment 13 Dimitri Glazkov (Google) 2013-01-09 10:20:53 PST
I am pretty sure the patch is the culprit. Please let me know if you have a fix. This is blocking the WebKit roll.
Comment 14 Martin Robinson 2013-01-09 10:35:55 PST
I recently bumped against this change when getting IndexedDatabase working for the GTK+ port. If this change stays in, I think it makes sense to wrap it with #if PLATFORM(CHROMIUM), since env_idb.h seems specific to Chromium.
Comment 15 Dimitri Glazkov (Google) 2013-01-09 10:51:54 PST
Reverted r139143 for reason:

Broke Chromium content_browsertests.

Committed r139208: <http://trac.webkit.org/changeset/139208>
Comment 16 David Grogan 2013-01-11 11:41:28 PST
Created attachment 182385 [details]
Patch
Comment 17 David Grogan 2013-01-11 11:45:28 PST
Comment on attachment 182385 [details]
Patch

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

The problems found by the DCHECK on the bots are addressed in https://codereview.chromium.org/11860015/.

I couldn't get the DCHECK to trigger locally but debugging and testing via trybot leads me to believe that all should be well after that CL lands and is rolled into webkit's chromium DEPS.

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:130
> +    options.env = leveldb::IDBEnv();

This is the only difference from the original patch that got reverted.
Comment 18 David Grogan 2013-01-11 11:50:58 PST
(In reply to comment #14)
> I recently bumped against this change when getting IndexedDatabase working for the GTK+ port. If this change stays in, I think it makes sense to wrap it with #if PLATFORM(CHROMIUM), since env_idb.h seems specific to Chromium.

Agreed. Send out a patch once you've gotten this working locally. It would probably make sense to have a new method LevelDBDatabase::getEnv whose implementation contains the #ifs.
Comment 19 Tony Chang 2013-01-11 13:35:05 PST
Comment on attachment 182385 [details]
Patch

OK, make sure to roll Source/WebKit/chromium/DEPS before landing this.
Comment 20 WebKit Review Bot 2013-01-12 17:45:28 PST
Comment on attachment 182385 [details]
Patch

Clearing flags on attachment: 182385

Committed r139556: <http://trac.webkit.org/changeset/139556>
Comment 21 WebKit Review Bot 2013-01-12 17:45:33 PST
All reviewed patches have been landed.  Closing bug.