Add the onBeforeNavigate and onErrorOccured events to the webNavigation API.

Add the onBeforeNavigate and onErrorOccured events to the webNavigation API.

Also, rewrite the onCommitted event such that AUTO_SUBFRAME events are also registered.

The frameId and the requestId fields are still unimplemented.

Also, there occur navigation events after an error is reported.

BUG=50943
TEST=WebNavigationEvents

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

[jochen (gone - plz use gerrit), 2010-09-14 14:20:22]

please review.

[yzshen, 2010-09-14 17:50:29]

http://codereview.chromium.org/3436002/diff/3001/2002
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/3436002/diff/3001/2002#newcode80
chrome/browser/extensions/extension_webnavigation_api.cc:80:
dict->SetReal(keys::kTimeStampKey, base::Time::Now().ToDoubleT());
We need to convert time to millisenconds. (There are similar problems below.)

http://codereview.chromium.org/3436002/diff/3001/2006
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/3436002/diff/3001/2006#newcode2455
chrome/browser/tab_contents/tab_contents.cc:2455: // different from the
NAV_ENTRY_COMMITTED notificatino which doesn't include
notificatino -> notification

http://codereview.chromium.org/3436002/diff/3001/2008
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3436002/diff/3001/2008#newcode21
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:21:
console.log('---onBeforeNavigate: ' + details.url);
Wrong indent?

According to our JS style guide:
"When declaring an anonymous function in the list of arguments for a function
call, the body of the function is indented two spaces from the left edge of the
function call or the statement, not two spaces from the left edge of the
function keyword."

(There are similar problems below. at line 44-51)

[jochen (gone - plz use gerrit), 2010-09-14 18:38:42]

http://codereview.chromium.org/3436002/diff/3001/2002
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/3436002/diff/3001/2002#newcode80
chrome/browser/extensions/extension_webnavigation_api.cc:80:
dict->SetReal(keys::kTimeStampKey, base::Time::Now().ToDoubleT());
On 2010/09/14 17:50:29, yzshen wrote:
> We need to convert time to millisenconds. (There are similar problems below.)

This is the format used by other extension APIs for time (at least the cookie
api uses this)

[yzshen, 2010-09-14 19:29:04]

http://codereview.chromium.org/3436002/diff/3001/2002
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/3436002/diff/3001/2002#newcode80
chrome/browser/extensions/extension_webnavigation_api.cc:80:
dict->SetReal(keys::kTimeStampKey, base::Time::Now().ToDoubleT());
On 2010/09/14 18:38:43, jochen wrote:
> On 2010/09/14 17:50:29, yzshen wrote:
> > We need to convert time to millisenconds. (There are similar problems
below.)
> 
> This is the format used by other extension APIs for time (at least the cookie
> api uses this)

In extension_api.json, we specified that the timestamp is in milliseconds.
However, ToDoubleT() returns seconds.

[Matt Perry, 2010-09-14 20:16:32]

most stuff LG, but I disagree that AUTO_SUBFRAME events should be considered
"navigations." Maybe Yuzhu has an opinion here.

http://codereview.chromium.org/3436002/diff/3001/2006
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/3436002/diff/3001/2006#newcode2456
chrome/browser/tab_contents/tab_contents.cc:2456: // the actual URL navigation
to and isn't sent for AUTO_SUBFRAME navigations.
navigated*

http://codereview.chromium.org/3436002/diff/3001/2006#newcode2456
chrome/browser/tab_contents/tab_contents.cc:2456: // the actual URL navigation
to and isn't sent for AUTO_SUBFRAME navigations.
Looking at the code, it seems that NAV_ENTRY_COMMITTED is sent for
AUTO_SUBFRAME, unless nothing has changed. It sounds like we actually *don't*
want to send out the onCommitted event when nothing has changed (for example,
case #1 in navigation_types.h comment on AUTO_SUBFRAME). Are you sure you want
to send the event?

Also see my comment in the testing code.

http://codereview.chromium.org/3436002/diff/3001/2008
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3436002/diff/3001/2008#newcode21
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:21:
console.log('---onBeforeNavigate: ' + details.url);
On 2010/09/14 17:50:29, yzshen wrote:
> Wrong indent?
> 
> According to our JS style guide:
> "When declaring an anonymous function in the list of arguments for a function
> call, the body of the function is indented two spaces from the left edge of
the
> function call or the statement, not two spaces from the left edge of the
> function keyword."
> 
> (There are similar problems below. at line 44-51)

The reasoning doesn't seem to apply when the argument list is wrapped. We have
other instances of this in the extension testing. I think it would be better to
stick with this style to be consistent with our other code.

http://codereview.chromium.org/3436002/diff/3001/2008#newcode180
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:180:
url: getURL('iframe/b.html') }],
I don't think we should include the b.html loads here. They are not really
navigations, since they don't show up in the back-forward list.

