Add a unique frame id and save it on HistoryItem.

Add a unique frame id and save it on HistoryItem.

This id is currently unused, but is a prerequisite for separating frame tree
state from per-frame state in HistoryItem.

BUG=312929
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161180

[abarth-chromium, 2013-10-29 21:53:59]

LGTM assuming creis is happy with this direction.

[Charlie Reis, 2013-10-29 22:35:03]

https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h...
Source/core/frame/Frame.h:115: int64_t id() const { return m_id; }
It would probably help to document the scope of this ID (e.g., per process or
unique across all processes).

Looks like it's specific to this renderer, so I'm not sure how this fits into
the plans yet.  We already have both a routing ID and a
WebFrame::embedderIdentifier for each frame that are renderer-specific, and this
is a third ID.  Soon we'll have a browser-wide FrameTreeNode ID in the browser
process as well.

Our intention was to eliminate WebFrame::embedderIdentifier in favor of the
routing ID, so that we have two IDs per frame (renderer-specific routing ID and
browser-global FrameTreeNode ID).  Do we need this additional one long term, or
is it a stepping stone?  Can we get rid of WebFrame::embedderIdentifier if we
have this?

[Charlie Reis, 2013-10-29 22:36:18]

CC'ing site-isolation-reviews@chromium.org.

[Nate Chapin, 2013-10-29 22:46:23]

On 2013/10/29 22:35:03, creis wrote:
> https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h
> File Source/core/frame/Frame.h (right):
> 
>
https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h...
> Source/core/frame/Frame.h:115: int64_t id() const { return m_id; }
> It would probably help to document the scope of this ID (e.g., per process or
> unique across all processes).
> 
> Looks like it's specific to this renderer, so I'm not sure how this fits into
> the plans yet.  We already have both a routing ID and a
> WebFrame::embedderIdentifier for each frame that are renderer-specific, and
this
> is a third ID.  Soon we'll have a browser-wide FrameTreeNode ID in the browser
> process as well.
> 
> Our intention was to eliminate WebFrame::embedderIdentifier in favor of the
> routing ID, so that we have two IDs per frame (renderer-specific routing ID
and
> browser-global FrameTreeNode ID).  Do we need this additional one long term,
or
> is it a stepping stone?  Can we get rid of WebFrame::embedderIdentifier if we
> have this?


This is unique per renderer and primarily intended as a stepping stone. If there
are existing blink-visible unique identifiers that I should be using instead,
I'm happy to switch it. 

It sounds to me like it might make sense to use this as a stopgap, then merge it
with FrameTreeNode or routing ID in the near future....maybe once more of the
history logic moves to the chromium repo?

[Charlie Reis, 2013-10-30 16:25:42]

On 2013/10/29 22:46:23, Nate Chapin wrote:
> On 2013/10/29 22:35:03, creis wrote:
> >
https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h
> > File Source/core/frame/Frame.h (right):
> > 
> >
>
https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h...
> > Source/core/frame/Frame.h:115: int64_t id() const { return m_id; }
> > It would probably help to document the scope of this ID (e.g., per process
or
> > unique across all processes).
> > 
> > Looks like it's specific to this renderer, so I'm not sure how this fits
into
> > the plans yet.  We already have both a routing ID and a
> > WebFrame::embedderIdentifier for each frame that are renderer-specific, and
> this
> > is a third ID.  Soon we'll have a browser-wide FrameTreeNode ID in the
browser
> > process as well.
> > 
> > Our intention was to eliminate WebFrame::embedderIdentifier in favor of the
> > routing ID, so that we have two IDs per frame (renderer-specific routing ID
> and
> > browser-global FrameTreeNode ID).  Do we need this additional one long term,
> or
> > is it a stepping stone?  Can we get rid of WebFrame::embedderIdentifier if
we
> > have this?
> 
> 
> This is unique per renderer and primarily intended as a stepping stone. If
there
> are existing blink-visible unique identifiers that I should be using instead,
> I'm happy to switch it. 

[+darin]

Well, we do already have WebFrame::embedderIdentifier as a per-renderer frame
ID.  I suppose you can't access that in HistoryItem, but maybe
WebFrame::embedderIdentifier() should return this one instead of having its own
member variable?  It just seems like we're ending up with a lot of frame IDs in
the short term, which could easily cause confusion and bugs if a developer isn't
sure which one they have.


> It sounds to me like it might make sense to use this as a stopgap, then merge
it
> with FrameTreeNode or routing ID in the near future....maybe once more of the
> history logic moves to the chromium repo?

Yes, longer term I think we can combine it with one of those.

