Implement the frame id required for the web navigation api.

Implement the frame id required for the web navigation api.

BUG=50943
TEST=*.WebNavigationEvents

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

[jochen (gone - plz use gerrit), 2010-10-01 12:33:13]

please review.

[jochen (gone - plz use gerrit), 2010-10-01 12:33:47]

+darin for render view parts

[darin (slow to review), 2010-10-01 19:31:37]

http://codereview.chromium.org/3561008/diff/1/15
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3561008/diff/1/15#newcode2907
chrome/renderer/render_view.cc:2907: routing_id_, is_top_most,
ds->request().url(), frame->identifier()));
i think it would be cleaner to always pass the frame identifier first (or just
after the routing_id).

routing_id specifies the tab, frame_id specifies the frame within the tab,
is_top_most describes the frame, url is contained within the frame.  there is
somewhat of a progression there.

http://codereview.chromium.org/3561008/diff/1/15#newcode2946
chrome/renderer/render_view.cc:2946: show_repost_interstitial,
frame->identifier()));
ditto

[Matt Perry, 2010-10-01 19:51:46]

Yuzhu, could you take a look at my comment here:
http://codereview.chromium.org/3561008/diff/1/23#newcode324

I can't tell from the spec what the desired behavior is for failed navigations.

http://codereview.chromium.org/3561008/diff/1/2
File DEPS (right):

http://codereview.chromium.org/3561008/diff/1/2#newcode7
DEPS:7: "webkit_revision": "68871",
If part of this change was made in webkit, could you link the webkit bug ID in
the change description?

http://codereview.chromium.org/3561008/diff/1/3
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/3561008/diff/1/3#newcode28
chrome/browser/extensions/extension_webnavigation_api.cc:28: int
getFrameId(ProvisionalLoadDetails* details) {
style: GetFrameID

http://codereview.chromium.org/3561008/diff/1/4
File chrome/browser/extensions/extension_webnavigation_api.h (right):

http://codereview.chromium.org/3561008/diff/1/4#newcode76
chrome/browser/extensions/extension_webnavigation_api.h:76: NavigationStateMap
navigation_state_;
This doesn't seem to be used anywhere.

http://codereview.chromium.org/3561008/diff/1/6
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/3561008/diff/1/6#newcode550
chrome/browser/renderer_host/render_view_host.h:550: long long frame_id);
Can we use int64 (or maybe uint64) for the frame_id (here and everywhere it's
used).

http://codereview.chromium.org/3561008/diff/1/10
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/3561008/diff/1/10#newcode2498
chrome/browser/tab_contents/tab_contents.cc:2498: did_navigate ?
details.is_main_frame : false, details.is_in_page,
I don't follow this logic. Maybe add a comment?

http://codereview.chromium.org/3561008/diff/1/23
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3561008/diff/1/23#newcode1
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:1:
<script>
Excellent testing!

http://codereview.chromium.org/3561008/diff/1/23#newcode314
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:314: {
error: "net::ERR_FILE_NOT_FOUND",
Where do these error descriptions come from? Do we have a way of guaranteeing
they don't change even if the chrome internal code changes?

http://codereview.chromium.org/3561008/diff/1/23#newcode324
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:324:
url: "chrome://chromewebdata/"}],
Hmm.. this event is weird. The last 4 events all happen in c.html's subframe,
right? I think we should filter this one out, since it's more of an
implementation detail of chrome (especially the URL). So the sequence would be:
  onBeforeNavigate(c.html)
  onErrorOccurred(c.html)
  onCommitted(c.html)

I also wonder if we should filter out the "onCommitted" event for failed loads.
That an error occurred is enough information, right?

I'll ask Yuzhu to chime in on this.

[jochen (gone - plz use gerrit), 2010-10-01 22:04:36]

http://codereview.chromium.org/3561008/diff/1/2
File DEPS (right):

http://codereview.chromium.org/3561008/diff/1/2#newcode7
DEPS:7: "webkit_revision": "68871",
On 2010/10/01 19:51:46, Matt Perry wrote:
> If part of this change was made in webkit, could you link the webkit bug ID in
> the change description?

This is just so the trybots pull the correct revision.

http://codereview.chromium.org/3561008/diff/1/6
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/3561008/diff/1/6#newcode550
chrome/browser/renderer_host/render_view_host.h:550: long long frame_id);
On 2010/10/01 19:51:46, Matt Perry wrote:
> Can we use int64 (or maybe uint64) for the frame_id (here and everywhere it's
> used).
The WebKit API returns a long long, I thought it would be best to use the exact
same type

