Bug 91161 - IndexedDB: Re-enable indexeddb in test_shell
Summary: IndexedDB: Re-enable indexeddb in test_shell
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-12 16:17 PDT by David Grogan
Modified: 2012-07-13 12:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.07 KB, patch)
2012-07-12 16:25 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (2.51 KB, patch)
2012-07-12 17:03 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (2.55 KB, patch)
2012-07-13 12:18 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-07-12 16:17:44 PDT
IndexedDB: Re-enable indexeddb in test_shell
Comment 1 David Grogan 2012-07-12 16:25:44 PDT
Created attachment 152091 [details]
Patch
Comment 2 David Grogan 2012-07-12 16:26:50 PDT
Josh, could you take a look?
Comment 3 Joshua Bell 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".
Comment 4 David Grogan 2012-07-12 17:03:37 PDT
Created attachment 152101 [details]
Patch
Comment 5 David Grogan 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.
Comment 6 Tony Chang 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?
Comment 7 David Grogan 2012-07-13 12:18:02 PDT
Created attachment 152316 [details]
Patch for landing
Comment 8 David Grogan 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
Comment 9 WebKit Review Bot 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>
Comment 10 WebKit Review Bot 2012-07-13 12:54:54 PDT
All reviewed patches have been landed.  Closing bug.