Block database access on allowDatabase instead of databaseOpenFile.

Block database access on allowDatabase instead of databaseOpenFile.

BUG=36435
TEST=Set cookie settings to ASK and open a page with web databases.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43183

[jochen (gone - plz use gerrit), 2010-03-25 17:54:08]

please review. 

requires WebKit r56548.

When you go to http://webkit.org/demos/sticky-notes/ you can check the prompt.
However, if you allow database access, it won't work (http://crbug.com/39297),
and opening database from workers is not yet completely implemented.

[michaeln, 2010-03-26 01:43:10]

http://codereview.chromium.org/1338001/diff/1/2
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1338001/diff/1/2#newcode389
chrome/browser/renderer_host/database_dispatcher_host.cc:389: content_setting =
request.WaitOnResponse();
Ouch... I'd vote to do this w/o blocking the file thread for the duration of the
prompt being up.

[michaeln, 2010-03-26 02:13:46]

> Ouch... I'd vote to do this w/o blocking the file thread for the duration
> of the prompt being up.

More than just a vote, we really should not block the FILE thread for this
purpose. Other database consumers and soon to be file api consumers shouldn't
have to wait on this modal dialog box to make progress.

[jochen (gone - plz use gerrit), 2010-03-26 08:12:15]

On 2010/03/26 02:13:46, michaeln wrote:
> > Ouch... I'd vote to do this w/o blocking the file thread for the duration
> > of the prompt being up.
> 
> More than just a vote, we really should not block the FILE thread for this
> purpose. Other database consumers and soon to be file api consumers shouldn't
> have to wait on this modal dialog box to make progress.

which thread should I block then? DB?

[jorlow, 2010-03-26 10:33:28]

