Summary: | IndexedDB: Provide LevelDB with IDBEnv instead of Env::Default | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||||
Component: | New Bugs | Assignee: | 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
David Grogan
2013-01-04 14:58:24 PST
Created attachment 181382 [details]
Patch
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 on attachment 181382 [details] Patch Attachment 181382 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15739178 Comment on attachment 181382 [details] Patch Attachment 181382 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15732218 Webkit's chromium deps will have to be rolled when https://codereview.chromium.org/11776009/ lands. Created attachment 181724 [details]
Patch
Josh, could you take a look at this? lgtm but implies work for other ports to ensure their leveldb copy exports IDBEnv. CC'ing some folks working on GTK. 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 on attachment 181724 [details] Patch Clearing flags on attachment: 181724 Committed r139143: <http://trac.webkit.org/changeset/139143> All reviewed patches have been landed. Closing bug. 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 I am pretty sure the patch is the culprit. Please let me know if you have a fix. This is blocking the WebKit roll. 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. Reverted r139143 for reason: Broke Chromium content_browsertests. Committed r139208: <http://trac.webkit.org/changeset/139208> Created attachment 182385 [details]
Patch
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. (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 on attachment 182385 [details]
Patch
OK, make sure to roll Source/WebKit/chromium/DEPS before landing this.
Comment on attachment 182385 [details] Patch Clearing flags on attachment: 182385 Committed r139556: <http://trac.webkit.org/changeset/139556> All reviewed patches have been landed. Closing bug. |