Indicate in the tab UI if appcache creation was blocked by privacy settings.

Indicate in the tab UI if appcache creation was blocked by privacy settings.

TEST=manual
BUG=38362
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44079

[jochen (gone - plz use gerrit), 2010-03-31 13:31:15]

WebKit part is in https://bugs.webkit.org/show_bug.cgi?id=36882

please review

[michaeln, 2010-03-31 23:03:56]

http://codereview.chromium.org/1600002/diff/3001/4004
File chrome/common/appcache/appcache_backend_proxy.h (right):

http://codereview.chromium.org/1600002/diff/3001/4004#newcode31
chrome/common/appcache/appcache_backend_proxy.h:31: virtual void
ContentBlocked(int render_view_id);
Per other comments, i think this can be removed.

http://codereview.chromium.org/1600002/diff/3001/4009
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/1600002/diff/3001/4009#newcode71
chrome/renderer/renderer_webkitclient_impl.h:71: WebKit::WebDataSource*,
WebKit::WebApplicationCacheHostClient*);
This needs to be synced up with our latest plans for the webkit API.

http://codereview.chromium.org/1600002/diff/3001/4013
File webkit/appcache/appcache_group.h (right):

http://codereview.chromium.org/1600002/diff/3001/4013#newcode33
webkit/appcache/appcache_group.h:33: virtual void OnContentBlocked() = 0;
For symmetry with OnUpdateCompelete, include an AppCacheGroup* parameter. Also,
can you add a comment about when this is called.

http://codereview.chromium.org/1600002/diff/3001/4013#newcode110
webkit/appcache/appcache_group.h:110: void ContentBlocked();
maybe NotifyContentBlocked()

http://codereview.chromium.org/1600002/diff/3001/4016
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/1600002/diff/3001/4016#newcode102
webkit/appcache/appcache_host.h:102: // AppCacheGroup::UpdateObserver method
Please keep these in the private section as they really aren't intended to be
part of the AppCacheHost interface.

http://codereview.chromium.org/1600002/diff/3001/4018
File webkit/appcache/appcache_interfaces.h (right):

http://codereview.chromium.org/1600002/diff/3001/4018#newcode74
webkit/appcache/appcache_interfaces.h:74: virtual void ContentBlocked(int
render_view_id) = 0;
Per other comments, i think this could be removed.

http://codereview.chromium.org/1600002/diff/3001/4019
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/1600002/diff/3001/4019#newcode198
webkit/appcache/appcache_request_handler.cc:198: host_->OnContentBlocked();
I see you're using the UpdateObserver method here too. That overload is a bit
sneaky.

Here we could use...
  host_->frontend()->OnContentBlocked(host_->host_id());

Also notice that at this point, the document associated with this appcache is
not yet committed to its Frame, its still the provisional document at this time.
We don't want to set icon until after our document has been committed to the
frame.

We could handle that on this side or on the renderer side... by deferring things
until 'selectAppCache' time, the doc is committed by then.

http://codereview.chromium.org/1600002/diff/3001/4026
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/1600002/diff/3001/4026#newcode226
webkit/appcache/appcache_update_job.cc:226: group_->ContentBlocked();
NotifyContentBlocked()

http://codereview.chromium.org/1600002/diff/3001/4031
File webkit/appcache/web_application_cache_host_impl.h (right):

http://codereview.chromium.org/1600002/diff/3001/4031#newcode36
webkit/appcache/web_application_cache_host_impl.h:36: void OnContentBlocked();
I think it may be time to introduce process type specific subclasses of this
class, and to make the OnContentBlocked method virtual (with an empty impl here)
so those derivatives can do something appropriate depending. What action is
taken in a worker will have to be different.

If we go this route we don't need the render_view_id in this class at all (to
have it in the webkit/appcache library is really a layering violation). We would
need that data member it in a RendererWebAppliationCacheHost subclass, and its
impl of OnContentBlocked() could use an already existing IPC to get the icon to
show up in the UI.

wdyt?

[jochen (gone - plz use gerrit), 2010-04-07 15:34:28]

please review.

http://codereview.chromium.org/1600002/diff/3001/4004
File chrome/common/appcache/appcache_backend_proxy.h (right):

