Removing --disable-application-cache flag

Removing --disable-application-cache flag as it has been shipped.

BUG=447206
Committed: https://crrev.com/72240eadd2bc19b17ddee08304dafe84524466d1
Cr-Commit-Position: refs/heads/master@{#310975}

[anujsharma, 2015-01-08 14:19:41]

anujk.sharma@samsung.com changed reviewers:
+ avi@chromium.org, lcwu@chromium.org

[anujsharma, 2015-01-08 14:19:42]

lcwu@chromium.org: Please review changes in chromecast module

avi@chromium.org: Please review changes in content module

[Avi (use Gerrit), 2015-01-08 15:46:53]

lgtm

[lcwu1, 2015-01-08 19:12:45]

Hi,

Can you elaborate on what feature has been shipped so that this flag is no
longer needed? Thanks.

[anujsharma, 2015-01-09 13:53:45]

On 2015/01/08 15:46:53, Avi wrote:
> lgtm

Thanks Avi for lgtm.

[anujsharma, 2015-01-09 13:59:38]

On 2015/01/08 19:12:45, lcwu1 wrote:
> Hi,
> 
> Can you elaborate on what feature has been shipped so that this flag is no
> longer needed? Thanks.

@Icwu1
First of all thanks for time to review this CL :)

As per Meta bug https://code.google.com/p/chromium/issues/detail?id=343941,
=================================================================================
Features that have shipped for a few releases, for a while, or for a long time -
--disable-application-cache
=================================================================================

That's motivated me to raise a "Blocked on Bug" and a CL for removal of this
command-line flag.

Thanks!!

[lcwu1, 2015-01-09 16:19:03]

On 2015/01/09 13:59:38, anujsharma wrote:
> On 2015/01/08 19:12:45, lcwu1 wrote:
> > Hi,
> > 
> > Can you elaborate on what feature has been shipped so that this flag is no
> > longer needed? Thanks.
> 
> @Icwu1
> First of all thanks for time to review this CL :)
> 
> As per Meta bug https://code.google.com/p/chromium/issues/detail?id=343941,
>
=================================================================================
> Features that have shipped for a few releases, for a while, or for a long time
-
> --disable-application-cache
>
=================================================================================
> 
> That's motivated me to raise a "Blocked on Bug" and a CL for removal of this
> command-line flag.
> 
> Thanks!!

Hi,

I understand that the support of the application cache has shipped for a while,
but this flag provides a way for a content embedder (such as Chromecast) to
disable it if necessary. And in the Chromecast's case, we don't need the
application cache and disabling it allows us to reduce the storage usage. A
quick code search shows that Blink still has this runtime feature function
setApplicationCacheEnabled to disable the application cache. This CL would
remove the ability for a content embedded to tell Blink not to use the
application cache. We would certainly prefer for this flag to stay. Thanks.

[Avi (use Gerrit), 2015-01-09 16:26:26]

On 2015/01/09 16:19:03, lcwu1 wrote:
> On 2015/01/09 13:59:38, anujsharma wrote:
> > On 2015/01/08 19:12:45, lcwu1 wrote:
> > > Hi,
> > > 
> > > Can you elaborate on what feature has been shipped so that this flag is no
> > > longer needed? Thanks.
> > 
> > @Icwu1
> > First of all thanks for time to review this CL :)
> > 
> > As per Meta bug https://code.google.com/p/chromium/issues/detail?id=343941,
> >
>
=================================================================================
> > Features that have shipped for a few releases, for a while, or for a long
time
> -
> > --disable-application-cache
> >
>
=================================================================================
> > 
> > That's motivated me to raise a "Blocked on Bug" and a CL for removal of this
> > command-line flag.
> > 
> > Thanks!!
> 
> Hi,
> 
> I understand that the support of the application cache has shipped for a
while,
> but this flag provides a way for a content embedder (such as Chromecast) to
> disable it if necessary. And in the Chromecast's case, we don't need the
> application cache and disabling it allows us to reduce the storage usage. A
> quick code search shows that Blink still has this runtime feature function
> setApplicationCacheEnabled to disable the application cache. This CL would
> remove the ability for a content embedded to tell Blink not to use the
> application cache. We would certainly prefer for this flag to stay. Thanks.

