PlzNavigate: AppCache support.

PlzNavigate: AppCache support

michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new host id
which is passed by the appcache code in the renderer via the AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a renderer when the
navigation request is instantiated. To workaround this we generate host ids in the newly
newly added AppCacheNavigationHandle class. This class is created on the UI thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed to the
passed to the renderer in the FrameMsg_CommitNavigation IPC in the RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle core.
   The AppCacheNavigationHandle class is created when the NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class which is used for
   HTTP requests until the renderer registers with us. When the navigation is committed this
   host is passed to the AppCacheBackImpl class via AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.

BUG=608375
TEST=manual
The test case at http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/292a917258e6ce3395a589fe40623d0697df044b
Cr-Commit-Position: refs/heads/master@{#436821}

[ananta, 2016-11-15 18:30:16]

Description was changed from

==========
PlzNavigate: AppCache support.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id and the renderer_process_id values in the
NavigationRequestInfo
   structure which eventually makes its way to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

2. The AppCache changes are mostly in the AppCacheDispatcherHost class. The main
changes
   are a new function RegisterPendingHost() in the AppCacheDispatcherHost class
which registers
   a pending host for PlzNavigate when a navigation request  is instantiated.
The other change
   is a static function GetHostForProcess() which returns the
AppCacheDispatcherHost
   instance for a renderer process id.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the AppCacheDispatcherHost
class in the RegisterPendingHost() function. This host id is passed to the
renderer in
the FrameMsg_CommitNavigation IPC in the RequestNavigationParams structure. The
host id
is eventually passed to the WebApplicationCacheHostImpl class.

BUG=608375

Initial changes for appcache
==========

to

==========
PlzNavigate: AppCache support.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id and the renderer_process_id values in the
NavigationRequestInfo
   structure which eventually makes its way to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

2. The AppCache changes are mostly in the AppCacheDispatcherHost class. The main
changes
   are a new function RegisterPendingHost() in the AppCacheDispatcherHost class
which registers
   a pending host for PlzNavigate when a navigation request  is instantiated.
The other change
   is a static function GetHostForProcess() which returns the
AppCacheDispatcherHost
   instance for a renderer process id.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the AppCacheDispatcherHost
class in the RegisterPendingHost() function. This host id is passed to the
renderer in
the FrameMsg_CommitNavigation IPC in the RequestNavigationParams structure. The
host id
is eventually passed to the WebApplicationCacheHostImpl class.

BUG=608375

Initial changes for appcache
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-11-15 18:41:33]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 18:42:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 19:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 19:19:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ananta, 2016-11-15 20:00:30]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 20:01:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-11-15 21:03:53]

ananta@chromium.org changed reviewers:
+ michaeln@chromium.org

[commit-bot: I haz the power, 2016-11-15 21:35:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 21:35:21]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[ananta, 2016-11-15 23:41:57]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 23:42:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 00:40:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 00:40:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[ananta, 2016-11-16 02:01:33]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 02:02:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-11-16 02:35:57]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 02:36:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 03:36:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 03:36:12]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[dcheng, 2016-11-16 13:58:16]

> The AppCache system has a concept of host ids, which map to different DOM's
being loaded
> in the renderer.

In other words, each navigation that loads a new Document gets a different host
ID?

[ananta, 2016-11-17 20:25:05]

Description was changed from

==========
PlzNavigate: AppCache support.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id and the renderer_process_id values in the
NavigationRequestInfo
   structure which eventually makes its way to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

2. The AppCache changes are mostly in the AppCacheDispatcherHost class. The main
changes
   are a new function RegisterPendingHost() in the AppCacheDispatcherHost class
which registers
   a pending host for PlzNavigate when a navigation request  is instantiated.
The other change
   is a static function GetHostForProcess() which returns the
AppCacheDispatcherHost
   instance for a renderer process id.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the AppCacheDispatcherHost
class in the RegisterPendingHost() function. This host id is passed to the
renderer in
the FrameMsg_CommitNavigation IPC in the RequestNavigationParams structure. The
host id
is eventually passed to the WebApplicationCacheHostImpl class.

BUG=608375

Initial changes for appcache
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
added AppCacheFrameNavigationHandler class via the BeginNavigate function. 
This host id is passed to the renderer in the FrameMsg_CommitNavigation IPC in
the 
RequestNavigationParams structure. The host id is eventually passed to the
WebApplicationCacheHostImpl
class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of a new class AppCacheFrameNavigationHandler. This class is defined
in the
   newly added files appcache_frame_node_navigation.h/.cc. This class provides
functionality
   to maintain state about navigations initiated for a FrameTreeNode. This state
includes
   registering AppCache hosts for the navigation, etc. 

2. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id values in the NavigationRequestInfo structure which
eventually makes its way
   to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class to switch the
AppCacheBackendImpl
      instance. This is provided via the function SwitchBackend().
   2. Add support in the AppCacheServiceImpl class to register backends based on
frame id.
   3. Provide functionality in the AppCacheBackendImpl class and the
AppCacheHost class to
      set the AppCacheFront end pointer. This is required when we pass the
AppCacheBackendImpl
      instance to the AppCacheDispatcherHost class described by 1 above.

BUG=608375

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-11-17 20:25:31]

On 2016/11/16 13:58:16, dcheng wrote:
> > The AppCache system has a concept of host ids, which map to different DOM's
> being loaded
> > in the renderer.
> 
> In other words, each navigation that loads a new Document gets a different
host
> ID?

Yes

[ananta, 2016-11-17 20:25:46]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 20:27:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 20:45:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 20:45:15]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[ananta, 2016-11-17 22:02:34]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 22:04:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-11-17 23:07:23]

ananta@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[commit-bot: I haz the power, 2016-11-17 23:25:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 23:25:24]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[ananta, 2016-11-18 01:19:56]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 01:21:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-11-18 02:01:06]

Description was changed from

==========
The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
added AppCacheFrameNavigationHandler class via the BeginNavigate function. 
This host id is passed to the renderer in the FrameMsg_CommitNavigation IPC in
the 
RequestNavigationParams structure. The host id is eventually passed to the
WebApplicationCacheHostImpl
class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of a new class AppCacheFrameNavigationHandler. This class is defined
in the
   newly added files appcache_frame_node_navigation.h/.cc. This class provides
functionality
   to maintain state about navigations initiated for a FrameTreeNode. This state
includes
   registering AppCache hosts for the navigation, etc. 

2. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id values in the NavigationRequestInfo structure which
eventually makes its way
   to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class to switch the
AppCacheBackendImpl
      instance. This is provided via the function SwitchBackend().
   2. Add support in the AppCacheServiceImpl class to register backends based on
frame id.
   3. Provide functionality in the AppCacheBackendImpl class and the
AppCacheHost class to
      set the AppCacheFront end pointer. This is required when we pass the
AppCacheBackendImpl
      instance to the AppCacheDispatcherHost class described by 1 above.