http://codereview.chromium.org/1600002/diff/3001/4004#newcode31
chrome/common/appcache/appcache_backend_proxy.h:31: virtual void
ContentBlocked(int render_view_id);
On 2010/03/31 23:03:56, michaeln wrote:
> Per other comments, i think this can be removed.

Done.

http://codereview.chromium.org/1600002/diff/3001/4009
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/1600002/diff/3001/4009#newcode71
chrome/renderer/renderer_webkitclient_impl.h:71: WebKit::WebDataSource*,
WebKit::WebApplicationCacheHostClient*);
On 2010/03/31 23:03:56, michaeln wrote:
> This needs to be synced up with our latest plans for the webkit API.

Done.

http://codereview.chromium.org/1600002/diff/3001/4013
File webkit/appcache/appcache_group.h (right):

http://codereview.chromium.org/1600002/diff/3001/4013#newcode33
webkit/appcache/appcache_group.h:33: virtual void OnContentBlocked() = 0;
On 2010/03/31 23:03:56, michaeln wrote:
> For symmetry with OnUpdateCompelete, include an AppCacheGroup* parameter.
Also,
> can you add a comment about when this is called.

Done.

http://codereview.chromium.org/1600002/diff/3001/4013#newcode110
webkit/appcache/appcache_group.h:110: void ContentBlocked();
On 2010/03/31 23:03:56, michaeln wrote:
> maybe NotifyContentBlocked()

Done.

http://codereview.chromium.org/1600002/diff/3001/4016
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/1600002/diff/3001/4016#newcode102
webkit/appcache/appcache_host.h:102: // AppCacheGroup::UpdateObserver method
On 2010/03/31 23:03:56, michaeln wrote:
> Please keep these in the private section as they really aren't intended to be
> part of the AppCacheHost interface.

Done.

http://codereview.chromium.org/1600002/diff/3001/4018
File webkit/appcache/appcache_interfaces.h (right):

http://codereview.chromium.org/1600002/diff/3001/4018#newcode74
webkit/appcache/appcache_interfaces.h:74: virtual void ContentBlocked(int
render_view_id) = 0;
On 2010/03/31 23:03:56, michaeln wrote:
> Per other comments, i think this could be removed.

Done.

http://codereview.chromium.org/1600002/diff/3001/4019
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/1600002/diff/3001/4019#newcode198
webkit/appcache/appcache_request_handler.cc:198: host_->OnContentBlocked();
On 2010/03/31 23:03:56, michaeln wrote:
> I see you're using the UpdateObserver method here too. That overload is a bit
> sneaky.
> 
> Here we could use...
>   host_->frontend()->OnContentBlocked(host_->host_id());
> 
> Also notice that at this point, the document associated with this appcache is
> not yet committed to its Frame, its still the provisional document at this
time.
> We don't want to set icon until after our document has been committed to the
> frame.
> 
> We could handle that on this side or on the renderer side... by deferring
things
> until 'selectAppCache' time, the doc is committed by then.

Done.

http://codereview.chromium.org/1600002/diff/3001/4026
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/1600002/diff/3001/4026#newcode226
webkit/appcache/appcache_update_job.cc:226: group_->ContentBlocked();
On 2010/03/31 23:03:56, michaeln wrote:
> NotifyContentBlocked()

Done.

http://codereview.chromium.org/1600002/diff/3001/4031
File webkit/appcache/web_application_cache_host_impl.h (right):

http://codereview.chromium.org/1600002/diff/3001/4031#newcode36
webkit/appcache/web_application_cache_host_impl.h:36: void OnContentBlocked();
On 2010/03/31 23:03:56, michaeln wrote:
> I think it may be time to introduce process type specific subclasses of this
> class, and to make the OnContentBlocked method virtual (with an empty impl
here)
> so those derivatives can do something appropriate depending. What action is
> taken in a worker will have to be different.
> 
> If we go this route we don't need the render_view_id in this class at all (to
> have it in the webkit/appcache library is really a layering violation). We
would
> need that data member it in a RendererWebAppliationCacheHost subclass, and its
> impl of OnContentBlocked() could use an already existing IPC to get the icon
to
> show up in the UI.
> 
> wdyt?

Done.

[michaeln, 2010-04-07 21:52:57]