According to the spreadsheet in http://crbug.com/343941,
https://docs.google.com/a/google.com/spreadsheet/pub?key=0ArH-w4dboLrgdGRqNU5...
, this flag is slated for removal.

michaeln@chromium.org commented that it should go; can you talk to him?

anujsharma, do you have permissions to update that spreadsheet with these
changes you're making?

[Avi (use Gerrit), 2015-01-09 16:27:24]

On 2015/01/09 16:26:26, Avi wrote:
> On 2015/01/09 16:19:03, lcwu1 wrote:
> > On 2015/01/09 13:59:38, anujsharma wrote:
> > > On 2015/01/08 19:12:45, lcwu1 wrote:
> > > > Hi,
> > > > 
> > > > Can you elaborate on what feature has been shipped so that this flag is
no
> > > > longer needed? Thanks.
> > > 
> > > @Icwu1
> > > First of all thanks for time to review this CL :)
> > > 
> > > As per Meta bug
https://code.google.com/p/chromium/issues/detail?id=343941,
> > >
> >
>
=================================================================================
> > > Features that have shipped for a few releases, for a while, or for a long
> time
> > -
> > > --disable-application-cache
> > >
> >
>
=================================================================================
> > > 
> > > That's motivated me to raise a "Blocked on Bug" and a CL for removal of
this
> > > command-line flag.
> > > 
> > > Thanks!!
> > 
> > Hi,
> > 
> > I understand that the support of the application cache has shipped for a
> while,
> > but this flag provides a way for a content embedder (such as Chromecast) to
> > disable it if necessary. And in the Chromecast's case, we don't need the
> > application cache and disabling it allows us to reduce the storage usage. A
> > quick code search shows that Blink still has this runtime feature function
> > setApplicationCacheEnabled to disable the application cache. This CL would
> > remove the ability for a content embedded to tell Blink not to use the
> > application cache. We would certainly prefer for this flag to stay. Thanks.
> 
> According to the spreadsheet in http://crbug.com/343941,
>
https://docs.google.com/a/google.com/spreadsheet/pub?key=0ArH-w4dboLrgdGRqNU5...
> , this flag is slated for removal.
> 
> mailto:michaeln@chromium.org commented that it should go; can you talk to him?
> 
> anujsharma, do you have permissions to update that spreadsheet with these
> changes you're making?

Also, this removes the flag. If you're embedding content, you should be setting
these kinds of flags via code, not via the commandline flags.

[lcwu1, 2015-01-09 18:03:46]

On 2015/01/09 16:27:24, Avi wrote:
> On 2015/01/09 16:26:26, Avi wrote:
> > On 2015/01/09 16:19:03, lcwu1 wrote:
> > > On 2015/01/09 13:59:38, anujsharma wrote:
> > > > On 2015/01/08 19:12:45, lcwu1 wrote:
> > > > > Hi,
> > > > > 
> > > > > Can you elaborate on what feature has been shipped so that this flag
is
> no
> > > > > longer needed? Thanks.
> > > > 
> > > > @Icwu1
> > > > First of all thanks for time to review this CL :)
> > > > 
> > > > As per Meta bug
> https://code.google.com/p/chromium/issues/detail?id=343941,
> > > >
> > >
> >
>
=================================================================================
> > > > Features that have shipped for a few releases, for a while, or for a
long
> > time
> > > -
> > > > --disable-application-cache
> > > >
> > >
> >
>
=================================================================================
> > > > 
> > > > That's motivated me to raise a "Blocked on Bug" and a CL for removal of
> this
> > > > command-line flag.
> > > > 
> > > > Thanks!!
> > > 
> > > Hi,
> > > 
> > > I understand that the support of the application cache has shipped for a
> > while,
> > > but this flag provides a way for a content embedder (such as Chromecast)
to
> > > disable it if necessary. And in the Chromecast's case, we don't need the
> > > application cache and disabling it allows us to reduce the storage usage.
A
> > > quick code search shows that Blink still has this runtime feature function
> > > setApplicationCacheEnabled to disable the application cache. This CL would
> > > remove the ability for a content embedded to tell Blink not to use the
> > > application cache. We would certainly prefer for this flag to stay.
Thanks.
> > 
> > According to the spreadsheet in http://crbug.com/343941,
> >
>
https://docs.google.com/a/google.com/spreadsheet/pub?key=0ArH-w4dboLrgdGRqNU5...
> > , this flag is slated for removal.
> > 
> > mailto:michaeln@chromium.org commented that it should go; can you talk to
him?
> > 
> > anujsharma, do you have permissions to update that spreadsheet with these
> > changes you're making?
> 
> Also, this removes the flag. If you're embedding content, you should be
setting
> these kinds of flags via code, not via the commandline flags.