http://codereview.chromium.org/3561008/diff/1/23
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3561008/diff/1/23#newcode314
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:314: {
error: "net::ERR_FILE_NOT_FOUND",
On 2010/10/01 19:51:46, Matt Perry wrote:
> Where do these error descriptions come from? Do we have a way of guaranteeing
> they don't change even if the chrome internal code changes?
No, they're just the textual representation of the raw error code.

The API, however, does not specify what error messages are possible

http://codereview.chromium.org/3561008/diff/1/23#newcode324
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:324:
url: "chrome://chromewebdata/"}],
On 2010/10/01 19:51:46, Matt Perry wrote:
> Hmm.. this event is weird. The last 4 events all happen in c.html's subframe,
> right? I think we should filter this one out, since it's more of an
> implementation detail of chrome (especially the URL). So the sequence would
be:
>   onBeforeNavigate(c.html)
>   onErrorOccurred(c.html)
>   onCommitted(c.html)
> 
> I also wonder if we should filter out the "onCommitted" event for failed
loads.
> That an error occurred is enough information, right?
> 
> I'll ask Yuzhu to chime in on this.
Yes, this event will go away.

The API basically says that once an error occurs, no further events are send for
this frame. To implement this, however, I first need a way to track frames
(that's this change)

The next change will make those strange events go away

[Matt Perry, 2010-10-01 22:15:49]

LGTM with my initial comments addressed (GetFrameID, navigation_state_map,
comment on ProvisionalLoadDetails)

http://codereview.chromium.org/3561008/diff/1/23
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3561008/diff/1/23#newcode314
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:314: {
error: "net::ERR_FILE_NOT_FOUND",
On 2010/10/01 22:04:37, jochen wrote:
> On 2010/10/01 19:51:46, Matt Perry wrote:
> > Where do these error descriptions come from? Do we have a way of
guaranteeing
> > they don't change even if the chrome internal code changes?
> No, they're just the textual representation of the raw error code.
> 
> The API, however, does not specify what error messages are possible
> 

True, but people may still come to rely on the error text, regardless of what
the spec says.

http://codereview.chromium.org/3561008/diff/1/23#newcode324
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:324:
url: "chrome://chromewebdata/"}],
On 2010/10/01 22:04:37, jochen wrote:
> On 2010/10/01 19:51:46, Matt Perry wrote:
> > Hmm.. this event is weird. The last 4 events all happen in c.html's
subframe,
> > right? I think we should filter this one out, since it's more of an
> > implementation detail of chrome (especially the URL). So the sequence would
> be:
> >   onBeforeNavigate(c.html)
> >   onErrorOccurred(c.html)
> >   onCommitted(c.html)
> > 
> > I also wonder if we should filter out the "onCommitted" event for failed
> loads.
> > That an error occurred is enough information, right?
> > 
> > I'll ask Yuzhu to chime in on this.
> Yes, this event will go away.
> 
> The API basically says that once an error occurs, no further events are send
for
> this frame. To implement this, however, I first need a way to track frames
> (that's this change)
> 
> The next change will make those strange events go away
> 

Perfect, thanks for explaining.

[yzshen, 2010-10-01 22:36:15]

http://codereview.chromium.org/3561008/diff/1/3
File chrome/browser/extensions/extension_webnavigation_api.cc (left):

http://codereview.chromium.org/3561008/diff/1/3#oldcode82
chrome/browser/extensions/extension_webnavigation_api.cc:82: (base::Time::Now()
- base::Time::UnixEpoch()).InMilliseconds()));
Sorry I didn't catch this in previous reviews. But I think this is not
consistent with timestamp used in other APIs. For example, in
extension_history_api.cc, GetVisitInfoDictionary() sets timestamp using
value->SetReal(keys::kVisitTime, MilliSecondsFromTime(row.visit_time));

http://codereview.chromium.org/3561008/diff/1/3
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/3561008/diff/1/3#newcode26
chrome/browser/extensions/extension_webnavigation_api.cc:26: // Returns 0 if the
navigation is happen in the main frame, or the frame id
1) is happen -> happens/is happening
2) id -> ID