[jochen (gone - plz use gerrit), 2010-09-15 14:37:16]

On 2010/09/14 20:16:32, Matt Perry wrote:
> most stuff LG, but I disagree that AUTO_SUBFRAME events should be considered
> "navigations." Maybe Yuzhu has an opinion here.

I talked to him before changing this, and he thinks the subframe events are
valuable (that's also the way the design doc specifies this). I personally don't
have a strong opinion about it, although I can imagine that it's useful to know
about all subframes, so you're not surprised if you later get a navigation event
for a subframe.

[yzshen, 2010-09-15 16:24:38]

http://codereview.chromium.org/3436002/diff/3001/2008
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3436002/diff/3001/2008#newcode21
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:21:
console.log('---onBeforeNavigate: ' + details.url);
On 2010/09/14 20:16:32, Matt Perry wrote:
> On 2010/09/14 17:50:29, yzshen wrote:
> > Wrong indent?
> > 
> > According to our JS style guide:
> > "When declaring an anonymous function in the list of arguments for a
function
> > call, the body of the function is indented two spaces from the left edge of
> the
> > function call or the statement, not two spaces from the left edge of the
> > function keyword."
> > 
> > (There are similar problems below. at line 44-51)
> 
> The reasoning doesn't seem to apply when the argument list is wrapped. We have
> other instances of this in the extension testing. I think it would be better
to
> stick with this style to be consistent with our other code.

I agree. :)
However, it seems there are still minor issues to fix:
line 27-28 & line 50: inconsistent indent.

http://codereview.chromium.org/3436002/diff/3001/2008#newcode180
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:180:
url: getURL('iframe/b.html') }],
On 2010/09/14 20:16:32, Matt Perry wrote:
> I don't think we should include the b.html loads here. They are not really
> navigations, since they don't show up in the back-forward list.

My personal opinion is that we should have webNavigation events for
AUTO_SUBFRAMEs. webNavigation is designed to send events about page loading in
frames; not about browsing history (back/forward list) changes.
(E.g., onBeforeRetarget and onDOMContentLoaded have nothing to do with
back/forward list. They are page loading events.)

As a result, I personally think all the frames should be treated in the same
way.

[Matt Perry, 2010-09-15 18:00:03]

LGTM

http://codereview.chromium.org/3436002/diff/3001/2008
File chrome/test/data/extensions/api_test/webnavigation/navigation/test.html
(right):

http://codereview.chromium.org/3436002/diff/3001/2008#newcode180
chrome/test/data/extensions/api_test/webnavigation/navigation/test.html:180:
url: getURL('iframe/b.html') }],
On 2010/09/15 16:24:39, yzshen wrote:
> On 2010/09/14 20:16:32, Matt Perry wrote:
> > I don't think we should include the b.html loads here. They are not really
> > navigations, since they don't show up in the back-forward list.
> 
> My personal opinion is that we should have webNavigation events for
> AUTO_SUBFRAMEs. webNavigation is designed to send events about page loading in
> frames; not about browsing history (back/forward list) changes.
> (E.g., onBeforeRetarget and onDOMContentLoaded have nothing to do with
> back/forward list. They are page loading events.)
> 
> As a result, I personally think all the frames should be treated in the same
> way. 
> 

OK, sounds like I just misunderstood the intent of the webNavigation API. Please
ignore me :).

[jochen (gone - plz use gerrit), 2010-09-16 12:41:42]

ok.

Yuzhu, I'll change the behavior of onErrorOccurred in the next CL

[yzshen, 2010-09-16 16:36:40]

LGTM

Thanks. :)

On Thu, Sep 16, 2010 at 5:41 AM,  <jochen@chromium.org> wrote:
> ok.
>
> Yuzhu, I'll change the behavior of onErrorOccurred in the next CL
>
> http://codereview.chromium.org/3436002/show
>



-- 
Best regards,
Yuzhu Shen.