BUG=608375

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
added AppCacheFrameNavigationHandler class via the BeginNavigate function. 
This host id is passed to the renderer in the FrameMsg_CommitNavigation IPC in
the 
RequestNavigationParams structure. The host id is eventually passed to the
WebApplicationCacheHostImpl
class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of a new class AppCacheFrameNavigationHandler. This class is defined
in the
   newly added files appcache_frame_node_navigation.h/.cc. This class provides
functionality
   to maintain state about navigations initiated for a FrameTreeNode. This state
includes
   registering AppCache hosts for the navigation, etc. 

2. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id values in the NavigationRequestInfo structure which
eventually makes its way
   to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class to switch the
AppCacheBackendImpl
      instance. This is provided via the function SwitchBackend().
   2. Add support in the AppCacheServiceImpl class to register backends based on
frame id.
   3. Provide functionality in the AppCacheBackendImpl class and the
AppCacheHost class to
      set the AppCacheFront end pointer. This is required when we pass the
AppCacheBackendImpl
      instance to the AppCacheDispatcherHost class described by 1 above.

BUG=608375

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-11-18 03:42:05]

Description was changed from

==========
michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
added AppCacheFrameNavigationHandler class via the BeginNavigate function. 
This host id is passed to the renderer in the FrameMsg_CommitNavigation IPC in
the 
RequestNavigationParams structure. The host id is eventually passed to the
WebApplicationCacheHostImpl
class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of a new class AppCacheFrameNavigationHandler. This class is defined
in the
   newly added files appcache_frame_node_navigation.h/.cc. This class provides
functionality
   to maintain state about navigations initiated for a FrameTreeNode. This state
includes
   registering AppCache hosts for the navigation, etc. 

2. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id values in the NavigationRequestInfo structure which
eventually makes its way
   to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class to switch the
AppCacheBackendImpl
      instance. This is provided via the function SwitchBackend().
   2. Add support in the AppCacheServiceImpl class to register backends based on
frame id.
   3. Provide functionality in the AppCacheBackendImpl class and the
AppCacheHost class to
      set the AppCacheFront end pointer. This is required when we pass the
AppCacheBackendImpl
      instance to the AppCacheDispatcherHost class described by 1 above.

BUG=608375

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
added AppCacheFrameNavigationHandler class via the BeginNavigate function. 
This host id is passed to the renderer in the FrameMsg_CommitNavigation IPC in
the 
RequestNavigationParams structure. The host id is eventually passed to the
WebApplicationCacheHostImpl
class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of a new class AppCacheFrameNavigationHandler. This class is defined
in the
   newly added files appcache_frame_node_navigation.h/.cc. This class provides
functionality
   to maintain state about navigations initiated for a FrameTreeNode. This state
includes
   registering AppCache hosts for the navigation, etc. 

2. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id values in the NavigationRequestInfo structure which
eventually makes its way
   to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class to switch the
AppCacheBackendImpl
      instance. This is provided via the function SwitchBackend().
   2. Add support in the AppCacheServiceImpl class to register backends based on
frame id.
   3. Provide functionality in the AppCacheBackendImpl class and the
AppCacheHost class to
      set the AppCacheFront end pointer. This is required when we pass the
AppCacheBackendImpl
      instance to the AppCacheDispatcherHost class described by 1 above.

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-18 03:57:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 03:57:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2016-11-18 14:53:19]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 14:53:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 16:36:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 16:36:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2016-11-18 19:14:40]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 19:15:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-11-18 20:03:44]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 20:04:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 22:07:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 22:07:36]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[ananta, 2016-11-19 01:02:17]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 01:02:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 01:58:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-19 01:58:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2016-11-19 02:30:37]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 02:30:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-11-19 02:46:16]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 02:46:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-11-19 04:04:53]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-19 04:05:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 05:09:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-19 05:09:39]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-11-21 17:56:53]

Sorry I did not have time to look at it today, but I'll try to pass on it
tomorrow.

[michaeln, 2016-11-22 00:17:27]

i think it'd be good if appcache+plznav integration looked more like
serviceworker+plznav

