Track in which frames navigation errors occurred and don't send further navigation events for them

Track in which frames navigation errors occurred and don't send further navigation events for them

BUG=50943
TEST=browser_tests:*.WebNavigationEvents*,unit_tests:FrameNavigationStateTest.*

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

[jochen (gone - plz use gerrit), 2010-10-27 09:54:39]

please review

[Paweł Hajdan Jr., 2010-10-27 10:03:02]

Drive-by with minor test comments. No need to wait for another review by me.

http://codereview.chromium.org/4136004/diff/1/4
File chrome/browser/extensions/extension_webnavigation_unittest.cc (right):

http://codereview.chromium.org/4136004/diff/1/4#newcode25
chrome/browser/extensions/extension_webnavigation_unittest.cc:25: 
nit: Remove empty line.

http://codereview.chromium.org/4136004/diff/1/4#newcode33
chrome/browser/extensions/extension_webnavigation_unittest.cc:33:
TestTabContents* tab_contents =
nit: How about a scoped_ptr to make sure this won't leak?

http://codereview.chromium.org/4136004/diff/1/4#newcode51
chrome/browser/extensions/extension_webnavigation_unittest.cc:51:
TestTabContents* tab_contents =
nit: Same here.

[yzshen, 2010-10-27 17:43:38]

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

http://codereview.chromium.org/4136004/diff/1/2#newcode86
chrome/browser/extensions/extension_webnavigation_api.cc:86: frame_id++) {
[optional] Better to use ++frame_id.

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

http://codereview.chromium.org/4136004/diff/1/3#newcode27
chrome/browser/extensions/extension_webnavigation_api.h:27: class
FrameNavigationState : public NotificationObserver {
1) The design says: For a specific *navigation*, there is no further events
fired after onErrorOccurred.
That doesn't mean that onErrorOccurred is guaranteed to be the last event from a
frame. Instead, it means that onErrorOccurred must be followed by an
onBeforeNavigate (the first event for a new navigation).
Does that make sense?
I think the implementation probably doesn't work for the following scenario:
onErrorOccurred in a subframe; a new navigation starts in that subframe.

2) Although this class is registered as a notification observer to listen to
TAB_CONTENTS_DESTROYED, it still relies on FRAME_PROVISIONAL_LOAD_START (via
TrackFrame).

To me, it seems better to keep FrameNavigationState simple (not register it as a
NotificationObserver), and let ExtensionWebNavigationEventRouter do the job of
listening to notifications.

http://codereview.chromium.org/4136004/diff/1/3#newcode30
chrome/browser/extensions/extension_webnavigation_api.h:30:
~FrameNavigationState();
Please consider to add "virtual" for the destructor. (Although it works fine
without "virtual", I think it is more clear to have it.)

http://codereview.chromium.org/4136004/diff/1/3#newcode35
chrome/browser/extensions/extension_webnavigation_api.h:35: // Starts the track
a frame given by its |frame_id| showing the URL |url| in
Do you mean "Starts to track"?

http://codereview.chromium.org/4136004/diff/1/3#newcode54
chrome/browser/extensions/extension_webnavigation_api.h:54: // Remove state
associated with this tab contents and all of its frames.
Remove -> Removes

http://codereview.chromium.org/4136004/diff/1/4
File chrome/browser/extensions/extension_webnavigation_unittest.cc (right):

http://codereview.chromium.org/4136004/diff/1/4#newcode66
chrome/browser/extensions/extension_webnavigation_unittest.cc:66:
navigation_state.TrackFrame(frame_id, url2, true, tab_contents);
What if the error happens in a subframe, and then another navigation happens in
that subframe?

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

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

http://codereview.chromium.org/4136004/diff/1/3#newcode27
chrome/browser/extensions/extension_webnavigation_api.h:27: class
FrameNavigationState : public NotificationObserver {
On 2010/10/27 17:43:38, yzshen wrote:
> 1) The design says: For a specific *navigation*, there is no further events
> fired after onErrorOccurred.
> That doesn't mean that onErrorOccurred is guaranteed to be the last event from
a
> frame. Instead, it means that onErrorOccurred must be followed by an
> onBeforeNavigate (the first event for a new navigation).
> Does that make sense?
> I think the implementation probably doesn't work for the following scenario:
> onErrorOccurred in a subframe; a new navigation starts in that subframe.

