Implemented WebStateObserver::DidFinishNavigation(NavigationHandle*).

ios/web/public/web_state/navigation_handle.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_WEB_PUBLIC_WEB_STATE_NAVIGATION_HANDLE_H_
+#define IOS_WEB_PUBLIC_WEB_STATE_NAVIGATION_HANDLE_H_
+
+#include "net/base/net_errors.h"

[kkhorimoto, 2017/02/22 01:42:37]

It doesn't look like this is needed here.

[Eugene But (OOO till 7-30), 2017/02/22 17:54:32]

Done.

+
+class GURL;
+
+namespace web {
+
+class WebState;
+
+// Tracks information related to a single navigation. NavigationHandle is

[marq (ping after 24h), 2017/02/22 13:26:36]

nit: 'A NavigationHandle'.

[Eugene But (OOO till 7-30), 2017/02/22 17:54:32]

Done.

+// provided to WebStateObserver methods to allow observers to track specific
+// navigations and their details. Observers should clear any references to a
+// NavigationHandle at the time of WebStateObserver::DidFinishNavigation, just
+// before the handle is destroyed.
+class NavigationHandle {

[marq (ping after 24h), 2017/02/22 13:26:36]

'Handle' is usually used to mean a pointer to a pointer, or for a data structure
that wraps a pointer for ownership purposes. We've used in in Chrome for the C++
object that wraps and owns an ObjC object that gets stored as user data in a
browserState, for example. 

'Context' is overused, but this actually seems like a good use for it -- so can
this change to web::NavigationContext?

[Eugene But (OOO till 7-30), 2017/02/22 17:54:32]

NavigationHandle was content term. But I agree that it is better to rename this
class for a few reasons:
1.) Context sounds better
2.) web should use different names for analogues of content classes.

Done.

+ public:
+  // The WebState the navigation is taking place in.
+  virtual WebState* GetWebState() = 0;

[kkhorimoto, 2017/02/22 01:42:37]

This is only used by a WSO callback right?  If so, the caller should already
have a reference to the WebState object, so I don't think we need to include it
here.  I don't think we have any situations where an object is observing
multiple WebStates, or else we would need to include this information in the
other callbacks as well.

[Eugene But (OOO till 7-30), 2017/02/22 17:54:32]

This is modeled from content, which has NavigationHandle::GetWebState but does
not pass WebState to DidFinishNavigation, so I don't think ios/web should be
different. Also Chrome for iOS allows observing multiple WebStates via
GlobalWebStateObserver, so including WebState in handle is useful. WDYT?

[kkhorimoto, 2017/02/22 18:38:05]

It looks like GlobalWebStateObserver includes analogs to WebStateObserver
callbacks, but with an additional WebState parameter.  Why not mimic that
pattern here?  I think it makes more sense for content// to have a
GetWebContents() function in their NavigationHandle, since they have renderer
access and a presumably sending these navigation notifications for each
frame/iframe.

[Eugene But (OOO till 7-30), 2017/02/22 21:44:56]

I don't think there is a difference here between web and content. Both
WebStateObserver and WebContentsObserver have access to WebState/WebContent.

Conceptually it makes sense to tell which WebContext/WebState has caused this
navigation. NavigationHandle/NavigationContext will be passed to various
callbacks and embedder can hold on NavigationHandle/NavigationContext pointer
for ongoing navigation. And if someone is holding on
NavigationHandle/NavigationContext it is important to know which
WebState/WebContents caused the navigation.

Why do you you think it's bad to have GetWebState() and why do you think ios
should be different from content? It's true that GetWebState is currently
unused, but it's just feels as required part of NavigationHandle.

[kkhorimoto, 2017/02/22 21:55:44]

Hmm, yeah you have a good point.  My main reason for this comment was that none
of out other WSO callbacks were passing a WebState along in the parameters. 
However, I hadn't thought of the fact that since we're passing an actual object
here that might be used by objects that aren't the actual observer, it would be
useful to bundle a WebState accessor in with this object.  Using the same
reasoning, do you think it'd make sense to also add it to LoadCommittedDetails
as well?

[Eugene But (OOO till 7-30), 2017/02/22 22:08:32]

LoadCommittedDetails is always passed as a reference and clients are not suppose
to retain or hold a pointer to that object. So think this is why
LoadCommittedDetails does not have a pointer to WebState. WDYT?

+
+  // The URL the frame is navigating to.

[marq (ping after 24h), 2017/02/22 13:26:36]

Not clear what 'frame' means in this context.

[Eugene But (OOO till 7-30), 2017/02/22 17:54:32]

Changed to "The URL the WebState is navigating to."

+  virtual const GURL& GetUrl() const = 0;

[kkhorimoto, 2017/02/22 01:42:37]

Other public web// interfaces use "URL"  (which, admittedly, is against the C++
style guide).  Do you want to continue using it here for consistency?  Maybe we
can do a batch update at some point to fix all the callers to use "Url" instead.

[Eugene But (OOO till 7-30), 2017/02/22 17:54:33]

Well, there is no real consistency, and URL is probably more prevailing that
Url, but Url is not rare. I think it is important to have a local consistency
within a file (which I have here), but I don't like violating Style Guide, just
because there are already multiple violation in other files. Doing batch update
to fix URL capitalization would be nice however :)

+
+  // Whether the navigation happened in the same page. Examples of same page
+  // navigations are:
+  // * reference fragment navigations
+  // * pushState/replaceState
+  // * same page history navigation
+  virtual bool IsSamePage() const = 0;

[kkhorimoto, 2017/02/22 01:42:37]

s/Page/Document

[Eugene But (OOO till 7-30), 2017/02/22 17:54:32]

This is modeled from content and I would like to use the same terminology as the
rest of Chrome. But then we have to fix inconsistency in ios naming and I think
we should change SameDocument to SamePage. If you agree I will file a bug and
s/SameDocument/SamePage in a separate CL.

[kkhorimoto, 2017/02/22 18:38:05]

While I agree that cross-platform consistency is important, I think we should
push back when their naming paradigms are ambiguous.  Two pages having the same
document is a clearly defined concept and is described in HTML specifications,
and is a more accurate description of what this bool represents.  Referencing
"documents" clearly indicates that if this bool is false, a new document object
is loaded (and vice versa).  I don't think that just because iOS is newer to the
open source community that we should be stuck with poor choices made by other
platforms.  I'm okay if you want to keep it as "same page" for this CL, since I
imagine that there are many use cases of the content// equivalent of this class
that we'd need to update, but I think we should look into changing this for all
platforms (or at the very least creating a bug so that we can have further
discussions).

[Eugene But (OOO till 7-30), 2017/02/22 21:44:56]

I started discussion with folks who wrote the original method and file a bug to
make things consistent (crbug.com/695189). If content owners are fine with
IsSameDocument name then I will rename content code. If they will have good
reasons for keeping their name, well... then we'll have to decide if we should
keep IsSamePage name or diverge.

WebKit code uses SameDocument term, so I do agree with your proposal, but I want
to have consistent code if possible:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr...

[kkhorimoto, 2017/02/22 21:55:44]

Great, thanks for that!

+
+  // Whether the navigation resulted in an error page.
+  virtual bool IsErrorPage() const = 0;
+
+  virtual ~NavigationHandle() {}
+};
+
+}  // namespace web
+
+#endif  // IOS_WEB_PUBLIC_WEB_STATE_NAVIGATION_HANDLE_H_