ServiceWorkerNavigationHandle is the main object that bridges the gap.
NavigationHandleImpl on the UI thread owns one of those which hooks into the sw
impl on the IO thread via a ServiceWorkerNavigationHandleCore object.

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:21: if (process_id_ != -1) {
are there constants defined for these -1 values anywhere?

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:39: if (frame_id != -1) {
are frame_id and process_id mutually exclusive

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:191: for (host_index =
hosts_.begin(); host_index != hosts_.end(); ++host_index)
for (auto& iter : backend->hosts_)

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:199: for (host_index =
backend->hosts_.begin();
iter might be a better name, i got tripped seeing too many host_ things in here

better yet...

for (auto& iter : backend->hosts_)

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.h:76: // possibly the
AppCacheServiceImpl instance we are registered with.
under what circumstances would we need to swap the appcacheService?

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:117:
appcache_service_.get());
it doesn't make sense to me that the service object can be different? existing
host definitely shouldn't have anything todo with a different service object

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:128:
std::set<int>::iterator pending_host_index = pending_hosts_.find(host_id);
readability nit: auto found = x.find(id)

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.h:47: void
FrameNavigationCommitted(std::unique_ptr<AppCacheBackendImpl> backend);
I think this assumes the destination process had no documents open in it, what
if the current this->backend_ contains hosts?

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.h:52: // on the IO thread.
why does the navigation we need more than the 'host' object?

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_frame_node_navigation.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_frame_node_navigation.cc:170: if
(BrowserThread::CurrentlyOn(BrowserThread::IO)) {
if we go thru this code path, we do we leave a dangling ptr in the
g_frame_navigation_handler_map?

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_frame_node_navigation.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_frame_node_navigation.h:25: // Handles
navigations for a frame. Sets up the AppCache backend for the frame
Do we ever get multiple hosts in this backend instance?

https://codereview.chromium.org/2501343003/diff/370001/content/browser/frame_...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/frame_...
content/browser/frame_host/navigation_request.h:234: int appcache_host_id_;
we put this value in the ServiceWorkerNavigationHandle in the servicewrker case

https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2268:
AppCacheInterceptor::SetExtraRequestInfo(
Can the techique we used for service worker work here too?

Take a look at what InitializeForNavigation does and what the object hierarchy
is, notice it takes as input the ServiceWorkerNavigationHandleCore with is given
ownership of the pre-created host object.

https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
content/common/navigation_params.h:327: // The host id to be used to identify
this navigation.
annotate with PlzNavigate, might be nice to put move it up above the
has_user_gesture to keep it close to the sw stuff

[ananta, 2016-11-22 04:07:03]

On 2016/11/22 00:17:27, michaeln wrote:
> i think it'd be good if appcache+plznav integration looked more like
> serviceworker+plznav
> 
> ServiceWorkerNavigationHandle is the main object that bridges the gap.
> NavigationHandleImpl on the UI thread owns one of those which hooks into the
sw
> impl on the IO thread via a ServiceWorkerNavigationHandleCore object.
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> File content/browser/appcache/appcache_backend_impl.cc (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_backend_impl.cc:21: if (process_id_ != -1) {
> are there constants defined for these -1 values anywhere?
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_backend_impl.cc:39: if (frame_id != -1) {
> are frame_id and process_id mutually exclusive
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_backend_impl.cc:191: for (host_index =
> hosts_.begin(); host_index != hosts_.end(); ++host_index)
> for (auto& iter : backend->hosts_)
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_backend_impl.cc:199: for (host_index =
> backend->hosts_.begin();
> iter might be a better name, i got tripped seeing too many host_ things in
here
> 
> better yet...
> 
> for (auto& iter : backend->hosts_)
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> File content/browser/appcache/appcache_backend_impl.h (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_backend_impl.h:76: // possibly the
> AppCacheServiceImpl instance we are registered with.
> under what circumstances would we need to swap the appcacheService?
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> File content/browser/appcache/appcache_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_dispatcher_host.cc:117:
> appcache_service_.get());
> it doesn't make sense to me that the service object can be different? existing
> host definitely shouldn't have anything todo with a different service object
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_dispatcher_host.cc:128:
> std::set<int>::iterator pending_host_index = pending_hosts_.find(host_id);
> readability nit: auto found = x.find(id)
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> File content/browser/appcache/appcache_dispatcher_host.h (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_dispatcher_host.h:47: void
> FrameNavigationCommitted(std::unique_ptr<AppCacheBackendImpl> backend);
> I think this assumes the destination process had no documents open in it, what
> if the current this->backend_ contains hosts?
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_dispatcher_host.h:52: // on the IO thread.
> why does the navigation we need more than the 'host' object?
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> File content/browser/appcache/appcache_frame_node_navigation.cc (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_frame_node_navigation.cc:170: if
> (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
> if we go thru this code path, we do we leave a dangling ptr in the
> g_frame_navigation_handler_map?
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> File content/browser/appcache/appcache_frame_node_navigation.h (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> content/browser/appcache/appcache_frame_node_navigation.h:25: // Handles
> navigations for a frame. Sets up the AppCache backend for the frame
> Do we ever get multiple hosts in this backend instance?
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/frame_...
> File content/browser/frame_host/navigation_request.h (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/frame_...
> content/browser/frame_host/navigation_request.h:234: int appcache_host_id_;
> we put this value in the ServiceWorkerNavigationHandle in the servicewrker
case
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.cc:2268:
> AppCacheInterceptor::SetExtraRequestInfo(
> Can the techique we used for service worker work here too?
> 
> Take a look at what InitializeForNavigation does and what the object hierarchy
> is, notice it takes as input the ServiceWorkerNavigationHandleCore with is
given
> ownership of the pre-created host object.
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
> File content/common/navigation_params.h (right):
> 
>
https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
> content/common/navigation_params.h:327: // The host id to be used to identify
> this navigation.
> annotate with PlzNavigate, might be nice to put move it up above the
> has_user_gesture to keep it close to the sw stuff

Thanks michaeln

I will look into this and update the patchset.

[ananta, 2016-11-23 03:30:39]

Description was changed from

==========
michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
added AppCacheFrameNavigationHandler class via the BeginNavigate function. 
This host id is passed to the renderer in the FrameMsg_CommitNavigation IPC in
the 
RequestNavigationParams structure. The host id is eventually passed to the
WebApplicationCacheHostImpl
class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of a new class AppCacheFrameNavigationHandler. This class is defined
in the
   newly added files appcache_frame_node_navigation.h/.cc. This class provides
functionality
   to maintain state about navigations initiated for a FrameTreeNode. This state
includes
   registering AppCache hosts for the navigation, etc. 

2. Changes in navigation_request.cc/.h are around passing in the
AppCacheServiceImpl pointer
   the appcache_host_id values in the NavigationRequestInfo structure which
eventually makes its way
   to RDHI::BeginNavigationRequest().
   These members are required to initialize AppCache support in the ongoing
navigation
   request.

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class to switch the
AppCacheBackendImpl
      instance. This is provided via the function SwitchBackend().
   2. Add support in the AppCacheServiceImpl class to register backends based on
frame id.
   3. Provide functionality in the AppCacheBackendImpl class and the
AppCacheHost class to
      set the AppCacheFront end pointer. This is required when we pass the
AppCacheBackendImpl
      instance to the AppCacheDispatcherHost class described by 1 above.

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
newly added AppCacheNavigationHandle class. This class is created on the UI
thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are
controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed
to the 
passed to the renderer in the FrameMsg_CommitNavigation IPC in the
RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle
core.
   The AppCacheNavigationHandle class is created when the
NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class
which is used for
   HTTP requests until the renderer registers with us. When the navigation is
committed this
   host is passed to the AppCacheBackImpl class via
AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the
precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the
AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.
  

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-11-23 03:42:43]

On 2016/11/22 04:07:03, ananta wrote:
> On 2016/11/22 00:17:27, michaeln wrote:
> > i think it'd be good if appcache+plznav integration looked more like
> > serviceworker+plznav
> > 
> > ServiceWorkerNavigationHandle is the main object that bridges the gap.
> > NavigationHandleImpl on the UI thread owns one of those which hooks into the
> sw
> > impl on the IO thread via a ServiceWorkerNavigationHandleCore object.
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > File content/browser/appcache/appcache_backend_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_backend_impl.cc:21: if (process_id_ != -1)
{
> > are there constants defined for these -1 values anywhere?
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_backend_impl.cc:39: if (frame_id != -1) {
> > are frame_id and process_id mutually exclusive
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_backend_impl.cc:191: for (host_index =
> > hosts_.begin(); host_index != hosts_.end(); ++host_index)
> > for (auto& iter : backend->hosts_)
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_backend_impl.cc:199: for (host_index =
> > backend->hosts_.begin();
> > iter might be a better name, i got tripped seeing too many host_ things in
> here
> > 
> > better yet...
> > 
> > for (auto& iter : backend->hosts_)
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > File content/browser/appcache/appcache_backend_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_backend_impl.h:76: // possibly the
> > AppCacheServiceImpl instance we are registered with.
> > under what circumstances would we need to swap the appcacheService?
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > File content/browser/appcache/appcache_dispatcher_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_dispatcher_host.cc:117:
> > appcache_service_.get());
> > it doesn't make sense to me that the service object can be different?
existing
> > host definitely shouldn't have anything todo with a different service object
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_dispatcher_host.cc:128:
> > std::set<int>::iterator pending_host_index = pending_hosts_.find(host_id);
> > readability nit: auto found = x.find(id)
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > File content/browser/appcache/appcache_dispatcher_host.h (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_dispatcher_host.h:47: void
> > FrameNavigationCommitted(std::unique_ptr<AppCacheBackendImpl> backend);
> > I think this assumes the destination process had no documents open in it,
what
> > if the current this->backend_ contains hosts?
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_dispatcher_host.h:52: // on the IO thread.
> > why does the navigation we need more than the 'host' object?
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > File content/browser/appcache/appcache_frame_node_navigation.cc (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_frame_node_navigation.cc:170: if
> > (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
> > if we go thru this code path, we do we leave a dangling ptr in the
> > g_frame_navigation_handler_map?
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > File content/browser/appcache/appcache_frame_node_navigation.h (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
> > content/browser/appcache/appcache_frame_node_navigation.h:25: // Handles
> > navigations for a frame. Sets up the AppCache backend for the frame
> > Do we ever get multiple hosts in this backend instance?
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/frame_...
> > File content/browser/frame_host/navigation_request.h (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/frame_...
> > content/browser/frame_host/navigation_request.h:234: int appcache_host_id_;
> > we put this value in the ServiceWorkerNavigationHandle in the servicewrker
> case
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
> > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
> > content/browser/loader/resource_dispatcher_host_impl.cc:2268:
> > AppCacheInterceptor::SetExtraRequestInfo(
> > Can the techique we used for service worker work here too?
> > 
> > Take a look at what InitializeForNavigation does and what the object
hierarchy
> > is, notice it takes as input the ServiceWorkerNavigationHandleCore with is
> given
> > ownership of the pre-created host object.
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
> > File content/common/navigation_params.h (right):
> > 
> >
>
https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
> > content/common/navigation_params.h:327: // The host id to be used to
identify
> > this navigation.
> > annotate with PlzNavigate, might be nice to put move it up above the
> > has_user_gesture to keep it close to the sw stuff
> 
> Thanks michaeln
> 
> I will look into this and update the patchset.

Added new classes AppCacheNavigationHandle and AppCacheNavigationHandleCore
which live on the UI thread and IO
thread respectively. Most of the comments in the previous iteration are no
longer valid with this change.
Please take another look. The AppCacheServiceImpl pointer changing problem is
still there. This happens with
IsolatedApps in one scenario.

[ananta, 2016-11-23 04:05:15]

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:21: if (process_id_ != -1) {
On 2016/11/22 00:17:27, michaeln wrote:
> are there constants defined for these -1 values anywhere?

Reverted

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:39: if (frame_id != -1) {
On 2016/11/22 00:17:27, michaeln wrote:
> are frame_id and process_id mutually exclusive

Reverted

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:191: for (host_index =
hosts_.begin(); host_index != hosts_.end(); ++host_index)
On 2016/11/22 00:17:27, michaeln wrote:
> for (auto& iter : backend->hosts_)

Reverted

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:199: for (host_index =
backend->hosts_.begin();
On 2016/11/22 00:17:27, michaeln wrote:
> iter might be a better name, i got tripped seeing too many host_ things in
here
> 
> better yet...
> 
> for (auto& iter : backend->hosts_)

Reverted

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.h:76: // possibly the
AppCacheServiceImpl instance we are registered with.
On 2016/11/22 00:17:27, michaeln wrote:
> under what circumstances would we need to swap the appcacheService?

IsolatedApps is one case. Need to look into why that happens. This  function has
been removed though.

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:117:
appcache_service_.get());
On 2016/11/22 00:17:27, michaeln wrote:
> it doesn't make sense to me that the service object can be different? existing
> host definitely shouldn't have anything todo with a different service object

Reverted.

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:128:
std::set<int>::iterator pending_host_index = pending_hosts_.find(host_id);
On 2016/11/22 00:17:27, michaeln wrote:
> readability nit: auto found = x.find(id)

Done.

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.h:47: void
FrameNavigationCommitted(std::unique_ptr<AppCacheBackendImpl> backend);
On 2016/11/22 00:17:27, michaeln wrote:
> I think this assumes the destination process had no documents open in it, what
> if the current this->backend_ contains hosts?

Reverted

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.h:52: // on the IO thread.
On 2016/11/22 00:17:27, michaeln wrote:
> why does the navigation we need more than the 'host' object?

Leaving this as is for now. I don't know if we can multiple navigations going on
together.

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_frame_node_navigation.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_frame_node_navigation.cc:170: if
(BrowserThread::CurrentlyOn(BrowserThread::IO)) {
On 2016/11/22 00:17:27, michaeln wrote:
> if we go thru this code path, we do we leave a dangling ptr in the
> g_frame_navigation_handler_map?

This file has gone

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
File content/browser/appcache/appcache_frame_node_navigation.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/appcac...
content/browser/appcache/appcache_frame_node_navigation.h:25: // Handles
navigations for a frame. Sets up the AppCache backend for the frame
On 2016/11/22 00:17:27, michaeln wrote:
> Do we ever get multiple hosts in this backend instance?

Reverted

https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/370001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2268:
AppCacheInterceptor::SetExtraRequestInfo(
On 2016/11/22 00:17:27, michaeln wrote:
> Can the techique we used for service worker work here too?
> 
> Take a look at what InitializeForNavigation does and what the object hierarchy
> is, notice it takes as input the ServiceWorkerNavigationHandleCore with is
given
> ownership of the pre-created host object. 

Added a new function AppCacheInterceptor::SetExtraRequestInfoForHost(). PTAL

https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2501343003/diff/370001/content/common/navigat...
content/common/navigation_params.h:327: // The host id to be used to identify
this navigation.
On 2016/11/22 00:17:27, michaeln wrote:
> annotate with PlzNavigate, might be nice to put move it up above the
> has_user_gesture to keep it close to the sw stuff

Done.

[ananta, 2016-11-23 14:01:36]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 14:02:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 14:04:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 14:04:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ananta, 2016-11-23 16:06:13]

Patchset #26 (id:490001) has been deleted

[ananta, 2016-11-23 16:43:28]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 16:44:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 16:55:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 16:55:26]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[ananta, 2016-11-23 17:03:53]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 17:04:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 17:26:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 17:26:45]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ananta, 2016-11-23 18:49:38]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 18:50:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 19:54:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 19:54:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ananta, 2016-11-23 21:42:57]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 21:43:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 23:46:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 23:46:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[ananta, 2016-11-24 02:21:41]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 02:22:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 04:23:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 04:23:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[clamy, 2016-11-25 13:32:24]

Thanks! I haven't reviewed the appcache part, since I'm not familiar with it.
The navigation part looks generally good, a few questions & comments below.

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:542: if
(appcache_handle_.get()) {
Note: at this point we have not committed the navigation yet, we just have a RFH
to use. It may still be showing the old page. Is that ok?

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:443: // registers the host
with the AppCache system.
Is it until the renderer registers the host or until we have a renderer for the
navigation browser-side and we call CommitNavigation?

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:521: if
(common_params_.url.scheme() == url::kHttpScheme ||
I'd prefer you use GURL::SchemeIsHTTPOrHTTPS().

https://codereview.chromium.org/2501343003/diff/590001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2501343003/diff/590001/content/common/navigat...
content/common/navigation_params.h:330: int appcache_host_id;
We've talked about introducing a navigation_id in the past (for example by
incrementing a global counter each time we create a NavigationRequest). I'm
wondering if we should do that and use it in lieu of having two different ids,
one for appcache and one for sw. I don't feel strongly on the question, though.

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5005: appcache_host_id_ =
request_params.appcache_host_id;
We store the request params in NavigateInternal, so we should manage to get the
id directly from them and avoid adding an extra parameter to RenderFrameImpl.

[ananta, 2016-11-28 21:36:53]

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:542: if
(appcache_handle_.get()) {
On 2016/11/25 13:32:24, clamy (slow) wrote:
> Note: at this point we have not committed the navigation yet, we just have a
RFH
> to use. It may still be showing the old page. Is that ok?

Yes. If the RFH is not going to change from this point on, then we are fine.

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:443: // registers the host
with the AppCache system.
On 2016/11/25 13:32:24, clamy (slow) wrote:
> Is it until the renderer registers the host or until we have a renderer for
the
> navigation browser-side and we call CommitNavigation?

Rephrased.

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:521: if
(common_params_.url.scheme() == url::kHttpScheme ||
On 2016/11/25 13:32:24, clamy (slow) wrote:
> I'd prefer you use GURL::SchemeIsHTTPOrHTTPS().

Done.

https://codereview.chromium.org/2501343003/diff/590001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2501343003/diff/590001/content/common/navigat...
content/common/navigation_params.h:330: int appcache_host_id;
On 2016/11/25 13:32:24, clamy (slow) wrote:
> We've talked about introducing a navigation_id in the past (for example by
> incrementing a global counter each time we create a NavigationRequest). I'm
> wondering if we should do that and use it in lieu of having two different ids,
> one for appcache and one for sw. I don't feel strongly on the question,
though.

If it ok with you we can look into this for a subsequent patch.

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5005: appcache_host_id_ =
request_params.appcache_host_id;
On 2016/11/25 13:32:24, clamy (slow) wrote:
> We store the request params in NavigateInternal, so we should manage to get
the
> id directly from them and avoid adding an extra parameter to RenderFrameImpl.

We save the request params in the pending_navigation_params_ member. However
that gets reset before the RenderFrameImpl::createApplicationCacheHost()
function is called. Leaving this as is.

[ananta, 2016-11-28 21:37:08]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 21:37:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 02:58:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 02:58:16]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-11-30 17:40:22]

Thanks! A few comments below. As last time, I'm leaving the appcache part for
michaeln to review.

https://codereview.chromium.org/2501343003/diff/590001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2501343003/diff/590001/content/common/navigat...
content/common/navigation_params.h:330: int appcache_host_id;
On 2016/11/28 21:36:53, ananta wrote:
> On 2016/11/25 13:32:24, clamy (slow) wrote:
> > We've talked about introducing a navigation_id in the past (for example by
> > incrementing a global counter each time we create a NavigationRequest). I'm
> > wondering if we should do that and use it in lieu of having two different
ids,
> > one for appcache and one for sw. I don't feel strongly on the question,
> though.
> 
> If it ok with you we can look into this for a subsequent patch. 

Acknowledged. As I mentioned, I don't feel strongly about it.

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5005: appcache_host_id_ =
request_params.appcache_host_id;
On 2016/11/28 21:36:53, ananta wrote:
> On 2016/11/25 13:32:24, clamy (slow) wrote:
> > We store the request params in NavigateInternal, so we should manage to get
> the
> > id directly from them and avoid adding an extra parameter to
RenderFrameImpl.
> 
> We save the request params in the pending_navigation_params_ member. However
> that gets reset before the RenderFrameImpl::createApplicationCacheHost()
> function is called. Leaving this as is.

You can still access them through the DataSource DocumentState's NavigationState
(in its NavigationStateImpl version). I would prefer we use them rather than
adding a member variable.

https://codereview.chromium.org/2501343003/diff/610001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2501343003/diff/610001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:443: // navigation is
committed, i.e we have a renderer process ready to service
nit: s/is committed/is ready to be committed

https://codereview.chromium.org/2501343003/diff/610001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2501343003/diff/610001/content/renderer/rende...
content/renderer/render_frame_impl.h:1344: // PlzNavigte: The host id to be used
by the AppCache system. Defaults to 0.
nit: s/PlzNavigte/PlzNavigate

[ananta, 2016-12-01 05:15:00]

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5005: appcache_host_id_ =
request_params.appcache_host_id;
On 2016/11/30 17:40:22, clamy (slow) wrote:
> On 2016/11/28 21:36:53, ananta wrote:
> > On 2016/11/25 13:32:24, clamy (slow) wrote:
> > > We store the request params in NavigateInternal, so we should manage to
get
> > the
> > > id directly from them and avoid adding an extra parameter to
> RenderFrameImpl.
> > 
> > We save the request params in the pending_navigation_params_ member. However
> > that gets reset before the RenderFrameImpl::createApplicationCacheHost()
> > function is called. Leaving this as is.
> 
> You can still access them through the DataSource DocumentState's
NavigationState
> (in its NavigationStateImpl version). I would prefer we use them rather than
> adding a member variable.

It looks like the right value is available via the provisionalDataSource()
function in the frame. I changed the code to look for the
RequestNavigationParams() first in the provisional data source and if that is
not available then in the regular data source. This works correctly. Please 
check and see if this is ok.

https://codereview.chromium.org/2501343003/diff/610001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2501343003/diff/610001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:443: // navigation is
committed, i.e we have a renderer process ready to service
On 2016/11/30 17:40:22, clamy (slow) wrote:
> nit: s/is committed/is ready to be committed

Done.

https://codereview.chromium.org/2501343003/diff/610001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2501343003/diff/610001/content/renderer/rende...
content/renderer/render_frame_impl.h:1344: // PlzNavigte: The host id to be used
by the AppCache system. Defaults to 0.
On 2016/11/30 17:40:22, clamy (slow) wrote:
> nit: s/PlzNavigte/PlzNavigate

Removed variable

[ananta, 2016-12-01 05:15:05]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 05:15:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 05:17:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 05:17:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2016-12-01 05:58:51]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 05:59:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 07:49:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 07:49:29]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-12-01 10:02:47]

Thanks! Lgtm. Please wait for michaeln's lgtm for the appcache part before
submitting though :).

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/590001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5005: appcache_host_id_ =
request_params.appcache_host_id;
On 2016/12/01 05:14:59, ananta wrote:
> On 2016/11/30 17:40:22, clamy (slow) wrote:
> > On 2016/11/28 21:36:53, ananta wrote:
> > > On 2016/11/25 13:32:24, clamy (slow) wrote:
> > > > We store the request params in NavigateInternal, so we should manage to
> get
> > > the
> > > > id directly from them and avoid adding an extra parameter to
> > RenderFrameImpl.
> > > 
> > > We save the request params in the pending_navigation_params_ member.
However
> > > that gets reset before the RenderFrameImpl::createApplicationCacheHost()
> > > function is called. Leaving this as is.
> > 
> > You can still access them through the DataSource DocumentState's
> NavigationState
> > (in its NavigationStateImpl version). I would prefer we use them rather than
> > adding a member variable.
> 
> It looks like the right value is available via the provisionalDataSource()
> function in the frame. I changed the code to look for the
> RequestNavigationParams() first in the provisional data source and if that is
> not available then in the regular data source. This works correctly. Please 
> check and see if this is ok.

Absolutely, this is what I meant. Sorry my comment wasn't clear: I forgot to
mention that I was talking about the provisional DataSource and not the current
one.

[michaeln, 2016-12-03 00:58:59]

looks great, thnx for following the ServiceWorker pattern, i have a couple
comments but nothing much

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:176: hosts_[host->host_id()] =
std::move(host);
yup, i'd vote to put the 'pending_host_ids_' collection in here rather than in
the caller, keeps it more encapsulated. RegisterHost could deal with testing and
removing ids at RegisterHost time

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:19: typedef std::map<int,
content::AppCacheDispatcherHost*> ProcessIdToHostMap;
would you mind using a using alias for this?

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:20:
base::LazyInstance<ProcessIdToHostMap> g_process_id_host_map;
too bad we need this, does this do the "leaky" behavior by default?

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:34:
g_process_id_host_map.Get()[process_id] = this;
i think we have thread-safety issues, the message filters are created on the UI
thread, what thread is this map accessed on?

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:74: void
AppCacheDispatcherHost::RegisterPendingHost(int host_id) {
please put the method bodies in the .cc file in the same order as their
declaration in the .h file

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:85: if ((index !=
g_process_id_host_map.Get().end()) && index->second == this)
do we ever expect second != this?

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:96:
RegisterPendingHost(host->host_id());
if this is the only callsite for this->RegisterPendingHost(), i think you could
put the pending_hosts_.insert() call right in this method body and nix the extra
method

or better yet, maybe the BackendImpl can deal with the pendingness all
internally and "do the right thing" when RegisterHost() is called?

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle.h (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle.h:70:
AppCacheNavigationHandleCore* core_;
i'd vote to use a std::unique_ptr<> here to express ownership even though we
explicitly delete it on the other thread

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle_core.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:32: if
(precreated_host_.get())
is the test and reset call explicitly needed, std::unique_ptr<> should do this
for us?

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:45:
AppCacheDispatcherHost::GetHostForProcess(process_id);
ah... the map is accessed on the IO thread

Maybe we don't need the map since we have a AppCacheService ptr and from that
can get to the AppcacheBackend, see AppCacheServiceImpl::GetBackend(id). If
collection of 'pending_host_ids_' is kept in the backend, we might not need to
muck with the DispatcherHost at all.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle_core.h (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.h:49: //
AppCacheFrontend methods
i'd vote to make these 'private' or 'protected' since they're not intended to be
called thru AppCacheNavigationHandleCore instance ptrs, the reason to implement
this intf is a an impl detail

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:445:
std::unique_ptr<AppCacheNavigationHandle> appcache_handle_;
maybe put this right next to the service_worker_handle_ data member

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:521: if
(common_params_.url.SchemeIsHTTPOrHTTPS()) {
i think we should use IsSchemeSupportedForAppCache() from
content/public/common/appcache_interface.h

[ananta, 2016-12-03 14:54:57]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 14:55:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2016-12-03 14:55:04]

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:176: hosts_[host->host_id()] =
std::move(host);
On 2016/12/03 00:58:58, michaeln wrote:
> yup, i'd vote to put the 'pending_host_ids_' collection in here rather than in
> the caller, keeps it more encapsulated. RegisterHost could deal with testing
and
> removing ids at RegisterHost time

Done.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:19: typedef std::map<int,
content::AppCacheDispatcherHost*> ProcessIdToHostMap;
On 2016/12/03 00:58:58, michaeln wrote:
> would you mind using a using alias for this?

Done.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:20:
base::LazyInstance<ProcessIdToHostMap> g_process_id_host_map;
On 2016/12/03 00:58:58, michaeln wrote:
> too bad we need this, does this do the "leaky" behavior by default?

By default it uses the DefaultLazyInstanceTraits which deletes the instance
during exit

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:34:
g_process_id_host_map.Get()[process_id] = this;
On 2016/12/03 00:58:58, michaeln wrote:
> i think we have thread-safety issues, the message filters are created on the
UI
> thread, what thread is this map accessed on?

The map is populated on the UI thread and is only accessed on the IO thread.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:74: void
AppCacheDispatcherHost::RegisterPendingHost(int host_id) {
On 2016/12/03 00:58:58, michaeln wrote:
> please put the method bodies in the .cc file in the same order as their
> declaration in the .h file

This function has gone

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:85: if ((index !=
g_process_id_host_map.Get().end()) && index->second == this)
On 2016/12/03 00:58:58, michaeln wrote:
> do we ever expect second != this?

I think there are cases when a new AppDispatcherHost is created for the same
pid.
I had a DCHECK in here for an error check an it was firing at times. Changed
this to a 
DCHECK. Will see if the run passes.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:96:
RegisterPendingHost(host->host_id());
On 2016/12/03 00:58:58, michaeln wrote:
> if this is the only callsite for this->RegisterPendingHost(), i think you
could
> put the pending_hosts_.insert() call right in this method body and nix the
extra
> method
> 
> or better yet, maybe the BackendImpl can deal with the pendingness all
> internally and "do the right thing" when RegisterHost() is called?

I moved the pending hosts to the backend. However we still need the
RegisterPrecreatedHost
method here because the renderer has not registered with the host and in turn
the appcache service when this is called (during commitnavigation).

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle.h (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle.h:70:
AppCacheNavigationHandleCore* core_;
On 2016/12/03 00:58:58, michaeln wrote:
> i'd vote to use a std::unique_ptr<> here to express ownership even though we
> explicitly delete it on the other thread

Done.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle_core.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:32: if
(precreated_host_.get())
On 2016/12/03 00:58:58, michaeln wrote:
> is the test and reset call explicitly needed, std::unique_ptr<> should do this
> for us?

Done.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:45:
AppCacheDispatcherHost::GetHostForProcess(process_id);
On 2016/12/03 00:58:58, michaeln wrote:
> ah... the map is accessed on the IO thread
> 
> Maybe we don't need the map since we have a AppCacheService ptr and from that
> can get to the AppcacheBackend, see AppCacheServiceImpl::GetBackend(id). If
> collection of 'pending_host_ids_' is kept in the backend, we might not need to
> muck with the DispatcherHost at all.

The renderer process has not registered with the host at this point. Hence the
service
will not have any information about the backend. I tried this change and found
that
the renderer process registers after the top level navigation commit request
happens

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle_core.h (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.h:49: //
AppCacheFrontend methods
On 2016/12/03 00:58:58, michaeln wrote:
> i'd vote to make these 'private' or 'protected' since they're not intended to
be
> called thru AppCacheNavigationHandleCore instance ptrs, the reason to
implement
> this intf is a an impl detail

Changed to protected:

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:445:
std::unique_ptr<AppCacheNavigationHandle> appcache_handle_;
On 2016/12/03 00:58:58, michaeln wrote:
> maybe put this right next to the service_worker_handle_ data member

Done.

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:521: if
(common_params_.url.SchemeIsHTTPOrHTTPS()) {
On 2016/12/03 00:58:58, michaeln wrote:
> i think we should use IsSchemeSupportedForAppCache() from
> content/public/common/appcache_interface.h

Done.

[commit-bot: I haz the power, 2016-12-03 15:27:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 15:27:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[ananta, 2016-12-03 22:42:29]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[ananta, 2016-12-03 22:42:35]

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/690001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:85: if ((index !=
g_process_id_host_map.Get().end()) && index->second == this)
On 2016/12/03 14:55:04, ananta wrote:
> On 2016/12/03 00:58:58, michaeln wrote:
> > do we ever expect second != this?
> 
> I think there are cases when a new AppDispatcherHost is created for the same
> pid.
> I had a DCHECK in here for an error check an it was firing at times. Changed
> this to a 
> DCHECK. Will see if the run passes.

So there are cases where multiple hosts are created for the same pid due to
multiple renderer processes getting launched for the same navigation. The ones
created earlier are eventually destroyed. The check for index->second == this is
necessary

[commit-bot: I haz the power, 2016-12-03 22:42:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 23:52:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 23:52:37]

Dry run: This issue passed the CQ dry run.

[michaeln, 2016-12-05 21:52:54]

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:46: DCHECK(found !=
pending_hosts_.end());
Since this |id| is given to us by a potentially compromised renderer, if its not
found in pending_host_, it might be better to return false, which indicates a
badipc and will cause the renderer to be killed.
(this comment might not apply depending... see the other comment)

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:34:
g_process_id_host_map.Get()[process_id] = this;
ok, we have to do something about accessing this map from multiple threads,
that's going to cause problems when the ui/io threads collide

i think it'd be great if we can get rid of that map altogether, that would
eliminate thread-safety issues and also the oddness with process ids that are
reused

I was just looking at how this same problem (stashing/retrieving a ptr to the
precreated host) is handled in the sw codebase, there's no global map in that
world.  I think the same technique could be used here.

The central 'service' (context_core) object is used as a means to store/retrieve
the 'host' instance, the service contains a map of ptrs to the navhandlecores
which own the precreated hosts. At RegisterHost() time (OnProviderCreated in
sw'speak) GetNavigationHandleCore() is used to retrieve the precreated object.

Take a look at how these ServiceWorkerContextCore methods are used:

  void AddNavigationHandleCore(int service_worker_provider_id,
                               ServiceWorkerNavigationHandleCore* handle);
  void RemoveNavigationHandleCore(int service_worker_provider_id);
  ServiceWorkerNavigationHandleCore* GetNavigationHandleCore(
      int service_worker_provider_id);

wdyt?

[ananta, 2016-12-06 01:24:39]

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
File content/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
content/browser/appcache/appcache_backend_impl.cc:46: DCHECK(found !=
pending_hosts_.end());
On 2016/12/05 21:52:54, michaeln wrote:
> Since this |id| is given to us by a potentially compromised renderer, if its
not
> found in pending_host_, it might be better to return false, which indicates a
> badipc and will cause the renderer to be killed.
> (this comment might not apply depending... see the other comment)

The pending hosts are no longer needed with your suggestion about the service
worker pattern.

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/730001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:34:
g_process_id_host_map.Get()[process_id] = this;
On 2016/12/05 21:52:54, michaeln wrote:
> ok, we have to do something about accessing this map from multiple threads,
> that's going to cause problems when the ui/io threads collide
> 
> i think it'd be great if we can get rid of that map altogether, that would
> eliminate thread-safety issues and also the oddness with process ids that are
> reused
> 
> I was just looking at how this same problem (stashing/retrieving a ptr to the
> precreated host) is handled in the sw codebase, there's no global map in that
> world.  I think the same technique could be used here.
> 
> The central 'service' (context_core) object is used as a means to
store/retrieve
> the 'host' instance, the service contains a map of ptrs to the navhandlecores
> which own the precreated hosts. At RegisterHost() time (OnProviderCreated in
> sw'speak) GetNavigationHandleCore() is used to retrieve the precreated object.
> 
> Take a look at how these ServiceWorkerContextCore methods are used:
> 
>   void AddNavigationHandleCore(int service_worker_provider_id,
>                                ServiceWorkerNavigationHandleCore* handle);
>   void RemoveNavigationHandleCore(int service_worker_provider_id);
>   ServiceWorkerNavigationHandleCore* GetNavigationHandleCore(
>       int service_worker_provider_id);
> 
> wdyt?

Done. Thanks for the tip. PTAL

[ananta, 2016-12-06 01:24:47]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 01:25:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 02:46:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 02:46:15]

Dry run: This issue passed the CQ dry run.

[michaeln, 2016-12-06 21:10:46]

lgtm, but please delete that xtra method if its not needed

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:69: void
AppCacheDispatcherHost::RegisterPrecreatedHost(
is this method needed given the code in OnRegisterHost which calls the backend
directly? i don't think it is

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:87: }
i spent some time looking into this error condition wondering, but i think its
fine as you have it

--------------
i'm not sure if this condition warrants a 'badmessage' or not? if host_id is
negative (a plznav generated id) and we don't find it in our map of precreated
hosts, how bad is that? i think if that happens, the lifetime of the 'handle'
would have to be borked such that the map entry got deleted, i'm not sure we
should let this slide silently by?

ah, ok, i guess this could be expected in some cases as this is what happens in
corresponding the sw code

    // If no host is found, the navigation has been cancelled in the meantime.
    // Just return as the navigation will be stopped in the renderer as well.
    if (provider_host == nullptr)
      return;

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle_core.cc (right):

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:46: auto index =
g_appcache_handle_map.Get().find(appcache_host_id_);
does .erase(id) work?

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:69: return
std::move(instance->precreated_host_);
ah... you leave the entry in the map but with a nullptr

[ananta, 2016-12-06 21:38:06]

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
File content/browser/appcache/appcache_dispatcher_host.cc (right):

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:69: void
AppCacheDispatcherHost::RegisterPrecreatedHost(
On 2016/12/06 21:10:46, michaeln wrote:
> is this method needed given the code in OnRegisterHost which calls the backend
> directly? i don't think it is

Done.

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_dispatcher_host.cc:87: }
On 2016/12/06 21:10:46, michaeln wrote:
> i spent some time looking into this error condition wondering, but i think its
> fine as you have it
> 
> --------------
> i'm not sure if this condition warrants a 'badmessage' or not? if host_id is
> negative (a plznav generated id) and we don't find it in our map of precreated
> hosts, how bad is that? i think if that happens, the lifetime of the 'handle'
> would have to be borked such that the map entry got deleted, i'm not sure we
> should let this slide silently by?
> 
> ah, ok, i guess this could be expected in some cases as this is what happens
in
> corresponding the sw code
> 
>     // If no host is found, the navigation has been cancelled in the meantime.
>     // Just return as the navigation will be stopped in the renderer as well.
>     if (provider_host == nullptr)
>       return;
> 

Acknowledged

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
File content/browser/appcache/appcache_navigation_handle_core.cc (right):

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:46: auto index =
g_appcache_handle_map.Get().find(appcache_host_id_);
On 2016/12/06 21:10:46, michaeln wrote:
> does .erase(id) work?

Done.

https://codereview.chromium.org/2501343003/diff/770001/content/browser/appcac...
content/browser/appcache/appcache_navigation_handle_core.cc:69: return
std::move(instance->precreated_host_);
On 2016/12/06 21:10:46, michaeln wrote:
> ah... you leave the entry in the map but with a nullptr

Yes. The map entry will go away when the object is deleted.

[ananta, 2016-12-06 21:38:32]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 21:39:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaeln, 2016-12-06 21:44:55]

Thnx for doing this!

[commit-bot: I haz the power, 2016-12-07 00:10:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 00:10:52]

Dry run: This issue passed the CQ dry run.

[ananta, 2016-12-07 00:16:55]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-12-07 00:16:56]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2501343003/#ps790001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-12-07 00:17:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 01:05:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 01:05:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ananta, 2016-12-07 01:11:50]

ananta@chromium.org changed reviewers:
+ wfh@chromium.org

[ananta, 2016-12-07 01:11:53]

+wfh for security owners (frame_messages.h)

[Will Harris, 2016-12-07 01:17:56]

frame_messages.h lgtm

[Will Harris, 2016-12-07 01:18:09]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2016-12-07 01:18:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Will Harris, 2016-12-07 01:19:01]

please update the first line of the changelist description to be the patch
description as this is used in the git commit.

[Will Harris, 2016-12-07 01:19:12]

The CQ bit was unchecked by wfh@chromium.org

[ananta, 2016-12-07 01:23:01]

Description was changed from

==========
michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
newly added AppCacheNavigationHandle class. This class is created on the UI
thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are
controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed
to the 
passed to the renderer in the FrameMsg_CommitNavigation IPC in the
RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle
core.
   The AppCacheNavigationHandle class is created when the
NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class
which is used for
   HTTP requests until the renderer registers with us. When the navigation is
committed this
   host is passed to the AppCacheBackImpl class via
AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the
precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the
AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.
  

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: AppCache support

michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
newly added AppCacheNavigationHandle class. This class is created on the UI
thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are
controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed
to the 
passed to the renderer in the FrameMsg_CommitNavigation IPC in the
RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle
core.
   The AppCacheNavigationHandle class is created when the
NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class
which is used for
   HTTP requests until the renderer registers with us. When the navigation is
committed this
   host is passed to the AppCacheBackImpl class via
AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the
precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the
AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.
  

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-12-07 01:23:26]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2016-12-07 01:24:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 01:29:32]

CQ is committing da patch.

Bot data: {"patchset_id": 790001, "attempt_start_ts": 1481073806221390,
"parent_rev": "1b91105f409662b02eb8ded1da00b12841276b41", "commit_rev":
"be70c4a3a9080be7b86dd6acb35e5c4754dded90"}

[commit-bot: I haz the power, 2016-12-07 01:30:05]

Description was changed from

==========
PlzNavigate: AppCache support

michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
newly added AppCacheNavigationHandle class. This class is created on the UI
thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are
controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed
to the 
passed to the renderer in the FrameMsg_CommitNavigation IPC in the
RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle
core.
   The AppCacheNavigationHandle class is created when the
NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class
which is used for
   HTTP requests until the renderer registers with us. When the navigation is
committed this
   host is passed to the AppCacheBackImpl class via
AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the
precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the
AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.
  

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: AppCache support

michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
newly added AppCacheNavigationHandle class. This class is created on the UI
thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are
controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed
to the 
passed to the renderer in the FrameMsg_CommitNavigation IPC in the
RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle
core.
   The AppCacheNavigationHandle class is created when the
NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class
which is used for
   HTTP requests until the renderer registers with us. When the navigation is
committed this
   host is passed to the AppCacheBackImpl class via
AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the
precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the
AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.
  

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-12-07 01:30:08]

Committed patchset #40 (id:790001)

[commit-bot: I haz the power, 2016-12-07 01:32:51]

Description was changed from

==========
PlzNavigate: AppCache support

michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
newly added AppCacheNavigationHandle class. This class is created on the UI
thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are
controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed
to the 
passed to the renderer in the FrameMsg_CommitNavigation IPC in the
RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle
core.
   The AppCacheNavigationHandle class is created when the
NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class
which is used for
   HTTP requests until the renderer registers with us. When the navigation is
committed this
   host is passed to the AppCacheBackImpl class via
AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the
precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the
AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.
  

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: AppCache support

michaeln : Please review the AppCache changes.
nasko/clamy: Please review the rest of the code.

The AppCache system has a concept of host ids, which map to different DOM's
being loaded
in the renderer. Whenever a navigation is initiated by the renderer we get a new
host id
which is passed by the appcache code in the renderer via the
AppCacheHostMsg_RegisterHost
message. This does not work correctly for PlzNavigate as we don't have a
renderer when the
navigation request is instantiated. To workaround this we generate host ids in
the newly
newly added AppCacheNavigationHandle class. This class is created on the UI
thread and has
a counterpart AppCacheNavigationHandleCore on the IO thread. Their lifetimes are
controlled
by the lifetime of the NavigationHandle class. The host id is eventually passed
to the
passed to the renderer in the FrameMsg_CommitNavigation IPC in the
RequestNavigationParams structure.
This eventually is passed to the WebApplicationCacheHostImpl class.

This patch adds support for AppCache in PlzNavigate. The main changes are as
below:
1. Addition of new classes AppCacheNavigationHandle and AppCacheNavigationHandle
core.
   The AppCacheNavigationHandle class is created when the
NavigationHandleImpl::InitAppCacheHandle()
   method is called.

2. The AppCacheNavigationHandleCore class precreates the AppCacheHost class
which is used for
   HTTP requests until the renderer registers with us. When the navigation is
committed this
   host is passed to the AppCacheBackImpl class via
AppCacheDispatcherHost::RegisterPrecreatedHost().

3. The other AppCache changes are as below: Please note that these changes are
all for PlzNavigate.
   1. Provide functionality in the AppCacheDispatcherHost class register the
precreated host instance
      This is provided via the function RegisterPrecreatedHost()
   2. Provide functionality in the AppCacheHost class to set the
AppCacheFrontend pointer. This is required
      when the precreated host is registered with the AppCacheBackendImpl class.

BUG=608375
TEST=manual
The test case at
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_html_manifest works
fine with PlzNavigate with this patch.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/292a917258e6ce3395a589fe40623d0697df044b
Cr-Commit-Position: refs/heads/master@{#436821}
==========

[commit-bot: I haz the power, 2016-12-07 01:32:53]

Patchset 40 (id:??) landed as
https://crrev.com/292a917258e6ce3395a589fe40623d0697df044b
Cr-Commit-Position: refs/heads/master@{#436821}