http://codereview.chromium.org/1600002/diff/14001/15004
File chrome/common/appcache/appcache_backend_proxy.cc (right):

http://codereview.chromium.org/1600002/diff/14001/15004#newcode7
chrome/common/appcache/appcache_backend_proxy.cc:7: #include
"chrome/common/content_settings_types.h"
Is the new include really needed?

http://codereview.chromium.org/1600002/diff/14001/15024
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/1600002/diff/14001/15024#newcode197
webkit/appcache/appcache_request_handler.cc:197: content_blocked_ |=
was_blocked_by_policy;
see comment below about moving this flag to AppCacheHost

http://codereview.chromium.org/1600002/diff/14001/15024#newcode302
webkit/appcache/appcache_request_handler.cc:302:
host_->frontend()->OnContentBlocked(host_->host_id());
This condition will never be met... content_block_ only has a chance to be set
for the 'main' resource... but notice line 296, we never get this far when
handling the 'main' resource. Generally, the RequestHandler for the main
resource can be deleted prior cache selection time. So this isn't going to do
what you're shooting at (defer delivery of the main resource was blocked
notification until after cache selection).

If we're going to do the deferral on the browser side, I think we have to retain
the "something_was_blocked" state in the host_ object, and upon cache selection
being complete... call the frontend from there. 

Or we could do the deferral on the client side, in
RendererWebApplicationCacheHost... either works, but i vote for putting the
logic on the browser side.

Does that make sense?

http://codereview.chromium.org/1600002/diff/14001/15038
File webkit/tools/test_shell/simple_appcache_system.cc (right):

http://codereview.chromium.org/1600002/diff/14001/15038#newcode232
webkit/tools/test_shell/simple_appcache_system.cc:232: virtual void
ContentBlocked(int render_view_id) {}
can this be deleted?

[jochen (gone - plz use gerrit), 2010-04-07 23:08:17]

On 2010/04/07 21:52:57, michaeln wrote:
> http://codereview.chromium.org/1600002/diff/14001/15004
> File chrome/common/appcache/appcache_backend_proxy.cc (right):
> 
> http://codereview.chromium.org/1600002/diff/14001/15004#newcode7
> chrome/common/appcache/appcache_backend_proxy.cc:7: #include
> "chrome/common/content_settings_types.h"
> Is the new include really needed?
> 
> http://codereview.chromium.org/1600002/diff/14001/15024
> File webkit/appcache/appcache_request_handler.cc (right):
> 
> http://codereview.chromium.org/1600002/diff/14001/15024#newcode197
> webkit/appcache/appcache_request_handler.cc:197: content_blocked_ |=
> was_blocked_by_policy;
> see comment below about moving this flag to AppCacheHost
> 
> http://codereview.chromium.org/1600002/diff/14001/15024#newcode302
> webkit/appcache/appcache_request_handler.cc:302:
> host_->frontend()->OnContentBlocked(host_->host_id());
> This condition will never be met... content_block_ only has a chance to be set
> for the 'main' resource... but notice line 296, we never get this far when
> handling the 'main' resource. Generally, the RequestHandler for the main
> resource can be deleted prior cache selection time. So this isn't going to do
> what you're shooting at (defer delivery of the main resource was blocked
> notification until after cache selection).
> 
> If we're going to do the deferral on the browser side, I think we have to
retain
> the "something_was_blocked" state in the host_ object, and upon cache
selection
> being complete... call the frontend from there. 
> 
> Or we could do the deferral on the client side, in
> RendererWebApplicationCacheHost... either works, but i vote for putting the
> logic on the browser side.
> 
> Does that make sense?

by browser side you mean the AppCacheHost object?

and by until after cache selection you mean in the method
AppCacheHost::FinishCacheSelection?
> 
> http://codereview.chromium.org/1600002/diff/14001/15038
> File webkit/tools/test_shell/simple_appcache_system.cc (right):
> 
> http://codereview.chromium.org/1600002/diff/14001/15038#newcode232
> webkit/tools/test_shell/simple_appcache_system.cc:232: virtual void
> ContentBlocked(int render_view_id) {}
> can this be deleted?

[jochen (gone - plz use gerrit), 2010-04-08 08:09:47]

please review

http://codereview.chromium.org/1600002/diff/14001/15004
File chrome/common/appcache/appcache_backend_proxy.cc (right):