I just did some digging and it looks like we can use webview settings'
setOfflineWebApplicationCacheEnabled() to disable the application cache when a
render view is first created. So we probably don't need this flag. Thanks.

[lcwu1, 2015-01-09 18:04:05]

lgtm

[anujsharma, 2015-01-10 07:41:26]

On 2015/01/09 16:26:26, Avi wrote:
> On 2015/01/09 16:19:03, lcwu1 wrote:
> > On 2015/01/09 13:59:38, anujsharma wrote:
> > > On 2015/01/08 19:12:45, lcwu1 wrote:
> > > > Hi,
> > > > 
> > > > Can you elaborate on what feature has been shipped so that this flag is
no
> > > > longer needed? Thanks.
> > > 
> > > @Icwu1
> > > First of all thanks for time to review this CL :)
> > > 
> > > As per Meta bug
https://code.google.com/p/chromium/issues/detail?id=343941,
> > >
> >
>
=================================================================================
> > > Features that have shipped for a few releases, for a while, or for a long
> time
> > -
> > > --disable-application-cache
> > >
> >
>
=================================================================================
> > > 
> > > That's motivated me to raise a "Blocked on Bug" and a CL for removal of
this
> > > command-line flag.
> > > 
> > > Thanks!!
> > 
> > Hi,
> > 
> > I understand that the support of the application cache has shipped for a
> while,
> > but this flag provides a way for a content embedder (such as Chromecast) to
> > disable it if necessary. And in the Chromecast's case, we don't need the
> > application cache and disabling it allows us to reduce the storage usage. A
> > quick code search shows that Blink still has this runtime feature function
> > setApplicationCacheEnabled to disable the application cache. This CL would
> > remove the ability for a content embedded to tell Blink not to use the
> > application cache. We would certainly prefer for this flag to stay. Thanks.
> 
> According to the spreadsheet in http://crbug.com/343941,
>
https://docs.google.com/a/google.com/spreadsheet/pub?key=0ArH-w4dboLrgdGRqNU5...
> , this flag is slated for removal.
> 
> mailto:michaeln@chromium.org commented that it should go; can you talk to him?
> 
> anujsharma, do you have permissions to update that spreadsheet with these
> changes you're making?

Thanks Avi for your response.

I think i am only having read-only access to this spreadsheet. :(

[anujsharma, 2015-01-10 07:42:34]

On 2015/01/09 18:04:05, lcwu1 wrote:
> lgtm

Thanks lcwu1 for lgtm.

[anujsharma, 2015-01-10 07:43:05]

The CQ bit was checked by anujk.sharma@samsung.com

[commit-bot: I haz the power, 2015-01-10 07:44:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/837363002/1

[commit-bot: I haz the power, 2015-01-10 08:38:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-10 08:39:37]

Patchset 1 (id:??) landed as
https://crrev.com/72240eadd2bc19b17ddee08304dafe84524466d1
Cr-Commit-Position: refs/heads/master@{#310975}

[Matt Giuca, 2015-01-15 02:39:29]

This CL appears to have broken AppCache (see comment).

I have filed a bug and will send out a fix: http://crbug.com/449016.

https://codereview.chromium.org/837363002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.cc (left):

https://codereview.chromium.org/837363002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:342:
!command_line.HasSwitch(switches::kDisableApplicationCache);
This seems to have broken AppCache. Since
WebPreferences::application_cache_enabled defaults to false, removing this line
means that it will not be set to true, so AppCache will be disabled permanently.