Why do you necessarily need to block a thread??  If you absolutely do
(haven't looked at the CL yet) you should create a new one.

Btw, LocalStorage currently blocks the WebKit thread but that's because it's
the only thing that runs there.  I'm going to change that once there's
something else on it.

On Fri, Mar 26, 2010 at 8:12 AM, <jochen@chromium.org> wrote:

> On 2010/03/26 02:13:46, michaeln wrote:
>
>> > Ouch... I'd vote to do this w/o blocking the file thread for the
>> duration
>> > of the prompt being up.
>>
>
>  More than just a vote, we really should not block the FILE thread for this
>> purpose. Other database consumers and soon to be file api consumers
>> shouldn't
>> have to wait on this modal dialog box to make progress.
>>
>
> which thread should I block then? DB?
>
>
>
>
>
> http://codereview.chromium.org/1338001
>

[jochen (gone - plz use gerrit), 2010-03-26 10:38:10]

On 2010/03/26 10:33:28, Jeremy Orlow wrote:
> Why do you necessarily need to block a thread??  If you absolutely do
> (haven't looked at the CL yet) you should create a new one.
> 
> Btw, LocalStorage currently blocks the WebKit thread but that's because it's
> the only thing that runs there.  I'm going to change that once there's
> something else on it.
> 

basically I just copied the code from there.. I'll make it non-blocking and
resubmit the CL.

> On Fri, Mar 26, 2010 at 8:12 AM, <mailto:jochen@chromium.org> wrote:
> 
> > On 2010/03/26 02:13:46, michaeln wrote:
> >
> >> > Ouch... I'd vote to do this w/o blocking the file thread for the
> >> duration
> >> > of the prompt being up.
> >>
> >
> >  More than just a vote, we really should not block the FILE thread for this
> >> purpose. Other database consumers and soon to be file api consumers
> >> shouldn't
> >> have to wait on this modal dialog box to make progress.
> >>
> >
> > which thread should I block then? DB?
> >
> >
> >
> >
> >
> > http://codereview.chromium.org/1338001
> >
>

[michaeln, 2010-03-26 17:46:23]

> basically I just copied the code from there.. I'll make it non-blocking and
> resubmit the CL.

You can take a look at how the prompt is made non-blocking when used with either
Cookies and/or AppCaches.

[darin (slow to review), 2010-03-27 00:09:48]

http://codereview.chromium.org/1338001/diff/1/12
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/1338001/diff/1/12#newcode376
chrome/renderer/render_view.h:376: virtual bool allowDatabase(
before landing this CL, please revise WebFrameClient as mentioned here: 
https://bugs.webkit.org/show_bug.cgi?id=36595#c11

[Andrew T Wilson (Slow), 2010-03-27 01:57:01]

http://codereview.chromium.org/1338001/diff/1/3
File chrome/browser/renderer_host/database_dispatcher_host.h (right):

http://codereview.chromium.org/1338001/diff/1/3#newcode31
chrome/browser/renderer_host/database_dispatcher_host.h:31: bool
OnMessageReceived(const IPC::Message& message,
nit: don't think this line needs to change?

http://codereview.chromium.org/1338001/diff/1/6
File chrome/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/1338001/diff/1/6#newcode460
chrome/browser/worker_host/worker_process_host.cc:460:
doc->render_view_route_id(), settings_type));
I don't think this will do what you want - this will send ViewHostMsg
_ContentBlocked back to the worker.

The code in the destructor that notifies renderers of worker crashes talks
directly to the RenderViewHost (via a task posted to the UI thread). I suspect
that's a better approach.

[jochen (gone - plz use gerrit), 2010-03-29 15:43:54]

Made the dialog non-blocking again. The webkit changes darin requested are
accepted for the commit queue. Fixed the worker communication (hopefully,
there's still no way to test it), and modified the win/gtk UI to show the
additionally available information. danno@ will do the cocoa bits once this is
landed.

please review.

http://codereview.chromium.org/1338001/diff/1/2
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1338001/diff/1/2#newcode389
chrome/browser/renderer_host/database_dispatcher_host.cc:389: content_setting =
request.WaitOnResponse();
On 2010/03/26 01:43:10, michaeln wrote:
> Ouch... I'd vote to do this w/o blocking the file thread for the duration of
the
> prompt being up.

Done.

http://codereview.chromium.org/1338001/diff/1/3
File chrome/browser/renderer_host/database_dispatcher_host.h (right):

http://codereview.chromium.org/1338001/diff/1/3#newcode31
chrome/browser/renderer_host/database_dispatcher_host.h:31: bool
OnMessageReceived(const IPC::Message& message,
On 2010/03/27 01:57:01, Andrew T Wilson wrote:
> nit: don't think this line needs to change?

Done.

http://codereview.chromium.org/1338001/diff/1/6
File chrome/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/1338001/diff/1/6#newcode460
chrome/browser/worker_host/worker_process_host.cc:460:
doc->render_view_route_id(), settings_type));
On 2010/03/27 01:57:01, Andrew T Wilson wrote:
> I don't think this will do what you want - this will send ViewHostMsg
> _ContentBlocked back to the worker.
> 
> The code in the destructor that notifies renderers of worker crashes talks
> directly to the RenderViewHost (via a task posted to the UI thread). I suspect
> that's a better approach.

Done.

http://codereview.chromium.org/1338001/diff/1/12
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/1338001/diff/1/12#newcode376
chrome/renderer/render_view.h:376: virtual bool allowDatabase(
On 2010/03/27 00:09:49, darin wrote:
> before landing this CL, please revise WebFrameClient as mentioned here: 
> https://bugs.webkit.org/show_bug.cgi?id=36595#c11

Done.

[jorlow, 2010-03-29 16:21:00]

LGTM, but please fix nits and wait for others to review (especially the workers
part).

http://codereview.chromium.org/1338001/diff/15001/16008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16008#newcode118
chrome/browser/renderer_host/database_dispatcher_host.cc:118: delete message;
Good catch.

http://codereview.chromium.org/1338001/diff/15001/16015
File chrome/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16015#newcode454
chrome/browser/worker_host/worker_process_host.cc:454: if (i->worker_route_id()
== message.routing_id()) {
I'd do != and do a continue if not

http://codereview.chromium.org/1338001/diff/15001/16020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2850
chrome/renderer/render_view.cc:2850: const WebString& name,
No need to line up on the ( with so many params...just line up on 4 spaces in

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2855
chrome/renderer/render_view.cc:2855: return false;  // Uninitialized document?
Should you DCHECK this happening too?

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2858
chrome/renderer/render_view.cc:2858: if (Send(new
ViewHostMsg_AllowDatabase(routing_id_,
if (!Send...)
    return false;
if (!result)
  ...

seems a bit better to me

http://codereview.chromium.org/1338001/diff/15001/16022
File chrome/renderer/webworker_proxy.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16022#newcode90
chrome/renderer/webworker_proxy.cc:90: const WebString& name,
nit: probably should just align this on 4 spaces to fit more onto each line

http://codereview.chromium.org/1338001/diff/15001/16022#newcode99
chrome/renderer/webworker_proxy.cc:99: child_thread_->Send(new
WorkerMsg_ContentBlocked(
Is this necessary?  Seems like you could notify without another IPC round trip.

[darin (slow to review), 2010-03-29 19:22:18]

http://codereview.chromium.org/1338001/diff/15001/16008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16008#newcode118
chrome/browser/renderer_host/database_dispatcher_host.cc:118: delete message;
On 2010/03/29 16:21:00, Jeremy Orlow wrote:
> Good catch.

This looks like a double free to me.  Please see the documentation for the
PostTask method in chrome_thread.h, or look at the implementation code.

http://codereview.chromium.org/1338001/diff/15001/16020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2851
chrome/renderer/render_view.cc:2851: const WebString& displayName,
please use google style for variable names

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2855
chrome/renderer/render_view.cc:2855: return false;  // Uninitialized document?
On 2010/03/29 16:21:00, Jeremy Orlow wrote:
> Should you DCHECK this happening too?

Please see
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

In particular, if a DCHECK is warranted, then we shouldn't need a release-mode
check.  DLOG(WARNING) might be better if we want the release-mode check and we
want to alert developers.

http://codereview.chromium.org/1338001/diff/15001/16021
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/1338001/diff/15001/16021#newcode378
chrome/renderer/render_view.h:378: const WebKit::WebString& displayName,
unsigned long estimatedSize);
please use google style for variable names

http://codereview.chromium.org/1338001/diff/15001/16022
File chrome/renderer/webworker_proxy.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16022#newcode91
chrome/renderer/webworker_proxy.cc:91: const WebString& displayName,
use google style for naming parameters

http://codereview.chromium.org/1338001/diff/15001/16023
File chrome/renderer/webworker_proxy.h (right):

http://codereview.chromium.org/1338001/diff/15001/16023#newcode48
chrome/renderer/webworker_proxy.h:48: const WebKit::WebString& displayName,
use google style for naming parameters

[jochen (gone - plz use gerrit), 2010-03-29 19:32:52]

http://codereview.chromium.org/1338001/diff/15001/16008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16008#newcode118
chrome/browser/renderer_host/database_dispatcher_host.cc:118: delete message;
On 2010/03/29 19:22:18, darin wrote:
> On 2010/03/29 16:21:00, Jeremy Orlow wrote:
> > Good catch.
> 
> This looks like a double free to me.  Please see the documentation for the
> PostTask method in chrome_thread.h, or look at the implementation code.

PostTask deletes the task (NewRunnableMethod) but not its arguments (message)

[michaeln, 2010-03-29 19:47:38]

http://codereview.chromium.org/1338001/diff/15001/16014
File chrome/browser/views/database_open_info_view.h (right):

http://codereview.chromium.org/1338001/diff/15001/16014#newcode24
chrome/browser/views/database_open_info_view.h:24: class DatabaseOpenInfoView :
public views::View {
Since you're touch this class, consider porting it to use GenericInfoView and
deleting a the boiler plate code in this class that we've replicated in the
various info views.

[Andrew T Wilson (Slow), 2010-03-30 00:35:17]

http://codereview.chromium.org/1338001/diff/15001/16015
File chrome/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16015#newcode453
chrome/browser/worker_host/worker_process_host.cc:453: for (Instances::iterator
i = instances_.begin(); i != instances_.end();) {
I'm looking for a ++i somewhere in this loop, but I don't see one. Is it an
infinite loop?

http://codereview.chromium.org/1338001/diff/15001/16022
File chrome/renderer/webworker_proxy.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16022#newcode58
chrome/renderer/webworker_proxy.cc:58: route_id_ = MSG_ROUTING_NONE;
This should not be necessary, as it's already done as part of Disconnect(). In
fact, doing it here will break Disconnect() since it relies on having a valid
route_id_.

http://codereview.chromium.org/1338001/diff/15001/16022#newcode100
chrome/renderer/webworker_proxy.cc:100: route_id_,
CONTENT_SETTINGS_TYPE_COOKIES));
This change shouldn't be here (see my comment in webworker_proxy.h), but instead
should be moved to worker/webworkerclient_proxy.cc (implementing the
WebKit::WebWorkerClient interface).

http://codereview.chromium.org/1338001/diff/15001/16023
File chrome/renderer/webworker_proxy.h (right):

http://codereview.chromium.org/1338001/diff/15001/16023#newcode49
chrome/renderer/webworker_proxy.h:49: unsigned long estimatedSize);
This is the wrong place to override allowDatabase() - we should make the changes
described here:

https://bugs.webkit.org/show_bug.cgi?id=36795

Then implement allowDatabase() in chrome/worker/webworkerclient_proxy.cc.

I would recommend finding some way to test this plumbing, perhaps by just
hacking the code to call allowDatabase() directly so you can at least manually
test the plumbing in advance of the worker API being exposed.

[jorlow, 2010-03-30 10:13:16]

http://codereview.chromium.org/1338001/diff/15001/16014
File chrome/browser/views/database_open_info_view.h (right):

http://codereview.chromium.org/1338001/diff/15001/16014#newcode24
chrome/browser/views/database_open_info_view.h:24: class DatabaseOpenInfoView :
public views::View {
On 2010/03/29 19:47:38, michaeln wrote:
> Since you're touch this class, consider porting it to use GenericInfoView and
> deleting a the boiler plate code in this class that we've replicated in the
> various info views.

How big would such a change be?  If it's not trivial, I'd suggest this is follow
up patch type work.

[jochen (gone - plz use gerrit), 2010-03-30 12:48:23]

I've stripped out the worker support for now. For MStone5, Workers don't support
web databases, and I'd rather wait until I have a better idea of what's going on
and how to actually test this for workers.

http://codereview.chromium.org/1338001/diff/15001/16014
File chrome/browser/views/database_open_info_view.h (right):

http://codereview.chromium.org/1338001/diff/15001/16014#newcode24
chrome/browser/views/database_open_info_view.h:24: class DatabaseOpenInfoView :
public views::View {
On 2010/03/29 19:47:38, michaeln wrote:
> Since you're touch this class, consider porting it to use GenericInfoView and
> deleting a the boiler plate code in this class that we've replicated in the
> various info views.

Done.

http://codereview.chromium.org/1338001/diff/15001/16020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2850
chrome/renderer/render_view.cc:2850: const WebString& name,
On 2010/03/29 16:21:00, Jeremy Orlow wrote:
> No need to line up on the ( with so many params...just line up on 4 spaces in

Done.

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2851
chrome/renderer/render_view.cc:2851: const WebString& displayName,
On 2010/03/29 19:22:18, darin wrote:
> please use google style for variable names

Done.

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2855
chrome/renderer/render_view.cc:2855: return false;  // Uninitialized document?

On 2010/03/29 19:22:18, darin wrote:
> On 2010/03/29 16:21:00, Jeremy Orlow wrote:
> > Should you DCHECK this happening too?
> 
> Please see
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
> 
> In particular, if a DCHECK is warranted, then we shouldn't need a release-mode
> check.  DLOG(WARNING) might be better if we want the release-mode check and we
> want to alert developers.

I basically copied this from allowScript.

http://codereview.chromium.org/1338001/diff/15001/16020#newcode2858
chrome/renderer/render_view.cc:2858: if (Send(new
ViewHostMsg_AllowDatabase(routing_id_,
On 2010/03/29 16:21:00, Jeremy Orlow wrote:
> if (!Send...)
>     return false;
> if (!result)
>   ...
> 
> seems a bit better to me

Done.

http://codereview.chromium.org/1338001/diff/15001/16021
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/1338001/diff/15001/16021#newcode378
chrome/renderer/render_view.h:378: const WebKit::WebString& displayName,
unsigned long estimatedSize);
On 2010/03/29 19:22:18, darin wrote:
> please use google style for variable names

Done.

[michaeln, 2010-03-30 19:08:13]

http://codereview.chromium.org/1338001/diff/23002/30014
File chrome/browser/views/database_open_info_view.h (right):

http://codereview.chromium.org/1338001/diff/23002/30014#newcode16
chrome/browser/views/database_open_info_view.h:16: class DatabaseOpenInfoView :
public GenericInfoView {
Thnx!

[michaeln, 2010-03-30 19:46:19]

LGTM

http://codereview.chromium.org/1338001/diff/23002/30008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1338001/diff/23002/30008#newcode353
chrome/browser/renderer_host/database_dispatcher_host.cc:353: void
DatabaseDispatcherHost::OnAllowDatabase(const std::string& origin,
Since every other call here uses 'origin_identifier' formatted strings... maybe
name the 'origin' parameter 'origin_url' to be just a little more clear that its
not one of these identifier strings (here and in the .h file and in messages.h
file).

http://codereview.chromium.org/1338001/diff/23002/30015
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/1338001/diff/23002/30015#newcode2124
chrome/common/render_messages_internal.h:2124: std::string /* origin */,
origin_url

[jochen (gone - plz use gerrit), 2010-03-31 07:36:34]

http://codereview.chromium.org/1338001/diff/23002/30008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/1338001/diff/23002/30008#newcode353
chrome/browser/renderer_host/database_dispatcher_host.cc:353: void
DatabaseDispatcherHost::OnAllowDatabase(const std::string& origin,
On 2010/03/30 19:46:19, michaeln wrote:
> Since every other call here uses 'origin_identifier' formatted strings...
maybe
> name the 'origin' parameter 'origin_url' to be just a little more clear that
its
> not one of these identifier strings (here and in the .h file and in messages.h
> file).

Done.