http://codereview.chromium.org/1600002/diff/14001/15004#newcode7
chrome/common/appcache/appcache_backend_proxy.cc:7: #include
"chrome/common/content_settings_types.h"
On 2010/04/07 21:52:57, michaeln wrote:
> Is the new include really needed?

Done.

http://codereview.chromium.org/1600002/diff/14001/15024
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/1600002/diff/14001/15024#newcode197
webkit/appcache/appcache_request_handler.cc:197: content_blocked_ |=
was_blocked_by_policy;
On 2010/04/07 21:52:57, michaeln wrote:
> see comment below about moving this flag to AppCacheHost

Done.

http://codereview.chromium.org/1600002/diff/14001/15038
File webkit/tools/test_shell/simple_appcache_system.cc (right):

http://codereview.chromium.org/1600002/diff/14001/15038#newcode232
webkit/tools/test_shell/simple_appcache_system.cc:232: virtual void
ContentBlocked(int render_view_id) {}
On 2010/04/07 21:52:57, michaeln wrote:
> can this be deleted?

Done.

[michaeln, 2010-04-08 18:17:42]

LGTM modulu the couple of comments below

http://codereview.chromium.org/1600002/diff/21001/22009
File chrome/renderer/renderer_webapplicationcachehost_impl.cc (right):

http://codereview.chromium.org/1600002/diff/21001/22009#newcode29
chrome/renderer/renderer_webapplicationcachehost_impl.cc:29:
render_view_->routing_id(), CONTENT_SETTINGS_TYPE_COOKIES));
Here's where the raciness i mentioned earlier comes into play.

I don't think we have any guarantees that the document this appcacheHost is
associated with is still the document that is committed to the frame. If not, we
*could* be erroneously setting the icon.

Also, I don't think we have any guarantee that the RenderView is still
allocated. If not, we *could* crash at this dereference (or pick up odd values
for routing_id).

I think I'd be more comfortable with setting a routing_id_ data member in the
ctor and using that data member here for now. (And not having the render_view_
data member which could be a dangling ptr)

http://codereview.chromium.org/1600002/diff/21001/22019
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/1600002/diff/21001/22019#newcode86
webkit/appcache/appcache_host.h:86: // Used to notify the host that a request
was blocked by a policy.
Maybe update the comment to say that the main resource was block, and rename the
method to NotifyMainResourceBlocked.

Its depending on be called prior to cache selection time to work properly. If
this gets called after cache selection time, its a noop. All resource loads
except the main resource load happen after cache selection.

So "main" is significant.

[jochen (gone - plz use gerrit), 2010-04-09 15:44:47]

http://codereview.chromium.org/1600002/diff/21001/22009
File chrome/renderer/renderer_webapplicationcachehost_impl.cc (right):

http://codereview.chromium.org/1600002/diff/21001/22009#newcode29
chrome/renderer/renderer_webapplicationcachehost_impl.cc:29:
render_view_->routing_id(), CONTENT_SETTINGS_TYPE_COOKIES));
On 2010/04/08 18:17:42, michaeln wrote:
> Here's where the raciness i mentioned earlier comes into play.
> 
> I don't think we have any guarantees that the document this appcacheHost is
> associated with is still the document that is committed to the frame. If not,
we
> *could* be erroneously setting the icon.
> 
> Also, I don't think we have any guarantee that the RenderView is still
> allocated. If not, we *could* crash at this dereference (or pick up odd values
> for routing_id).
> 
> I think I'd be more comfortable with setting a routing_id_ data member in the
> ctor and using that data member here for now. (And not having the render_view_
> data member which could be a dangling ptr)

Done.

http://codereview.chromium.org/1600002/diff/21001/22019
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/1600002/diff/21001/22019#newcode86
webkit/appcache/appcache_host.h:86: // Used to notify the host that a request
was blocked by a policy.
On 2010/04/08 18:17:42, michaeln wrote:
> Maybe update the comment to say that the main resource was block, and rename
the
> method to NotifyMainResourceBlocked.
> 
> Its depending on be called prior to cache selection time to work properly. If
> this gets called after cache selection time, its a noop. All resource loads
> except the main resource load happen after cache selection.
> 
> So "main" is significant.

Done.