Bug 91161

Summary: IndexedDB: Re-enable indexeddb in test_shell
Product: WebKit Reporter: David Grogan <dgrogan>
Component: New BugsAssignee: David Grogan <dgrogan>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, jsbell, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

David Grogan
Reported 2012-07-12 16:17:44 PDT
IndexedDB: Re-enable indexeddb in test_shell
Attachments
Patch (2.07 KB, patch)
2012-07-12 16:25 PDT, David Grogan 		no flags
DetailsFormatted DiffDiff
Patch (2.51 KB, patch)
2012-07-12 17:03 PDT, David Grogan 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.55 KB, patch)
2012-07-13 12:18 PDT, David Grogan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
David Grogan
Comment 1 2012-07-12 16:25:44 PDT
Created attachment 152091 [details] Patch
David Grogan
Comment 2 2012-07-12 16:26:50 PDT
Josh, could you take a look?
Joshua Bell
Comment 3 2012-07-12 16:38:20 PDT
Oops, thanks. LGTM. I misread the boolean logic when making the patch in 90310. It wasn't intentional. I think you can dispense with the comment. I was reading the logic as "must have a permission client, and permission client must say okay" since the whole thing was inside an if (...) { error } structure. This patch makes it clearer: "allowed if either no permission client, or permission client says okay".
David Grogan
Comment 4 2012-07-12 17:03:37 PDT
Created attachment 152101 [details] Patch
David Grogan
Comment 5 2012-07-12 17:17:33 PDT
Tony, could you review this? Josh, good to know it was accidental. I left the comment in because it's otherwise hard to know the effect of us erring on the side of permissiveness there.
Tony Chang
Comment 6 2012-07-13 10:35:17 PDT
Comment on attachment 152101 [details] Patch OK since it's fixing a regression. It seems weird to me that we are allowed to run if there's no permissionClient. What does content_shell do? Would it be better to provide a mock permission client in test_shell/content_shell?
David Grogan
Comment 7 2012-07-13 12:18:02 PDT
Created attachment 152316 [details] Patch for landing
David Grogan
Comment 8 2012-07-13 12:20:49 PDT
(In reply to comment #6) > (From update of attachment 152101 [details]) > OK since it's fixing a regression. > > It seems weird to me that we are allowed to run if there's no permissionClient. What does content_shell do? Would it be better to provide a mock permission client in test_shell/content_shell? Agreed. content_shell also returns NULL. Bug filed for mock permission client: http://crbug.com/137269
WebKit Review Bot
Comment 9 2012-07-13 12:54:50 PDT
Comment on attachment 152316 [details] Patch for landing Clearing flags on attachment: 152316 Committed r122622: <http://trac.webkit.org/changeset/122622>
WebKit Review Bot
Comment 10 2012-07-13 12:54:54 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.