Introduce WorkerStoragePartitionId and use it in SharedWorkerInstance.

Introduce WorkerStoragePartitionId and use it in SharedWorkerInstance.

WorkerStoragePartitionId can be used to identify each WorkerStoragePartitions.
We can hold WorkerStoragePartitionId without extending the lifetime of all objects in the WorkerStoragePartition.

BUG=366182, 327256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265930

[horo, 2014-04-24 03:21:25]

kinuko@
Could you please review?

[kinuko, 2014-04-24 04:11:46]

https://codereview.chromium.org/258513002/diff/20001/content/browser/worker_h...
File content/browser/worker_host/worker_storage_partition.h (right):

https://codereview.chromium.org/258513002/diff/20001/content/browser/worker_h...
content/browser/worker_host/worker_storage_partition.h:112: // extending the
lifetime of all objects in the WorkerStoragePartition.
Probably we should also note that that means holding a PartitionId doesn't mean
corresponding partition and its members are kept alive.

https://codereview.chromium.org/258513002/diff/20001/content/browser/worker_h...
content/browser/worker_host/worker_storage_partition.h:117: const
WorkerStoragePartitionId& operator=(
I don't feel we need to explicitly declare these since these could get
auto-inlined without member type definitions?

[horo, 2014-04-24 04:26:32]

https://codereview.chromium.org/258513002/diff/20001/content/browser/worker_h...
File content/browser/worker_host/worker_storage_partition.h (right):

https://codereview.chromium.org/258513002/diff/20001/content/browser/worker_h...
content/browser/worker_host/worker_storage_partition.h:112: // extending the
lifetime of all objects in the WorkerStoragePartition.
On 2014/04/24 04:11:47, kinuko wrote:
> Probably we should also note that that means holding a PartitionId doesn't
mean
> corresponding partition and its members are kept alive.

Done.

https://codereview.chromium.org/258513002/diff/20001/content/browser/worker_h...
content/browser/worker_host/worker_storage_partition.h:117: const
WorkerStoragePartitionId& operator=(
On 2014/04/24 04:11:47, kinuko wrote:
> I don't feel we need to explicitly declare these since these could get
> auto-inlined without member type definitions?

Done.

[kinuko, 2014-04-24 04:53:38]

lgtm

(I can't see issue 366182, but assume this should fix it)

https://codereview.chromium.org/258513002/diff/40001/content/browser/worker_h...
File content/browser/worker_host/worker_storage_partition.h (right):

https://codereview.chromium.org/258513002/diff/40001/content/browser/worker_h...
content/browser/worker_host/worker_storage_partition.h:114: // corresponding
partitionand its members are kept alive.
nit: missing space between 'partition' and 'and'

[horo, 2014-04-24 05:09:52]

https://codereview.chromium.org/258513002/diff/40001/content/browser/worker_h...
File content/browser/worker_host/worker_storage_partition.h (right):

https://codereview.chromium.org/258513002/diff/40001/content/browser/worker_h...
content/browser/worker_host/worker_storage_partition.h:114: // corresponding
partitionand its members are kept alive.
On 2014/04/24 04:53:38, kinuko wrote:
> nit: missing space between 'partition' and 'and'

Done.

[horo, 2014-04-24 05:09:59]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-24 05:13:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/258513002/60001

[commit-bot: I haz the power, 2014-04-24 05:48:50]

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

[commit-bot: I haz the power, 2014-04-24 05:48:51]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[michaeln, 2014-04-24 08:04:31]

https://codereview.chromium.org/258513002/diff/60001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_message_filter.h (right):

https://codereview.chromium.org/258513002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_message_filter.h:71: const
WorkerStoragePartitionId partition_id_;
The bug has to do with the devtools holding onto references to these objects.
Why are these changes being made to the  message filter class and to the
instances held by the sharedworkerservice to no longer hold references to these
objects?

[horo, 2014-04-24 08:23:13]

https://codereview.chromium.org/258513002/diff/60001/content/browser/shared_w...
File content/browser/shared_worker/shared_worker_message_filter.h (right):

https://codereview.chromium.org/258513002/diff/60001/content/browser/shared_w...
content/browser/shared_worker/shared_worker_message_filter.h:71: const
WorkerStoragePartitionId partition_id_;
On 2014/04/24 08:04:32, michaeln wrote:
> The bug has to do with the devtools holding onto references to these objects.
> Why are these changes being made to the  message filter class and to the
> instances held by the sharedworkerservice to no longer hold references to
these
> objects?

Done.
Oh yes.
SharedWorkerMessageFilter should hold WorkerStoragePartition.

[kinuko, 2014-04-24 08:59:31]

On 2014/04/24 08:23:13, horo wrote:
>
https://codereview.chromium.org/258513002/diff/60001/content/browser/shared_w...
> File content/browser/shared_worker/shared_worker_message_filter.h (right):
> 
>
https://codereview.chromium.org/258513002/diff/60001/content/browser/shared_w...
> content/browser/shared_worker/shared_worker_message_filter.h:71: const
> WorkerStoragePartitionId partition_id_;
> On 2014/04/24 08:04:32, michaeln wrote:
> > The bug has to do with the devtools holding onto references to these
objects.
> > Why are these changes being made to the  message filter class and to the
> > instances held by the sharedworkerservice to no longer hold references to
> these
> > objects?
> 
> Done.
> Oh yes.
> SharedWorkerMessageFilter should hold WorkerStoragePartition.

I haven't looked into deeper but at a quick look I have a feeling that it might
be ok not to hold the reference in SharedWorkerMessageFilter either? ...but to
fix this particular issue not to change the current behavior for the working
part would be probably safer (and it looks we must fix this issue asap, without
other potential breakages).

[horo, 2014-04-24 09:20:19]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-24 09:20:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/258513002/70001

[commit-bot: I haz the power, 2014-04-24 09:30:08]

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

[commit-bot: I haz the power, 2014-04-24 09:30:08]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[horo, 2014-04-24 10:21:29]

yurys@
Please review shared_worker_devtools_manager_unittest.cc

[yurys, 2014-04-24 11:02:37]

On 2014/04/24 10:21:29, horo wrote:
> yurys@
> Please review shared_worker_devtools_manager_unittest.cc

shared_worker_devtools_manager_unittest.cc - lgtm

[horo, 2014-04-24 11:03:00]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-24 11:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/258513002/70001

[commit-bot: I haz the power, 2014-04-24 12:11:38]

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

[commit-bot: I haz the power, 2014-04-24 12:11:39]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[horo, 2014-04-24 12:31:27]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-24 12:31:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/258513002/70001

[commit-bot: I haz the power, 2014-04-24 14:18:35]

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

[commit-bot: I haz the power, 2014-04-24 14:18:36]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[horo, 2014-04-24 14:38:58]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-24 14:39:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/258513002/70001

[commit-bot: I haz the power, 2014-04-24 16:20:13]

Change committed as 265930

[michaeln1, 2014-04-24 21:41:52]

> > Done.
> > Oh yes.
> > SharedWorkerMessageFilter should hold WorkerStoragePartition.

Thnx!

> I haven't looked into deeper but at a quick look I have a feeling that it
might
> be ok not to hold the reference in SharedWorkerMessageFilter either? ...but to
> fix this particular issue not to change the current behavior for the working
> part would be probably safer (and it looks we must fix this issue asap,
without
> other potential breakages).

It might be, but it's a bit of a leap of faith, and there's no need to make that
leap.