http://codereview.chromium.org/3561008/diff/1/3#newcode27
chrome/browser/extensions/extension_webnavigation_api.cc:27: // modulo 32 bits
otherwise (frame ids start with 0, so we add 1)
1) ids -> IDs
2) Please add a period at the end of the sentence.
3) I think frame IDs start with 1, according to your code in
trunk/WebKit/chromium/src/WebFrameImpl.cpp

http://codereview.chromium.org/3561008/diff/1/4
File chrome/browser/extensions/extension_webnavigation_api.h (right):

http://codereview.chromium.org/3561008/diff/1/4#newcode40
chrome/browser/extensions/extension_webnavigation_api.h:40: } NavigationState;
1) Please add comments for the navigation states.
2) I think there are other navigation states, right? For example, the frame is
not navigating, but without any error.

http://codereview.chromium.org/3561008/diff/1/4#newcode76
chrome/browser/extensions/extension_webnavigation_api.h:76: NavigationStateMap
navigation_state_;
1) If this map won't be used, please remove it;
2) Otherwise, there is a problem: webNavigation notifications are sent for
frames instead of tabs. Tracking the navigating state of tabs is not sufficient.

Consider the following case:
[ mainframe: navigating [subframe_1: error] [subframe_2: done]]

http://codereview.chromium.org/3561008/diff/1/14
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/3561008/diff/1/14#newcode208
chrome/common/render_messages_params.h:208: // The frame_id for this navigation.
The frame_id uniquely identifies the
frame_id -> frame ID ?

http://codereview.chromium.org/3561008/diff/1/23
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3561008/diff/1/23#newcode5
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:5: var
frameIds
Please add a ";".

http://codereview.chromium.org/3561008/diff/1/23#newcode314
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:314: {
error: "net::ERR_FILE_NOT_FOUND",
On 2010/10/01 22:04:37, jochen wrote:
> On 2010/10/01 19:51:46, Matt Perry wrote:
> > Where do these error descriptions come from? Do we have a way of
guaranteeing
> > they don't change even if the chrome internal code changes?
> No, they're just the textual representation of the raw error code.
> 
> The API, however, does not specify what error messages are possible
> 

Yes, currently the design doc doesn't specify error messages. We need to define
a set of errors.

My personal opinion is to find out the possible values of the error code passed
into didFailProvisionalLoad in our code, and convert them into proper strings.

http://codereview.chromium.org/3561008/diff/1/23#newcode324
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:324:
url: "chrome://chromewebdata/"}],
On 2010/10/01 22:04:37, jochen wrote:
> On 2010/10/01 19:51:46, Matt Perry wrote:
> > Hmm.. this event is weird. The last 4 events all happen in c.html's
subframe,
> > right? I think we should filter this one out, since it's more of an
> > implementation detail of chrome (especially the URL). So the sequence would
> be:
> >   onBeforeNavigate(c.html)
> >   onErrorOccurred(c.html)
> >   onCommitted(c.html)
> > 
> > I also wonder if we should filter out the "onCommitted" event for failed
> loads.
> > That an error occurred is enough information, right?
> > 
> > I'll ask Yuzhu to chime in on this.
> Yes, this event will go away.
> 
> The API basically says that once an error occurs, no further events are send
for
> this frame. To implement this, however, I first need a way to track frames
> (that's this change)
> 
> The next change will make those strange events go away
> 

I think the expected behavior should be "once an error occurs, no further events
are sent for _the_current_navigation_in_ this frame".

If later a new navigation is started in the same frame, notifications should be
sent.

[jochen (gone - plz use gerrit), 2010-10-05 11:59:14]

http://codereview.chromium.org/3561008/diff/1/3
File chrome/browser/extensions/extension_webnavigation_api.cc (left):

http://codereview.chromium.org/3561008/diff/1/3#oldcode82
chrome/browser/extensions/extension_webnavigation_api.cc:82: (base::Time::Now()
- base::Time::UnixEpoch()).InMilliseconds()));
On 2010/10/01 22:36:16, yzshen wrote:
> Sorry I didn't catch this in previous reviews. But I think this is not
> consistent with timestamp used in other APIs. For example, in
> extension_history_api.cc, GetVisitInfoDictionary() sets timestamp using
> value->SetReal(keys::kVisitTime, MilliSecondsFromTime(row.visit_time));
> 

Done.