I think it works for that case :)

I'll add a unit test to verify

> 
> 2) Although this class is registered as a notification observer to listen to
> TAB_CONTENTS_DESTROYED, it still relies on FRAME_PROVISIONAL_LOAD_START (via
> TrackFrame).
> 
> To me, it seems better to keep FrameNavigationState simple (not register it as
a
> NotificationObserver), and let ExtensionWebNavigationEventRouter do the job of
> listening to notifications.

sure, why not

http://codereview.chromium.org/4136004/diff/1/3#newcode30
chrome/browser/extensions/extension_webnavigation_api.h:30:
~FrameNavigationState();
On 2010/10/27 17:43:38, yzshen wrote:
> Please consider to add "virtual" for the destructor. (Although it works fine
> without "virtual", I think it is more clear to have it.)

why? It'll just make the destructor call more expensive

[yzshen, 2010-10-27 18:52:28]

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

http://codereview.chromium.org/4136004/diff/1/3#newcode30
chrome/browser/extensions/extension_webnavigation_api.h:30:
~FrameNavigationState();
On 2010/10/27 18:01:14, jochen wrote:
> On 2010/10/27 17:43:38, yzshen wrote:
> > Please consider to add "virtual" for the destructor. (Although it works fine
> > without "virtual", I think it is more clear to have it.)
> 
> why? It'll just make the destructor call more expensive
> 

Since ~NotificationObserver is declared as "virtual",
~FrameNavigationState is already virtual, no matter you add the "virtual"
keyword or not.

Moreover, it is required by the C++ style guide:
Make your destructor virtual if necessary. If your class has virtual methods,
its destructor should be virtual.
...
When redefining an inherited virtual function, explicitly declare it virtual in
the declaration of the derived class. Rationale: If virtual is omitted, the
reader has to check all ancestors of the class in question to determine if the
function is virtual or not.

[Matt Perry, 2010-10-27 19:23:42]

LGTM

http://codereview.chromium.org/4136004/diff/1/6
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/4136004/diff/1/6#newcode141
chrome/common/url_constants.cc:141: const char kChromeUnreachableWebDataURL[] =
"chrome://chromewebdata/";
nit: the "Chrome" part of this name seems redundant, since it's in the chrome::
namespace. The constants below don't include "Chrome" either.

[jochen (gone - plz use gerrit), 2010-10-27 20:08:25]

On 2010/10/27 18:52:28, yzshen wrote:
> http://codereview.chromium.org/4136004/diff/1/3
> File chrome/browser/extensions/extension_webnavigation_api.h (right):
> 
> http://codereview.chromium.org/4136004/diff/1/3#newcode30
> chrome/browser/extensions/extension_webnavigation_api.h:30:
> ~FrameNavigationState();
> On 2010/10/27 18:01:14, jochen wrote:
> > On 2010/10/27 17:43:38, yzshen wrote:
> > > Please consider to add "virtual" for the destructor. (Although it works
fine
> > > without "virtual", I think it is more clear to have it.)
> > 
> > why? It'll just make the destructor call more expensive
> > 
> 
> Since ~NotificationObserver is declared as "virtual",
> ~FrameNavigationState is already virtual, no matter you add the "virtual"
> keyword or not.
> 
> Moreover, it is required by the C++ style guide:
> Make your destructor virtual if necessary. If your class has virtual methods,
> its destructor should be virtual.
> ...
> When redefining an inherited virtual function, explicitly declare it virtual
in
> the declaration of the derived class. Rationale: If virtual is omitted, the
> reader has to check all ancestors of the class in question to determine if the
> function is virtual or not.

you're right, I missed the fact that the base class is virtual, so it's not
something to consider but a bug not to make it virtual :)