Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698413004:
Implemented WebStateObserver::DidFinishNavigation(NavigationHandle*). (Closed)
|
Created:
3 years, 10 months ago by Eugene But (OOO till 7-30) Modified:
3 years, 10 months ago CC:
chromium-reviews, Eugene But (OOO till 7-30) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented WebStateObserver::DidFinishNavigation(NavigationContext*).
This callback aims to replace the following methods:
WebStateObserver::UrlHashChanged()
WebStateObserver::HistoryStateChanged()
-[CRWWebDelegate webDidUpdateHistoryStateWithPageURL:]
-[CRWWebDelegate webWillFinishHistoryNavigationFromEntry:]
-[CRWWebDelegate webDidStartLoadingURL:shouldUpdateHistory:]
These callbacks will be replaced in a separate CL. Currently NavigationContext is
created only for DidFinishNavigation callback. Long term plan is to create it
for DidStartNavigation and keep it alive until DidFinishNavigation.
This CL has small logic change by updating MIME type for Native Content
navigation, which is the right thing to do anyway.
BUG=692331
Review-Url: https://codereview.chromium.org/2698413004
Cr-Commit-Position: refs/heads/master@{#452732}
Committed: https://chromium.googlesource.com/chromium/src/+/17faf252763df7fb3dbe510549ed5fd8e94e84db
Patch Set 1 #Patch Set 2 : Added integration tests #Patch Set 3 : Self review #
Total comments: 49
Patch Set 4 : Addressed review comments #Patch Set 5 : Self review #Patch Set 6 : Made NavigationContext constructor private #
Total comments: 10
Patch Set 7 : Addressed review comments #Messages
Total messages: 34 (19 generated)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
eugenebut@chromium.org changed reviewers: + kkhorimoto@chromium.org, marq@chromium.org, rohitrao@chromium.org
PTAL. This is new navigation callback that matches other platforms and intended to replace 5 existing callbacks. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/test/fak... File ios/web/public/test/fakes/test_web_client.mm (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/test/fak... ios/web/public/test/fakes/test_web_client.mm:25: url::SchemeWithType native_scheme = {kTestNativeContentScheme, Used to test NativeContent https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... File ios/web/web_state/navigation_callbacks_inttest.mm (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_callbacks_inttest.mm:158: I could not write tests for error page navigation, because they need real networking: https://cs.chromium.org/chromium/src/ios/web/public/test/response_providers/e...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/navigation_handle.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:8: #include "net/base/net_errors.h" It doesn't look like this is needed here. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:24: virtual WebState* GetWebState() = 0; 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. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:27: virtual const GURL& GetUrl() const = 0; 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. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:34: virtual bool IsSamePage() const = 0; s/Page/Document https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/web_state_observer.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:55: // NavigationHandle::IsErrorPage. IsErrorPage() https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:58: // will still be ongoing in the WebState returned by |navigation_handle|. This comment should be updated if we remove NavigationHandle::GetWebState(). https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:63: // Note that this is fired by same-page navigations, such as fragment NIT: s/Note that this is fired/This is also fired/ "Note that" has a conversational, imperative tone similarly to when we use "We" in comments. I think it's better to have comments that are simply declarative statements about functionality. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:65: // change. To filter these out, use NavigationHandle::IsSamePage. IsSameDocument() https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:67: // Note that |navigation_handle| will be destroyed at the end of this call, Same comment about "Note that" https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:69: virtual void DidFinishNavigation(NavigationHandle* navigation_handle) {} Should we use a "const NavigationHandle&" here? https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... File ios/web/web_state/navigation_callbacks_inttest.mm (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_callbacks_inttest.mm:25: ACTION_P2(VerifyNewPageHandle, web_state, url) { s/Page/Document https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_callbacks_inttest.mm:36: ACTION_P2(VerifySamePageHandle, web_state, url) { s/Page/Document https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... File ios/web/web_state/navigation_handle_impl.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_handle_impl.h:37: bool is_error_page); Do we want to enforce the use of the static factory methods by moving the constructor to private? https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4992: if (!isSameDocumentNavigaiton) { Let's reverse the ordering of these conditions so we're not checking the negative of the bool. https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.h:92: void OnSamePageNavigation(const GURL& url); s/Page/Document
I didn't have time for a close review, and likely won't this week. I'm in agreement with the general philosophy of the change, and I took a look at the new API and made a few comments. Please ping me if there are specific files you need an OWNER review on. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/navigation_handle.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:16: // Tracks information related to a single navigation. NavigationHandle is nit: 'A NavigationHandle'. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:21: class NavigationHandle { '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? https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:26: // The URL the frame is navigating to. Not clear what 'frame' means in this context.
Mark, I'm fine with your high-level review. PTAL. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/navigation_handle.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:8: #include "net/base/net_errors.h" On 2017/02/22 01:42:37, kkhorimoto_ wrote: > It doesn't look like this is needed here. Done. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:16: // Tracks information related to a single navigation. NavigationHandle is On 2017/02/22 13:26:36, marq wrote: > nit: 'A NavigationHandle'. Done. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:21: class NavigationHandle { On 2017/02/22 13:26:36, marq wrote: > '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? 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. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:24: virtual WebState* GetWebState() = 0; On 2017/02/22 01:42:37, kkhorimoto_ wrote: > 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. 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? https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:26: // The URL the frame is navigating to. On 2017/02/22 13:26:36, marq wrote: > Not clear what 'frame' means in this context. Changed to "The URL the WebState is navigating to." https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:27: virtual const GURL& GetUrl() const = 0; On 2017/02/22 01:42:37, kkhorimoto_ wrote: > 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. 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 :) https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:34: virtual bool IsSamePage() const = 0; On 2017/02/22 01:42:37, kkhorimoto_ wrote: > s/Page/Document 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. https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... File ios/web/web_state/navigation_callbacks_inttest.mm (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_callbacks_inttest.mm:25: ACTION_P2(VerifyNewPageHandle, web_state, url) { On 2017/02/22 01:42:38, kkhorimoto_ wrote: > s/Page/Document Acknowledged. https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_callbacks_inttest.mm:36: ACTION_P2(VerifySamePageHandle, web_state, url) { On 2017/02/22 01:42:38, kkhorimoto_ wrote: > s/Page/Document Acknowledged. https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... File ios/web/web_state/navigation_handle_impl.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_handle_impl.h:37: bool is_error_page); On 2017/02/22 01:42:38, kkhorimoto_ wrote: > Do we want to enforce the use of the static factory methods by moving the > constructor to private? I've actually tried that but base::MakeUnique needs to call that constructor. I did not find any Chrome headers with friend declaration of MakeUnique, so doubt that I can make constructor private and use MakeUnique at the same time. Do you have any ideas how to fix this without using operator new? https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4992: if (!isSameDocumentNavigaiton) { On 2017/02/22 01:42:38, kkhorimoto_ wrote: > Let's reverse the ordering of these conditions so we're not checking the > negative of the bool. Done. https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.h:92: void OnSamePageNavigation(const GURL& url); On 2017/02/22 01:42:38, kkhorimoto_ wrote: > s/Page/Document Acknowledged.
LGTM at a high level.
https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/navigation_handle.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:24: virtual WebState* GetWebState() = 0; On 2017/02/22 17:54:32, Eugene But wrote: > On 2017/02/22 01:42:37, kkhorimoto_ wrote: > > 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. > 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? 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. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:34: virtual bool IsSamePage() const = 0; On 2017/02/22 17:54:32, Eugene But wrote: > On 2017/02/22 01:42:37, kkhorimoto_ wrote: > > s/Page/Document > 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. 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). https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... File ios/web/web_state/navigation_handle_impl.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_handle_impl.h:37: bool is_error_page); On 2017/02/22 17:54:33, Eugene But wrote: > On 2017/02/22 01:42:38, kkhorimoto_ wrote: > > Do we want to enforce the use of the static factory methods by moving the > > constructor to private? > I've actually tried that but base::MakeUnique needs to call that constructor. I > did not find any Chrome headers with friend declaration of MakeUnique, so doubt > that I can make constructor private and use MakeUnique at the same time. Do you > have any ideas how to fix this without using operator new? I'm not sure. I haven't been using MakeUnique to create unique pointers, as it feels more natural to me to just use the std::unique_ptr() constructor that takes a pointer created with the "new" keyword. I did a quick search for C++ objects created using the "new" keyword and found over 5000 in our codebase, so it doesn't seem like there's much consensus about MakeUnique() clearly being the best practice. I'd say that if using "new" helps to refine an object's interface, as it does here, we should use it instead of MakeUnique.
Thanks! https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/navigation_handle.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:24: virtual WebState* GetWebState() = 0; > 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. 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. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:34: virtual bool IsSamePage() const = 0; On 2017/02/22 18:38:05, kkhorimoto_ wrote: > On 2017/02/22 17:54:32, Eugene But wrote: > > On 2017/02/22 01:42:37, kkhorimoto_ wrote: > > > s/Page/Document > > 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. > > 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). 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... https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... File ios/web/web_state/navigation_handle_impl.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/web_state/navig... ios/web/web_state/navigation_handle_impl.h:37: bool is_error_page); On 2017/02/22 18:38:05, kkhorimoto_ wrote: > On 2017/02/22 17:54:33, Eugene But wrote: > > On 2017/02/22 01:42:38, kkhorimoto_ wrote: > > > Do we want to enforce the use of the static factory methods by moving the > > > constructor to private? > > I've actually tried that but base::MakeUnique needs to call that constructor. > I > > did not find any Chrome headers with friend declaration of MakeUnique, so > doubt > > that I can make constructor private and use MakeUnique at the same time. Do > you > > have any ideas how to fix this without using operator new? > > I'm not sure. I haven't been using MakeUnique to create unique pointers, as it > feels more natural to me to just use the std::unique_ptr() constructor that > takes a pointer created with the "new" keyword. I did a quick search for C++ > objects created using the "new" keyword and found over 5000 in our codebase, so > it doesn't seem like there's much consensus about MakeUnique() clearly being the > best practice. I'd say that if using "new" helps to refine an object's > interface, as it does here, we should use it instead of MakeUnique. Done.
https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/navigation_handle.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:24: virtual WebState* GetWebState() = 0; On 2017/02/22 21:44:56, Eugene But wrote: > > 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. > 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. 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? https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:34: virtual bool IsSamePage() const = 0; On 2017/02/22 21:44:56, Eugene But wrote: > On 2017/02/22 18:38:05, kkhorimoto_ wrote: > > On 2017/02/22 17:54:32, Eugene But wrote: > > > On 2017/02/22 01:42:37, kkhorimoto_ wrote: > > > > s/Page/Document > > > 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. > > > > 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). > 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... Great, thanks for that! https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/web_state_observer.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:69: virtual void DidFinishNavigation(NavigationHandle* navigation_handle) {} On 2017/02/22 01:42:38, kkhorimoto_ wrote: > Should we use a "const NavigationHandle&" here? Can you address the comments in this file as well? They're mostly comment nits (and page/document stuff), but this would be an actual implementation change that we might want, since we're passing LoadCommittedDetails this way.
Sorry for web_state_observer.h :) https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/navigation_handle.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/navigation_handle.h:24: virtual WebState* GetWebState() = 0; On 2017/02/22 21:55:44, kkhorimoto_ wrote: > On 2017/02/22 21:44:56, Eugene But wrote: > > > 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. > > 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. > > 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? 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? https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... File ios/web/public/web_state/web_state_observer.h (right): https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:55: // NavigationHandle::IsErrorPage. On 2017/02/22 01:42:38, kkhorimoto_ wrote: > IsErrorPage() Done. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:58: // will still be ongoing in the WebState returned by |navigation_handle|. On 2017/02/22 01:42:38, kkhorimoto_ wrote: > This comment should be updated if we remove NavigationHandle::GetWebState(). Acknowledged. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:63: // Note that this is fired by same-page navigations, such as fragment On 2017/02/22 01:42:38, kkhorimoto_ wrote: > NIT: s/Note that this is fired/This is also fired/ > > "Note that" has a conversational, imperative tone similarly to when we use "We" > in comments. I think it's better to have comments that are simply declarative > statements about functionality. Done. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:65: // change. To filter these out, use NavigationHandle::IsSamePage. On 2017/02/22 01:42:38, kkhorimoto_ wrote: > IsSameDocument() Done. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:67: // Note that |navigation_handle| will be destroyed at the end of this call, On 2017/02/22 01:42:38, kkhorimoto_ wrote: > Same comment about "Note that" Done. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:69: virtual void DidFinishNavigation(NavigationHandle* navigation_handle) {} On 2017/02/22 21:55:44, kkhorimoto_ wrote: > On 2017/02/22 01:42:38, kkhorimoto_ wrote: > > Should we use a "const NavigationHandle&" here? > > Can you address the comments in this file as well? They're mostly comment nits > (and page/document stuff), but this would be an actual implementation change > that we might want, since we're passing LoadCommittedDetails this way. Sorry, I missed those. https://codereview.chromium.org/2698413004/diff/40001/ios/web/public/web_stat... ios/web/public/web_state/web_state_observer.h:69: virtual void DidFinishNavigation(NavigationHandle* navigation_handle) {} On 2017/02/22 01:42:38, kkhorimoto_ wrote: > Should we use a "const NavigationHandle&" here? Pointer is used here, so clients can hold a pointer to this object. Reference must always be valid, so it does not work that well.
lgtm after offline discussion!
Thanks! Rohit, PTAL
lgtm Couple high level questions, because I'm looking at just this one CL without a sense of the bigger picture: 1) This CL "aims to replace" those four callbacks, but it doesn't actually replace them, right? We're adding new WebStateObserver methods but we haven't yet deleted the old CRWWebDelegate methods, or removed their implementations? 2) I don't have a good sense for what a NavigationCallback is or how it is meant to be used. The comments mention that you need to delete them when you get a call to DidFinishNavigation(), but it doesn't look like they are used in any other methods. Will we eventually pass NavigationCallbacks to other observer methods? Will observers hold on to callback pointers and compare them across methods? What kinds of things will this be used for? https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/web_state_observer.h (right): https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/web_state_observer.h:55: // NavigationHandle::IsErrorPage. What is "NavigationHandle"? I don't see any other references to it in this file. Oh, is this the original name of NavigationContext? Then this comment just needs to be updated? https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/web_state_observer.h:58: // will still be ongoing in the WebState returned by |navigation_handle|. And here. https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/web_state_observer.h:65: // change. To filter these out, use NavigationHandle::IsSamePage. And here. https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/navi... File ios/web/web_state/navigation_context_impl_unittest.mm (right): https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/navi... ios/web/web_state/navigation_context_impl_unittest.mm:14: class PlatformTestNavigationContextImplTest : public PlatformTest { Could this fixture go into an anonymous namespace instead? Why is "PlatformTest" in the fixture name? https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:4902: BOOL isSameDocumentNavigaiton = Typo: navigation
Description was changed from ========== Implemented WebStateObserver::DidFinishNavigation(NavigationHandle*). This callback aims to replace the following methods: WebStateObserver::UrlHashChanged() WebStateObserver::HistoryStateChanged() -[CRWWebDelegate webDidUpdateHistoryStateWithPageURL:] -[CRWWebDelegate webWillFinishHistoryNavigationFromEntry:] -[CRWWebDelegate webDidStartLoadingURL:shouldUpdateHistory:] This CL has small logic change by updating MIME type for Native Content navigation, which is the right thing to do anyway. BUG=692331 ========== to ========== Implemented WebStateObserver::DidFinishNavigation(NavigationContext*). This callback aims to replace the following methods: WebStateObserver::UrlHashChanged() WebStateObserver::HistoryStateChanged() -[CRWWebDelegate webDidUpdateHistoryStateWithPageURL:] -[CRWWebDelegate webWillFinishHistoryNavigationFromEntry:] -[CRWWebDelegate webDidStartLoadingURL:shouldUpdateHistory:] These callbacks will be replaced in a separate CL. Currently NavigationContext is created only for DidFinishNavigation callback. Long term plan is to create it for DidStartNavigation and keep it alive until DidFinishNavigation. This CL has small logic change by updating MIME type for Native Content navigation, which is the right thing to do anyway. BUG=692331 ==========
Thanks. Answering your question: 1.) Correct. Those 5 methods will be replaced in subsequent CLs. 2.) NavigationContext will eventually be passed to other methods. Observers may hold NavigationContext pointer to track the navigation. Currently I can think of a few applications for holding on NavigationContext: a.) Better implementation of CWVNavigationDelegate (one from ios/web_view) b.) Add a reliable metric for Page Load Time If it will be hard to implement NavigationContext in the same way as NavigationHandle (keep it alive, while the navigation is in progress), then I will pass new NavigationContext to every call. Updated CL description to clarify those points. https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... File ios/web/public/web_state/web_state_observer.h (right): https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/web_state_observer.h:55: // NavigationHandle::IsErrorPage. On 2017/02/24 01:22:48, rohitrao wrote: > What is "NavigationHandle"? I don't see any other references to it in this > file. > > Oh, is this the original name of NavigationContext? Then this comment just > needs to be updated? Good catch. Done. https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/web_state_observer.h:58: // will still be ongoing in the WebState returned by |navigation_handle|. On 2017/02/24 01:22:48, rohitrao wrote: > And here. Done. https://codereview.chromium.org/2698413004/diff/100001/ios/web/public/web_sta... ios/web/public/web_state/web_state_observer.h:65: // change. To filter these out, use NavigationHandle::IsSamePage. On 2017/02/24 01:22:48, rohitrao wrote: > And here. Done. https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/navi... File ios/web/web_state/navigation_context_impl_unittest.mm (right): https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/navi... ios/web/web_state/navigation_context_impl_unittest.mm:14: class PlatformTestNavigationContextImplTest : public PlatformTest { On 2017/02/24 01:22:48, rohitrao wrote: > Could this fixture go into an anonymous namespace instead? > > Why is "PlatformTest" in the fixture name? Fixed fixture name, that was a mistake. As for an anonymous namespace, there was a discussion on Chromium dev with conclusion to put fixtures to the same namespace as class that being tested. I agree with that conclusion and trying to follow that recommendation when I'm writing tests. https://groups.google.com/a/chromium.org/forum/#!topicsearchin/chromium-dev/a... https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698413004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:4902: BOOL isSameDocumentNavigaiton = On 2017/02/24 01:22:48, rohitrao wrote: > Typo: navigation Done.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by eugenebut@chromium.org
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org, kkhorimoto@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2698413004/#ps120001 (title: "Addressed review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487904971449430,
"parent_rev": "8c4c9243dfc3d8ef05562ae4e3f858956ec4ae14", "commit_rev":
"17faf252763df7fb3dbe510549ed5fd8e94e84db"}
Message was sent while issue was closed.
Description was changed from ========== Implemented WebStateObserver::DidFinishNavigation(NavigationContext*). This callback aims to replace the following methods: WebStateObserver::UrlHashChanged() WebStateObserver::HistoryStateChanged() -[CRWWebDelegate webDidUpdateHistoryStateWithPageURL:] -[CRWWebDelegate webWillFinishHistoryNavigationFromEntry:] -[CRWWebDelegate webDidStartLoadingURL:shouldUpdateHistory:] These callbacks will be replaced in a separate CL. Currently NavigationContext is created only for DidFinishNavigation callback. Long term plan is to create it for DidStartNavigation and keep it alive until DidFinishNavigation. This CL has small logic change by updating MIME type for Native Content navigation, which is the right thing to do anyway. BUG=692331 ========== to ========== Implemented WebStateObserver::DidFinishNavigation(NavigationContext*). This callback aims to replace the following methods: WebStateObserver::UrlHashChanged() WebStateObserver::HistoryStateChanged() -[CRWWebDelegate webDidUpdateHistoryStateWithPageURL:] -[CRWWebDelegate webWillFinishHistoryNavigationFromEntry:] -[CRWWebDelegate webDidStartLoadingURL:shouldUpdateHistory:] These callbacks will be replaced in a separate CL. Currently NavigationContext is created only for DidFinishNavigation callback. Long term plan is to create it for DidStartNavigation and keep it alive until DidFinishNavigation. This CL has small logic change by updating MIME type for Native Content navigation, which is the right thing to do anyway. BUG=692331 Review-Url: https://codereview.chromium.org/2698413004 Cr-Commit-Position: refs/heads/master@{#452732} Committed: https://chromium.googlesource.com/chromium/src/+/17faf252763df7fb3dbe510549ed... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/17faf252763df7fb3dbe510549ed... |