http://codereview.chromium.org/3561008/diff/1/3
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/3561008/diff/1/3#newcode26
chrome/browser/extensions/extension_webnavigation_api.cc:26: // Returns 0 if the
navigation is happen in the main frame, or the frame id
On 2010/10/01 22:36:16, yzshen wrote:
> 1) is happen -> happens/is happening
> 2) id -> ID

Done.

http://codereview.chromium.org/3561008/diff/1/3#newcode27
chrome/browser/extensions/extension_webnavigation_api.cc:27: // modulo 32 bits
otherwise (frame ids start with 0, so we add 1)
On 2010/10/01 22:36:16, yzshen wrote:
> 1) ids -> IDs
> 2) Please add a period at the end of the sentence.
> 3) I think frame IDs start with 1, according to your code in
> trunk/WebKit/chromium/src/WebFrameImpl.cpp

Done.

http://codereview.chromium.org/3561008/diff/1/3#newcode28
chrome/browser/extensions/extension_webnavigation_api.cc:28: int
getFrameId(ProvisionalLoadDetails* details) {
On 2010/10/01 19:51:46, Matt Perry wrote:
> style: GetFrameID

Done.

http://codereview.chromium.org/3561008/diff/1/4
File chrome/browser/extensions/extension_webnavigation_api.h (right):

http://codereview.chromium.org/3561008/diff/1/4#newcode40
chrome/browser/extensions/extension_webnavigation_api.h:40: } NavigationState;
On 2010/10/01 22:36:16, yzshen wrote:
> 1) Please add comments for the navigation states.
> 2) I think there are other navigation states, right? For example, the frame is
> not navigating, but without any error.
Removed the this block

http://codereview.chromium.org/3561008/diff/1/4#newcode76
chrome/browser/extensions/extension_webnavigation_api.h:76: NavigationStateMap
navigation_state_;
On 2010/10/01 22:36:16, yzshen wrote:
> 1) If this map won't be used, please remove it;
> 2) Otherwise, there is a problem: webNavigation notifications are sent for
> frames instead of tabs. Tracking the navigating state of tabs is not
sufficient.
> 
> Consider the following case:
> [ mainframe: navigating [subframe_1: error] [subframe_2: done]]
> 
> 

removed this block

http://codereview.chromium.org/3561008/diff/1/10
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/3561008/diff/1/10#newcode2498
chrome/browser/tab_contents/tab_contents.cc:2498: did_navigate ?
details.is_main_frame : false, details.is_in_page,
On 2010/10/01 19:51:46, Matt Perry wrote:
> I don't follow this logic. Maybe add a comment?

Done.

http://codereview.chromium.org/3561008/diff/1/14
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/3561008/diff/1/14#newcode208
chrome/common/render_messages_params.h:208: // The frame_id for this navigation.
The frame_id uniquely identifies the
On 2010/10/01 22:36:16, yzshen wrote:
> frame_id -> frame ID ?

Done.

http://codereview.chromium.org/3561008/diff/1/15
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3561008/diff/1/15#newcode2907
chrome/renderer/render_view.cc:2907: routing_id_, is_top_most,
ds->request().url(), frame->identifier()));
On 2010/10/01 19:31:37, darin wrote:
> i think it would be cleaner to always pass the frame identifier first (or just
> after the routing_id).
> 
> routing_id specifies the tab, frame_id specifies the frame within the tab,
> is_top_most describes the frame, url is contained within the frame.  there is
> somewhat of a progression there.

Done.

http://codereview.chromium.org/3561008/diff/1/15#newcode2946
chrome/renderer/render_view.cc:2946: show_repost_interstitial,
frame->identifier()));
On 2010/10/01 19:31:37, darin wrote:
> ditto

Done.

http://codereview.chromium.org/3561008/diff/1/23
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3561008/diff/1/23#newcode5
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:5: var
frameIds
On 2010/10/01 22:36:16, yzshen wrote:
> Please add a ";".

Done.

[yzshen, 2010-10-05 17:04:29]

