When starting a provisional load include the parent frame ID in the IPC and pass it down the th WCO…

When starting a provisional load include the parent frame ID in the IPC and pass it down the th WCObservers

This is the first half of the change, the second half will send the parent frame id with the webNavigation events

See RenderViewImpl::willSendRequest when it creates the RequestExtraData for a place where we already send the parent frame id (this is for the webRequest API that looks at network requests instead of navigation events)

BUG=128927
TEST=FrameNavigationState.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161570

[jochen (gone - plz use gerrit), 2012-10-11 13:12:07]

Marja: chrome/browser/extensions/api/web_navigation/*
John: content/ (except for content/browser/browser_plugin/)

the rest is just adding the new parameter to WebContentsObserver interface
implementations

[marja, 2012-10-11 13:25:58]

chrome/browser/extensions/api/web_navigation/* lgtm

[jam, 2012-10-11 19:35:36]

On 2012/10/11 13:12:07, jochen wrote:
> Marja: chrome/browser/extensions/api/web_navigation/*
> John: content/ (except for content/browser/browser_plugin/)
> 
> the rest is just adding the new parameter to WebContentsObserver interface
> implementations

seems that this reviewer instruction doesn't cover the whole change?

[jam, 2012-10-11 19:51:44]

my parts lgtm

[Bernhard Bauer, 2012-10-12 08:32:49]

A bunch of stupid questions below. That's what you get for making me review
stuff I don't understand ;-)

https://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(right):

https://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:106:
frame_state.parent_frame_num = -1;
What does |parent_frame_id| look like in this case? Shouldn't it already have a
-1 as |frame_num|?

https://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.h
(right):

https://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.h:126: int64
parent_frame_num;
It's a bit confusing to have "frame ID" mean both the plain frame ID (which this
identifier seems to be, or the |parent_frame_id| that we pass around) and
FrameID (which contains the former, but also has a RenderViewHost). Then, to add
to the confusion, the frame ID inside of a FrameID is called |frame_num|?

https://codereview.chromium.org/11090068/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/11090068/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_observer.h:55: int64 parent_frame_id,
Some tests appear to use -1 here. Could you add a comment what that value means?

[jochen (gone - plz use gerrit), 2012-10-12 10:44:47]

http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(right):

http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:106:
frame_state.parent_frame_num = -1;
On 2012/10/12 08:32:49, Bernhard Bauer wrote:
> What does |parent_frame_id| look like in this case? Shouldn't it already have
a
> -1 as |frame_num|?
it should, but I want to be on the safe side and not store inconsistent data,
just because somebody screwed up the parent frame num somewhere.

http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.h
(right):

http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.h:126: int64
parent_frame_num;
On 2012/10/12 08:32:49, Bernhard Bauer wrote:
> It's a bit confusing to have "frame ID" mean both the plain frame ID (which
this
> identifier seems to be, or the |parent_frame_id| that we pass around) and
> FrameID (which contains the former, but also has a RenderViewHost). Then, to
add
> to the confusion, the frame ID inside of a FrameID is called |frame_num|?

a frame is identified by this in, and the RV it is in. A WebContents (which a
frameNavigationState object relates to) can have multiple RVH, which is why it
needs to track both the int and the RVH :-/

I could also put a FrameID parent_frame_id here, but this would require a
ctor/dtor for this struct.

http://codereview.chromium.org/11090068/diff/1/content/public/browser/web_con...
File content/public/browser/web_contents_observer.h (right):

http://codereview.chromium.org/11090068/diff/1/content/public/browser/web_con...
content/public/browser/web_contents_observer.h:55: int64 parent_frame_id,
On 2012/10/12 08:32:49, Bernhard Bauer wrote:
> Some tests appear to use -1 here. Could you add a comment what that value
means?

Done.

[Bernhard Bauer, 2012-10-12 11:22:45]

LGTM w/ one comment:

http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
(right):

http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:106:
frame_state.parent_frame_num = -1;
On 2012/10/12 10:44:48, jochen wrote:
> On 2012/10/12 08:32:49, Bernhard Bauer wrote:
> > What does |parent_frame_id| look like in this case? Shouldn't it already
have
> a
> > -1 as |frame_num|?
> it should, but I want to be on the safe side and not store inconsistent data,
> just because somebody screwed up the parent frame num somewhere.

Right. Could you DCHECK that then?

[jochen (gone - plz use gerrit), 2012-10-12 11:40:06]

On 2012/10/12 11:22:45, Bernhard Bauer wrote:
> LGTM w/ one comment:
> 
>
http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
> File chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
> (right):
> 
>
http://codereview.chromium.org/11090068/diff/1/chrome/browser/extensions/api/...
> chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc:106:
> frame_state.parent_frame_num = -1;
> On 2012/10/12 10:44:48, jochen wrote:
> > On 2012/10/12 08:32:49, Bernhard Bauer wrote:
> > > What does |parent_frame_id| look like in this case? Shouldn't it already
> have
> > a
> > > -1 as |frame_num|?
> > it should, but I want to be on the safe side and not store inconsistent
data,
> > just because somebody screwed up the parent frame num somewhere.
> 
> Right. Could you DCHECK that then?

done

[commit-bot: I haz the power, 2012-10-12 11:40:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/11090068/8002

[commit-bot: I haz the power, 2012-10-12 13:35:15]

Change committed as 161570