[darin (slow to review), 2013-10-30 16:41:07]

On Wed, Oct 30, 2013 at 9:25 AM, <creis@chromium.org> wrote:

> On 2013/10/29 22:46:23, Nate Chapin wrote:
>
>> On 2013/10/29 22:35:03, creis wrote:
>> >
>>
> https://codereview.chromium.**org/46783003/diff/20001/**
>
Source/core/frame/Frame.h<https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h>
>
>> > File Source/core/frame/Frame.h (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/46783003/diff/20001/**
>
Source/core/frame/Frame.h#**newcode115<https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h#newcode115>
>
>> > Source/core/frame/Frame.h:115: int64_t id() const { return m_id; }
>> > It would probably help to document the scope of this ID (e.g., per
>> process
>>
> or
>
>> > unique across all processes).
>> >
>> > Looks like it's specific to this renderer, so I'm not sure how this fits
>>
> into
>
>> > the plans yet.  We already have both a routing ID and a
>> > WebFrame::embedderIdentifier for each frame that are renderer-specific,
>> and
>> this
>> > is a third ID.  Soon we'll have a browser-wide FrameTreeNode ID in the
>>
> browser
>
>> > process as well.
>> >
>> > Our intention was to eliminate WebFrame::embedderIdentifier in favor of
>> the
>> > routing ID, so that we have two IDs per frame (renderer-specific
>> routing ID
>> and
>> > browser-global FrameTreeNode ID).  Do we need this additional one long
>> term,
>> or
>> > is it a stepping stone?  Can we get rid of WebFrame::embedderIdentifier
>> if
>>
> we
>
>> > have this?
>>
>
>
>  This is unique per renderer and primarily intended as a stepping stone. If
>>
> there
>
>> are existing blink-visible unique identifiers that I should be using
>> instead,
>> I'm happy to switch it.
>>
>
> [+darin]
>
> Well, we do already have WebFrame::embedderIdentifier as a per-renderer
> frame
> ID.  I suppose you can't access that in HistoryItem, but maybe
> WebFrame::embedderIdentifier() should return this one instead of having
> its own
> member variable?  It just seems like we're ending up with a lot of frame
> IDs in
> the short term, which could easily cause confusion and bugs if a developer
> isn't
> sure which one they have.
>

+1


>
>
>  It sounds to me like it might make sense to use this as a stopgap, then
>> merge
>>
> it
>
>> with FrameTreeNode or routing ID in the near future....maybe once more of
>> the
>> history logic moves to the chromium repo?
>>
>
> Yes, longer term I think we can combine it with one of those.
>
>
>
https://codereview.chromium.**org/46783003/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Nate Chapin, 2013-10-30 16:44:03]

On 2013/10/30 16:41:07, darin wrote:
> On Wed, Oct 30, 2013 at 9:25 AM, <mailto:creis@chromium.org> wrote:
> 
> > On 2013/10/29 22:46:23, Nate Chapin wrote:
> >
> >> On 2013/10/29 22:35:03, creis wrote:
> >> >
> >>
> > https://codereview.chromium.**org/46783003/diff/20001/**
> >
>
Source/core/frame/Frame.h<https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h>
> >
> >> > File Source/core/frame/Frame.h (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.**org/46783003/diff/20001/**
> >
>
Source/core/frame/Frame.h#**newcode115<https://codereview.chromium.org/46783003/diff/20001/Source/core/frame/Frame.h#newcode115>
> >
> >> > Source/core/frame/Frame.h:115: int64_t id() const { return m_id; }
> >> > It would probably help to document the scope of this ID (e.g., per
> >> process
> >>
> > or
> >
> >> > unique across all processes).
> >> >
> >> > Looks like it's specific to this renderer, so I'm not sure how this fits
> >>
> > into
> >
> >> > the plans yet.  We already have both a routing ID and a
> >> > WebFrame::embedderIdentifier for each frame that are renderer-specific,
> >> and
> >> this
> >> > is a third ID.  Soon we'll have a browser-wide FrameTreeNode ID in the
> >>
> > browser
> >
> >> > process as well.
> >> >
> >> > Our intention was to eliminate WebFrame::embedderIdentifier in favor of
> >> the
> >> > routing ID, so that we have two IDs per frame (renderer-specific
> >> routing ID
> >> and
> >> > browser-global FrameTreeNode ID).  Do we need this additional one long
> >> term,
> >> or
> >> > is it a stepping stone?  Can we get rid of WebFrame::embedderIdentifier
> >> if
> >>
> > we
> >
> >> > have this?
> >>
> >
> >
> >  This is unique per renderer and primarily intended as a stepping stone. If
> >>
> > there
> >
> >> are existing blink-visible unique identifiers that I should be using
> >> instead,
> >> I'm happy to switch it.
> >>
> >
> > [+darin]
> >
> > Well, we do already have WebFrame::embedderIdentifier as a per-renderer
> > frame
> > ID.  I suppose you can't access that in HistoryItem, but maybe
> > WebFrame::embedderIdentifier() should return this one instead of having
> > its own
> > member variable?  It just seems like we're ending up with a lot of frame
> > IDs in
> > the short term, which could easily cause confusion and bugs if a developer
> > isn't
> > sure which one they have.
> >
> 
> +1
> 

Ok, I'll see what's required to plumb the embedderIdentifier() down into core/.

[Charlie Reis, 2013-10-30 17:01:54]

On 2013/10/30 16:44:03, Nate Chapin wrote:
> > > Well, we do already have WebFrame::embedderIdentifier as a per-renderer
> > > frame
> > > ID.  I suppose you can't access that in HistoryItem, but maybe
> > > WebFrame::embedderIdentifier() should return this one instead of having
> > > its own
> > > member variable?  It just seems like we're ending up with a lot of frame
> > > IDs in
> > > the short term, which could easily cause confusion and bugs if a developer
> > > isn't
> > > sure which one they have.
> > >
> > 
> > +1
> > 
> 
> Ok, I'll see what's required to plumb the embedderIdentifier() down into
core/.

Would it be easier to keep the ID you just added and get rid of
WebFrame::m_embedderIdentifier?  Then WebFrame::embedderIdentifier() could just
return your ID.  (I'm not sure about all the ways that m_embedderIdentifier is
set and whether you'll need to support those, but that could be an easier
option.)

[Nate Chapin, 2013-10-30 17:21:23]

On 2013/10/30 17:01:54, creis wrote:
> On 2013/10/30 16:44:03, Nate Chapin wrote:
> > > > Well, we do already have WebFrame::embedderIdentifier as a per-renderer
> > > > frame
> > > > ID.  I suppose you can't access that in HistoryItem, but maybe
> > > > WebFrame::embedderIdentifier() should return this one instead of having
> > > > its own
> > > > member variable?  It just seems like we're ending up with a lot of frame
> > > > IDs in
> > > > the short term, which could easily cause confusion and bugs if a
developer
> > > > isn't
> > > > sure which one they have.
> > > >
> > > 
> > > +1
> > > 
> > 
> > Ok, I'll see what's required to plumb the embedderIdentifier() down into
> core/.
> 
> Would it be easier to keep the ID you just added and get rid of
> WebFrame::m_embedderIdentifier?  Then WebFrame::embedderIdentifier() could
just
> return your ID.  (I'm not sure about all the ways that m_embedderIdentifier is
> set and whether you'll need to support those, but that could be an easier
> option.)

m_embedderIdentifier is always set before the WebCore::Frame is constructed, so
I'd need some way to hold it temporarily on WebFrame before transferring it.
That doesn't seem like much of an advantage.

[Nate Chapin, 2013-10-30 17:43:08]

On 2013/10/30 17:21:23, Nate Chapin wrote:
> On 2013/10/30 17:01:54, creis wrote:
> > On 2013/10/30 16:44:03, Nate Chapin wrote:
> > > > > Well, we do already have WebFrame::embedderIdentifier as a
per-renderer
> > > > > frame
> > > > > ID.  I suppose you can't access that in HistoryItem, but maybe
> > > > > WebFrame::embedderIdentifier() should return this one instead of
having
> > > > > its own
> > > > > member variable?  It just seems like we're ending up with a lot of
frame
> > > > > IDs in
> > > > > the short term, which could easily cause confusion and bugs if a
> developer
> > > > > isn't
> > > > > sure which one they have.
> > > > >
> > > > 
> > > > +1
> > > > 
> > > 
> > > Ok, I'll see what's required to plumb the embedderIdentifier() down into
> > core/.
> > 
> > Would it be easier to keep the ID you just added and get rid of
> > WebFrame::m_embedderIdentifier?  Then WebFrame::embedderIdentifier() could
> just
> > return your ID.  (I'm not sure about all the ways that m_embedderIdentifier
is
> > set and whether you'll need to support those, but that could be an easier
> > option.)
> 
> m_embedderIdentifier is always set before the WebCore::Frame is constructed,
so
> I'd need some way to hold it temporarily on WebFrame before transferring it.
> That doesn't seem like much of an advantage.

Updated. This version leaves WebCore::Frame unmodified, and instead exposes
embedderIdentifier() to core/ via FrameLoaderClient.

[Charlie Reis, 2013-10-30 22:03:53]

Great, thanks!  LGTM, but either Darin or Adam should probably approve the
change to FrameLoaderClient.

[darin (slow to review), 2013-10-30 22:05:06]

It seems like FrameID should be pushed down to Frame instead of using the
FrameLoaderClient to retrieve it from core/.

-Darin

[Nate Chapin, 2013-10-30 22:06:50]

On 2013/10/30 22:05:06, darin wrote:
> It seems like FrameID should be pushed down to Frame instead of using the
> FrameLoaderClient to retrieve it from core/.
> 
> -Darin

That could be done, but this is the least invasive way to share the id, given
how the id is set currently (namely, before the WebCore::Frame is created).

[darin (slow to review), 2013-10-30 22:07:40]

Couldn't Frame have a setFrameID method?


On Wed, Oct 30, 2013 at 3:06 PM, <japhet@chromium.org> wrote:

> On 2013/10/30 22:05:06, darin wrote:
>
>> It seems like FrameID should be pushed down to Frame instead of using the
>> FrameLoaderClient to retrieve it from core/.
>>
>
>  -Darin
>>
>
> That could be done, but this is the least invasive way to share the id,
> given
> how the id is set currently (namely, before the WebCore::Frame is created).
>
>
https://codereview.chromium.**org/46783003/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-10-30 22:08:25]

or rather Frame::setIdentifier? My point is that if core/ needs to know
about the concept of frame identifiers, then perhaps it should know about
frame identifiers ;-)


On Wed, Oct 30, 2013 at 3:07 PM, Darin Fisher <darin@chromium.org> wrote:

> Couldn't Frame have a setFrameID method?
>
>
> On Wed, Oct 30, 2013 at 3:06 PM, <japhet@chromium.org> wrote:
>
>> On 2013/10/30 22:05:06, darin wrote:
>>
>>> It seems like FrameID should be pushed down to Frame instead of using the
>>> FrameLoaderClient to retrieve it from core/.
>>>
>>
>>  -Darin
>>>
>>
>> That could be done, but this is the least invasive way to share the id,
>> given
>> how the id is set currently (namely, before the WebCore::Frame is
>> created).
>>
>>
https://codereview.chromium.**org/46783003/<https://codereview.chromium.org/4...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Nate Chapin, 2013-10-30 22:09:06]

On 2013/10/30 22:07:40, darin wrote:
> Couldn't Frame have a setFrameID method?

Sure, but that either requires a 3-sided patch or stashing the value somewhere
temporarily until the frame is created.

> 
> 
> On Wed, Oct 30, 2013 at 3:06 PM, <mailto:japhet@chromium.org> wrote:
> 
> > On 2013/10/30 22:05:06, darin wrote:
> >
> >> It seems like FrameID should be pushed down to Frame instead of using the
> >> FrameLoaderClient to retrieve it from core/.
> >>
> >
> >  -Darin
> >>
> >
> > That could be done, but this is the least invasive way to share the id,
> > given
> > how the id is set currently (namely, before the WebCore::Frame is created).
> >
> >
>
https://codereview.chromium.**org/46783003/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2013-10-30 22:12:33]

On 2013/10/30 22:09:06, Nate Chapin wrote:
> On 2013/10/30 22:07:40, darin wrote:
> > Couldn't Frame have a setFrameID method?
> 
> Sure, but that either requires a 3-sided patch or stashing the value somewhere
> temporarily until the frame is created.

OK, I see. Next question: Is there a way for the web/ layer to assign the
TargetFrameID at the right time so that we don't need the FrameLoaderClient
call?

[Nate Chapin, 2013-10-30 22:15:03]

On 2013/10/30 22:12:33, darin wrote:
> On 2013/10/30 22:09:06, Nate Chapin wrote:
> > On 2013/10/30 22:07:40, darin wrote:
> > > Couldn't Frame have a setFrameID method?
> > 
> > Sure, but that either requires a 3-sided patch or stashing the value
somewhere
> > temporarily until the frame is created.
> 
> OK, I see. Next question: Is there a way for the web/ layer to assign the
> TargetFrameID at the right time so that we don't need the FrameLoaderClient
> call?

Absolutely. That means the same value will live on both WebFrameImpl and
WebCore::Frame, but if you prefer that, I can do that.

[darin (slow to review), 2013-10-30 22:18:16]

I meant, can we assign it to the HistoryItem?


On Wed, Oct 30, 2013 at 3:15 PM, <japhet@chromium.org> wrote:

> On 2013/10/30 22:12:33, darin wrote:
>
>> On 2013/10/30 22:09:06, Nate Chapin wrote:
>> > On 2013/10/30 22:07:40, darin wrote:
>> > > Couldn't Frame have a setFrameID method?
>> >
>> > Sure, but that either requires a 3-sided patch or stashing the value
>>
> somewhere
>
>> > temporarily until the frame is created.
>>
>
>  OK, I see. Next question: Is there a way for the web/ layer to assign the
>> TargetFrameID at the right time so that we don't need the
>> FrameLoaderClient
>> call?
>>
>
> Absolutely. That means the same value will live on both WebFrameImpl and
> WebCore::Frame, but if you prefer that, I can do that.
>
>
https://codereview.chromium.**org/46783003/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Nate Chapin, 2013-10-30 22:23:39]

On 2013/10/30 22:18:16, darin wrote:
> I meant, can we assign it to the HistoryItem?

Hrm. I don't think web/ can reach in early enough to set it on the HistoryItem
when I want to use it in
https://codereview.chromium.org/28983004/diff/60001/Source/core/loader/Histor...

> 
> 
> On Wed, Oct 30, 2013 at 3:15 PM, <mailto:japhet@chromium.org> wrote:
> 
> > On 2013/10/30 22:12:33, darin wrote:
> >
> >> On 2013/10/30 22:09:06, Nate Chapin wrote:
> >> > On 2013/10/30 22:07:40, darin wrote:
> >> > > Couldn't Frame have a setFrameID method?
> >> >
> >> > Sure, but that either requires a 3-sided patch or stashing the value
> >>
> > somewhere
> >
> >> > temporarily until the frame is created.
> >>
> >
> >  OK, I see. Next question: Is there a way for the web/ layer to assign the
> >> TargetFrameID at the right time so that we don't need the
> >> FrameLoaderClient
> >> call?
> >>
> >
> > Absolutely. That means the same value will live on both WebFrameImpl and
> > WebCore::Frame, but if you prefer that, I can do that.
> >
> >
>
https://codereview.chromium.**org/46783003/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[Nate Chapin, 2013-10-31 19:21:33]

On 2013/10/30 22:23:39, Nate Chapin wrote:
> On 2013/10/30 22:18:16, darin wrote:
> > I meant, can we assign it to the HistoryItem?
> 
> Hrm. I don't think web/ can reach in early enough to set it on the HistoryItem
> when I want to use it in
>
https://codereview.chromium.org/28983004/diff/60001/Source/core/loader/Histor...
> 
> > 
> > 
> > On Wed, Oct 30, 2013 at 3:15 PM, <mailto:japhet@chromium.org> wrote:
> > 
> > > On 2013/10/30 22:12:33, darin wrote:
> > >
> > >> On 2013/10/30 22:09:06, Nate Chapin wrote:
> > >> > On 2013/10/30 22:07:40, darin wrote:
> > >> > > Couldn't Frame have a setFrameID method?
> > >> >
> > >> > Sure, but that either requires a 3-sided patch or stashing the value
> > >>
> > > somewhere
> > >
> > >> > temporarily until the frame is created.
> > >>
> > >
> > >  OK, I see. Next question: Is there a way for the web/ layer to assign the
> > >> TargetFrameID at the right time so that we don't need the
> > >> FrameLoaderClient
> > >> call?
> > >>
> > >
> > > Absolutely. That means the same value will live on both WebFrameImpl and
> > > WebCore::Frame, but if you prefer that, I can do that.
> > >
> > >
> >
>
https://codereview.chromium.**org/46783003/%253Chttps://codereview.chromium.o...>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.

New version adds a FrameInit object that is the sole parameter to
Frame::create(). This object is RefCounted and stores, among other things, the
FrameLoaderClient and the frame id.

[abarth-chromium, 2013-11-01 16:36:44]

LGTM

I wonder if FrameInit will grow into the state that preserved when we flip
between inprocess and out-of-process frames.  That's something we'll need to
work out as we go.

[Nate Chapin, 2013-11-01 16:38:49]

On 2013/11/01 16:36:44, abarth wrote:
> LGTM
> 
> I wonder if FrameInit will grow into the state that preserved when we flip
> between inprocess and out-of-process frames.  That's something we'll need to
> work out as we go.

Hmm.  I hadn't thought about it that way, but it certainly could work.

[commit-bot: I haz the power, 2013-11-01 16:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/46783003/390001

[commit-bot: I haz the power, 2013-11-01 17:38:03]

Change committed as 161180