On 2010/10/05 11:59:14, jochen wrote:
> http://codereview.chromium.org/3561008/diff/1/3
> File chrome/browser/extensions/extension_webnavigation_api.cc (left):
> 
> http://codereview.chromium.org/3561008/diff/1/3#oldcode82
> chrome/browser/extensions/extension_webnavigation_api.cc:82:
(base::Time::Now()
> - base::Time::UnixEpoch()).InMilliseconds()));
> On 2010/10/01 22:36:16, yzshen wrote:
> > Sorry I didn't catch this in previous reviews. But I think this is not
> > consistent with timestamp used in other APIs. For example, in
> > extension_history_api.cc, GetVisitInfoDictionary() sets timestamp using
> > value->SetReal(keys::kVisitTime, MilliSecondsFromTime(row.visit_time));
> > 
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/3
> File chrome/browser/extensions/extension_webnavigation_api.cc (right):
> 
> http://codereview.chromium.org/3561008/diff/1/3#newcode26
> chrome/browser/extensions/extension_webnavigation_api.cc:26: // Returns 0 if
the
> navigation is happen in the main frame, or the frame id
> On 2010/10/01 22:36:16, yzshen wrote:
> > 1) is happen -> happens/is happening
> > 2) id -> ID
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/3#newcode27
> chrome/browser/extensions/extension_webnavigation_api.cc:27: // modulo 32 bits
> otherwise (frame ids start with 0, so we add 1)
> On 2010/10/01 22:36:16, yzshen wrote:
> > 1) ids -> IDs
> > 2) Please add a period at the end of the sentence.
> > 3) I think frame IDs start with 1, according to your code in
> > trunk/WebKit/chromium/src/WebFrameImpl.cpp
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/3#newcode28
> chrome/browser/extensions/extension_webnavigation_api.cc:28: int
> getFrameId(ProvisionalLoadDetails* details) {
> On 2010/10/01 19:51:46, Matt Perry wrote:
> > style: GetFrameID
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/4
> File chrome/browser/extensions/extension_webnavigation_api.h (right):
> 
> http://codereview.chromium.org/3561008/diff/1/4#newcode40
> chrome/browser/extensions/extension_webnavigation_api.h:40: } NavigationState;
> On 2010/10/01 22:36:16, yzshen wrote:
> > 1) Please add comments for the navigation states.
> > 2) I think there are other navigation states, right? For example, the frame
is
> > not navigating, but without any error.
> Removed the this block
> 
> http://codereview.chromium.org/3561008/diff/1/4#newcode76
> chrome/browser/extensions/extension_webnavigation_api.h:76: NavigationStateMap
> navigation_state_;
> On 2010/10/01 22:36:16, yzshen wrote:
> > 1) If this map won't be used, please remove it;
> > 2) Otherwise, there is a problem: webNavigation notifications are sent for
> > frames instead of tabs. Tracking the navigating state of tabs is not
> sufficient.
> > 
> > Consider the following case:
> > [ mainframe: navigating [subframe_1: error] [subframe_2: done]]
> > 
> > 
> 
> removed this block
> 
> http://codereview.chromium.org/3561008/diff/1/10
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
> http://codereview.chromium.org/3561008/diff/1/10#newcode2498
> chrome/browser/tab_contents/tab_contents.cc:2498: did_navigate ?
> details.is_main_frame : false, details.is_in_page,
> On 2010/10/01 19:51:46, Matt Perry wrote:
> > I don't follow this logic. Maybe add a comment?
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/14
> File chrome/common/render_messages_params.h (right):
> 
> http://codereview.chromium.org/3561008/diff/1/14#newcode208
> chrome/common/render_messages_params.h:208: // The frame_id for this
navigation.
> The frame_id uniquely identifies the
> On 2010/10/01 22:36:16, yzshen wrote:
> > frame_id -> frame ID ?
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/15
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/3561008/diff/1/15#newcode2907
> chrome/renderer/render_view.cc:2907: routing_id_, is_top_most,
> ds->request().url(), frame->identifier()));
> On 2010/10/01 19:31:37, darin wrote:
> > i think it would be cleaner to always pass the frame identifier first (or
just
> > after the routing_id).
> > 
> > routing_id specifies the tab, frame_id specifies the frame within the tab,
> > is_top_most describes the frame, url is contained within the frame.  there
is
> > somewhat of a progression there.
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/15#newcode2946
> chrome/renderer/render_view.cc:2946: show_repost_interstitial,
> frame->identifier()));
> On 2010/10/01 19:31:37, darin wrote:
> > ditto
> 
> Done.
> 
> http://codereview.chromium.org/3561008/diff/1/23
> File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
> (right):
> 
> http://codereview.chromium.org/3561008/diff/1/23#newcode5
> chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:5: var
> frameIds
> On 2010/10/01 22:36:16, yzshen wrote:
> > Please add a ";".
> 
> Done.

LGTM

Thanks, Jochen!