Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2048123002:
Created TestRedirectObserver. (Closed)
|
Created:
4 years, 6 months ago by kkhorimoto Modified:
4 years, 6 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@const_navigation_manager_getter Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreated TestRedirectObserver.
This class observes redirects using WebStateObserver rather than the
net stack, and can be used by both KIF and EG tests.
BUG=589968
Committed: https://crrev.com/f34caa12e3b2b955ea7ef930a0f5354eeef13f54
Cr-Commit-Position: refs/heads/master@{#400749}
Patch Set 1 #Patch Set 2 : Added ivar comments #
Total comments: 26
Patch Set 3 : test_redirect_observer #
Total comments: 2
Patch Set 4 : Added comments #
Depends on Patchset: Messages
Total messages: 21 (8 generated)
kkhorimoto@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.h (right): https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:17: typedef std::pair<GURL, GURL> URLPair; Do you want to use struct instead of std::pair? first and second fields do not perfectly comminicate original and last urls. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:17: typedef std::pair<GURL, GURL> URLPair; Should this typedef be inside TestRedirectObserver class? https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:31: void BeginObservingRedirectsForURL(const GURL& url); Is |url| the same as |original_url|? If so please use the same name. If no, then I'm not sure if I understand the difference... https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:34: GURL GetFinalURLForURL(const GURL& original_url); Per C++ Style Guide this should be GetFinalUrlForUrl. Unless you have a good reason to use all caps. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:37: void ProvisionalNavigationStarted(const GURL& url) override; Should this be private? https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:40: // TestRedirectObservers must be instantiated using |FromWebState()|. Is this comment related to TestRedirectObserverUserDataWrapper? https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.mm (right): https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:5: #import "ios/web/public/test/test_redirect_observer.h" s/import/include https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:8: #include "ios/web/public/navigation_item.h" s/include/import https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:10: #include "ios/web/public/web_state/web_state.h" s/include/import https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:23: class TestRedirectObserverUserDataWrapper Is having Wrapper object an existing pattern for base::SupportsUserData? Is there a reason why you don't want TestRedirectObserver to inherit from base::SupportsUserData::Data? https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:69: if (url_pair_for_item.second.first == original_url) Can you create an explanation variable for |url_pair_for_item.second|? https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:72: return GURL(); Whet this can happen? Is this a valid flow?
https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.h (right): https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:17: typedef std::pair<GURL, GURL> URLPair; On 2016/06/08 22:52:07, Eugene But wrote: > Should this typedef be inside TestRedirectObserver class? Done. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:31: void BeginObservingRedirectsForURL(const GURL& url); On 2016/06/08 22:52:07, Eugene But wrote: > Is |url| the same as |original_url|? If so please use the same name. If no, then > I'm not sure if I understand the difference... Instead, I renamed the other parameter to simply |url|. I think if we wanted to get more descriptive parameter names, this function would take a variable called |expected_url| since it's added to |expected_urls_| until a page load begins with that URL, whereas GetFinalURLForURL() makes more sense to take a parameter called |original_url|, since it corresponds with the struct member. If we instead value consistency between these two functions, a common name that describes both inputs is |url|. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:34: GURL GetFinalURLForURL(const GURL& original_url); On 2016/06/08 22:52:07, Eugene But wrote: > Per C++ Style Guide this should be GetFinalUrlForUrl. Unless you have a good > reason to use all caps. Done. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:37: void ProvisionalNavigationStarted(const GURL& url) override; On 2016/06/08 22:52:07, Eugene But wrote: > Should this be private? I moved this to private and it compiles/works fine. I'm curious what the motivation for that suggestion is though. In other classes, overridden functions are declared with the same visibility as they are declared in the superclass. Moreover, how this code is actually executed is kinda confusing to me: ProvisionalNavigationStarted is called by WebStateImpl since it's part of WebStateObserver's public interface; what do we gain from moving it to the private interface here? https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:40: // TestRedirectObservers must be instantiated using |FromWebState()|. On 2016/06/08 22:52:07, Eugene But wrote: > Is this comment related to TestRedirectObserverUserDataWrapper? No, it's talking about TestRedirectObservers since all constructors for this class are private. TestRedirectObserverUserDataWrapper is a friend class here because it is responsible for owning/instantiating TestRedirectObservers. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.mm (right): https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:5: #import "ios/web/public/test/test_redirect_observer.h" On 2016/06/08 22:52:07, Eugene But wrote: > s/import/include Done. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:8: #include "ios/web/public/navigation_item.h" On 2016/06/08 22:52:07, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:10: #include "ios/web/public/web_state/web_state.h" On 2016/06/08 22:52:07, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:23: class TestRedirectObserverUserDataWrapper On 2016/06/08 22:52:08, Eugene But wrote: > Is having Wrapper object an existing pattern for base::SupportsUserData? Is > there a reason why you don't want TestRedirectObserver to inherit from > base::SupportsUserData::Data? I originally had TestRedirectObserver inherit from base::SupportsUserData::Data, but then we'd need a separate helper object that inherits from web::WebStateObserver since these classes aren't interfaces that can be used with multiple inheritance with our style guide. I figured that the WSO portion of the object seems more in line with its public API while SupportsUserData is more of an implementation detail that should be done in the implementation file. We do use wrapper objects for SupportsUserData in other places, though this is usually to bridge between objective c objects and c++. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:69: if (url_pair_for_item.second.first == original_url) On 2016/06/08 22:52:08, Eugene But wrote: > Can you create an explanation variable for |url_pair_for_item.second|? Done. https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:72: return GURL(); On 2016/06/08 22:52:08, Eugene But wrote: > Whet this can happen? Is this a valid flow? This can occur if this function is called for a URL that has not begun loading yet, and is handled by |+stepToVerifyURL|, which is the only caller of this code.
PTAL!
lgtm https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.h (right): https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:37: void ProvisionalNavigationStarted(const GURL& url) override; On 2016/06/14 22:54:19, kkhorimoto_ wrote: > On 2016/06/08 22:52:07, Eugene But wrote: > > Should this be private? > > I moved this to private and it compiles/works fine. I'm curious what the > motivation for that suggestion is though. In other classes, overridden > functions are declared with the same visibility as they are declared in the > superclass. Moreover, how this code is actually executed is kinda confusing to > me: ProvisionalNavigationStarted is called by WebStateImpl since it's part of > WebStateObserver's public interface; what do we gain from moving it to the > private interface here? Not much, but clients will know that they should not call ProvisionalNavigationStarted :) https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.mm (right): https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:72: return GURL(); On 2016/06/14 22:54:19, kkhorimoto_ wrote: > On 2016/06/08 22:52:08, Eugene But wrote: > > Whet this can happen? Is this a valid flow? > > This can occur if this function is called for a URL that has not begun loading > yet, and is handled by |+stepToVerifyURL|, which is the only caller of this > code. Thanks. Comment can be helpful here. https://codereview.chromium.org/2048123002/diff/40001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.h (right): https://codereview.chromium.org/2048123002/diff/40001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:18: class TestRedirectObserver : public web::WebStateObserver { Sorry, missed this during the previous round. Please add comments.
https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.mm (right): https://codereview.chromium.org/2048123002/diff/20001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.mm:72: return GURL(); On 2016/06/16 22:51:13, Eugene But wrote: > On 2016/06/14 22:54:19, kkhorimoto_ wrote: > > On 2016/06/08 22:52:08, Eugene But wrote: > > > Whet this can happen? Is this a valid flow? > > > > This can occur if this function is called for a URL that has not begun loading > > yet, and is handled by |+stepToVerifyURL|, which is the only caller of this > > code. > Thanks. Comment can be helpful here. Done. https://codereview.chromium.org/2048123002/diff/40001/ios/web/public/test/tes... File ios/web/public/test/test_redirect_observer.h (right): https://codereview.chromium.org/2048123002/diff/40001/ios/web/public/test/tes... ios/web/public/test/test_redirect_observer.h:18: class TestRedirectObserver : public web::WebStateObserver { On 2016/06/16 22:51:13, Eugene But wrote: > Sorry, missed this during the previous round. Please add comments. Done.
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2048123002/#ps60001 (title: "Added comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048123002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by kkhorimoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048123002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by kkhorimoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2048123002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Created TestRedirectObserver. This class observes redirects using WebStateObserver rather than the net stack, and can be used by both KIF and EG tests. BUG=589968 ========== to ========== Created TestRedirectObserver. This class observes redirects using WebStateObserver rather than the net stack, and can be used by both KIF and EG tests. BUG=589968 Committed: https://crrev.com/f34caa12e3b2b955ea7ef930a0f5354eeef13f54 Cr-Commit-Position: refs/heads/master@{#400749} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f34caa12e3b2b955ea7ef930a0f5354eeef13f54 Cr-Commit-Position: refs/heads/master@{#400749} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
