Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892193002:
Add unittest to test CWVWebView. (Closed)
DescriptionAdd unittest target to test CWVWebView.
Add test which validates CWVWebView's canGoBack and canGoForward KVO properties.
BUG=719990
Review-Url: https://codereview.chromium.org/2892193002
Cr-Commit-Position: refs/heads/master@{#474350}
Committed: https://chromium.googlesource.com/chromium/src/+/8ffc03d37d7e7f39ead0994db70dcf0b91fffb4f
Patch Set 1 #
Total comments: 41
Patch Set 2 : Respond to comments. #
Total comments: 24
Patch Set 3 : Respond to comments. #
Total comments: 8
Patch Set 4 : Respond to comments. #
Messages
Total messages: 18 (6 generated)
michaeldo@chromium.org changed reviewers: + eugenebut@chromium.org, gab@chromium.org, mmenke@chromium.org
mmenke@ for DEPS on //net gab@ for DEPS on //base eugenebut@ for overall review
Thanks you for the tests! https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... File ios/web_view/test/boolean_observer.h (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.h:10: // Class to observe a boolean KVO compliant property. Could you please explain in the comments how to use this class. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.h:15: @property(nonatomic, readonly) BOOL lastValue; Should we implement more generic class that can observe all types? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.h:25: - (void)setObservedObject:(NSObject*)object keyPath:(NSString*)keyPath; Do you want to use nullability annotation and explain that object can be null but keyPath can't? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... File ios/web_view/test/boolean_observer.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.mm:46: _lastValue = [object performSelector:selector]; Is this something you can get from |change| dictionary? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... File ios/web_view/test/chrome_web_view_kvo_unittest.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:7: This test creates local HTTP server and it is not a unit test. Do you want to rename this to inttest (*int for integration)? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:20: class ChromeWebViewKVOTest : public ChromeWebViewTest {}; ChromeWebViewKvoTest because it's C++ https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:24: TEST_F(ChromeWebViewKVOTest, CanGoBackForward) { Please add comments https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:28: [[CWVWebView alloc] initWithFrame:CGRectMake(0.0, 0.0, 100.0, 100.0) Is this something that you want to do in ChromeWebViewKVOTest's constructor? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:40: // Define pages in reverse order so the links can reference the "next" page. This code which sets up server responses looks different from ios_web_inttest and EG test code. Can we be more consistent? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:49: ChromeWebViewTest::LoadURL(web_view, net::NSURLWithGURL(GURL(page_1))); You don't need ChromeWebViewTest:: https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... File ios/web_view/test/chrome_web_view_test.h (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.h:18: // Loads |URL| in |web_view|. Please document that this method waits until URL is loaded. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.h:21: // Waits until |web_view| stops loading. Please document what happens on timeout. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... File ios/web_view/test/chrome_web_view_test.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.mm:24: ios_web_view::TestServer::Start(); Can we be more consistent with ios_web_inttets and EG tests here? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.mm:43: EXPECT_TRUE(success); Should this be ASSERT_TRUE? There is no point in running the test if page did not load. Right? https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... File ios/web_view/test/test_server.h (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.h:12: class TestServer { I think we should be consistent with other tests in the way how Local Server is setup. So if there are any helpers you'd like to create then we need to make them generic and reusable in other tests. But that should be a separate CL and a separate discussion. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.h:14: static bool Start(); Generally there is no need to create a class which only has static methods. Functions placed in C++ namespacing work pretty well. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/web_view_... File ios/web_view/test/web_view_interaction_test_util.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/web_view_... ios/web_view/test/web_view_interaction_test_util.mm:20: stringWithFormat:@"(function() {" This script already exist somewhere else. Is there a good way to share it?
LGTM, just a couple suggestions. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... File ios/web_view/test/test_server.cc (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.cc:20: std::unique_ptr<net::EmbeddedTestServer> g_test_server; Per style guide, non-POD globals are forbidden. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.cc:37: std::unique_ptr<net::test_server::HttpResponse> EchoPageHTMLBodyInResponse( I guess EmbeddedTestServer::AddDefaultHandlers() + /echoall?<text> doesn't meet your needs? It does use pre tags around the content. There's also echoraw, but that doesn't include HTTP headers. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.cc:96: } } // namespace ios_web_view https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.cc:96: } Blank line before ending namespace (For consistency with opening it, and the anonymous namespace above)
//base DEPS r-s lgtm
https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... File ios/web_view/test/boolean_observer.h (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.h:10: // Class to observe a boolean KVO compliant property. On 2017/05/19 18:39:48, Eugene But wrote: > Could you please explain in the comments how to use this class. Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.h:15: @property(nonatomic, readonly) BOOL lastValue; On 2017/05/19 18:39:48, Eugene But wrote: > Should we implement more generic class that can observe all types? Yes, but I will do this in a followup CL as part of supporting NSString KVO observing (to test CWVWebView title changes). In that CL we can determine the best way to abstract this into a more generic class or split it into a few specific observer classes. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.h:25: - (void)setObservedObject:(NSObject*)object keyPath:(NSString*)keyPath; On 2017/05/19 18:39:48, Eugene But wrote: > Do you want to use nullability annotation and explain that object can be null > but keyPath can't? Added. PTAL. I think |keyPath| can be null if object is allowed to be null. otherwise we should require nonnull for both params. (we could expose separate method to stop observing an object if necessary.) https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... File ios/web_view/test/boolean_observer.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/boolean_o... ios/web_view/test/boolean_observer.mm:46: _lastValue = [object performSelector:selector]; On 2017/05/19 18:39:48, Eugene But wrote: > Is this something you can get from |change| dictionary? Yes, it is cleaner to get out of |change|. Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... File ios/web_view/test/chrome_web_view_kvo_unittest.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:7: On 2017/05/19 18:39:48, Eugene But wrote: > This test creates local HTTP server and it is not a unit test. Do you want to > rename this to inttest (*int for integration)? Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:20: class ChromeWebViewKVOTest : public ChromeWebViewTest {}; On 2017/05/19 18:39:48, Eugene But wrote: > ChromeWebViewKvoTest because it's C++ Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:24: TEST_F(ChromeWebViewKVOTest, CanGoBackForward) { On 2017/05/19 18:39:48, Eugene But wrote: > Please add comments Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:28: [[CWVWebView alloc] initWithFrame:CGRectMake(0.0, 0.0, 100.0, 100.0) On 2017/05/19 18:39:48, Eugene But wrote: > Is this something that you want to do in ChromeWebViewKVOTest's constructor? Good idea, moved to constructor. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:40: // Define pages in reverse order so the links can reference the "next" page. On 2017/05/19 18:39:48, Eugene But wrote: > This code which sets up server responses looks different from ios_web_inttest > and EG test code. Can we be more consistent? This is consistent with components/cronet/ios/test but not ios/web/public/test. Unfortunately, if I convert this to use SetUpSimpleHttpServer, I need to add ios/web:test_support as a dep which causes lots of duplicate CRW* class warnings. The warnings are caused by building the test target with web and also including web inside the ChromeWebView framework. However, per mmenke's comment, I may be able to use echo-raw and echoall which is build into the EmbeddedServer logic that will reduce code needed in the http_server. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_kvo_unittest.mm:49: ChromeWebViewTest::LoadURL(web_view, net::NSURLWithGURL(GURL(page_1))); On 2017/05/19 18:39:48, Eugene But wrote: > You don't need ChromeWebViewTest:: Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... File ios/web_view/test/chrome_web_view_test.h (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.h:18: // Loads |URL| in |web_view|. On 2017/05/19 18:39:49, Eugene But wrote: > Please document that this method waits until URL is loaded. Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.h:21: // Waits until |web_view| stops loading. On 2017/05/19 18:39:49, Eugene But wrote: > Please document what happens on timeout. Done. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... File ios/web_view/test/chrome_web_view_test.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.mm:24: ios_web_view::TestServer::Start(); On 2017/05/19 18:39:49, Eugene But wrote: > Can we be more consistent with ios_web_inttets and EG tests here? Unfortunately, I don't think so. See my comment in the test file about why I can't include the public web test helpers. We need to figure out if we can completely hide symbols within the ChromeWebView.framework first. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/chrome_we... ios/web_view/test/chrome_web_view_test.mm:43: EXPECT_TRUE(success); On 2017/05/19 18:39:49, Eugene But wrote: > Should this be ASSERT_TRUE? There is no point in running the test if page did > not load. Right? You're right, changed to ASSERT_TRUE. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... File ios/web_view/test/test_server.cc (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.cc:20: std::unique_ptr<net::EmbeddedTestServer> g_test_server; On 2017/05/19 18:57:15, mmenke wrote: > Per style guide, non-POD globals are forbidden. You're right, thank you. I've removed this class as everything was static and moved the logic to the ChromeWebViewTest implementation. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.cc:37: std::unique_ptr<net::test_server::HttpResponse> EchoPageHTMLBodyInResponse( On 2017/05/19 18:57:15, mmenke wrote: > I guess EmbeddedTestServer::AddDefaultHandlers() + /echoall?<text> doesn't meet > your needs? It does use pre tags around the content. There's also echoraw, but > that doesn't include HTTP headers. Thanks for pointing that out, echotitle will work, but I don't think echoall will correctly render html. PTAL inside ChromeWebViewTest where I am still registering my own handler for the echo of html, but I can replace with echoall or echoraw if there is a better way to do it. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.cc:96: } On 2017/05/19 18:57:15, mmenke wrote: > Blank line before ending namespace (For consistency with opening it, and the > anonymous namespace above) Ack. File removed. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... File ios/web_view/test/test_server.h (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.h:12: class TestServer { On 2017/05/19 18:39:49, Eugene But wrote: > I think we should be consistent with other tests in the way how Local Server is > setup. So if there are any helpers you'd like to create then we need to make > them generic and reusable in other tests. But that should be a separate CL and a > separate discussion. Acknowledged. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/test_serv... ios/web_view/test/test_server.h:14: static bool Start(); On 2017/05/19 18:39:49, Eugene But wrote: > Generally there is no need to create a class which only has static methods. > Functions placed in C++ namespacing work pretty well. Good point, I removed this class and followed example in net/test/EmbeddedTestServer.h by creating a test server in Setup inside the test class which also fixes the global test ptr that was in this first patch. https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/web_view_... File ios/web_view/test/web_view_interaction_test_util.mm (right): https://codereview.chromium.org/2892193002/diff/1/ios/web_view/test/web_view_... ios/web_view/test/web_view_interaction_test_util.mm:20: stringWithFormat:@"(function() {" On 2017/05/19 18:39:49, Eugene But wrote: > This script already exist somewhere else. Is there a good way to share it? Unfortunately no at this point. But I will continue to try to solve the sharing issue between ios/chrome and ios/web_view. I've filed a bug and added it here.
Description was changed from ========== Add unittest to test CWVWebView. BUG=719990 ========== to ========== Add unittest target to test CWVWebView. Add test which validates CWVWebView's canGoBack and canGoForward KVO properties. BUG=719990 ==========
I like your helpers. But we have completely different helpers for all other tests and I don't want us to silently diverge. Menglu is deprecating web::HttpServer, so in the future it may happen that we will use something similar to your helpers. It's ok to land them now, but we should talk to baxley@ and huangml@ and design helpers which we should be able to use across all iOS code. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... File ios/web_view/test/boolean_observer.h (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.h:17: // KVO value change. NO if a change has not been observed. "has not been observed" and "observed NO" are not distinguishable. Should we use NSNumber instead? https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... File ios/web_view/test/boolean_observer.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.mm:18: if (_object) { No need for this check, it's perfectly fine to call a method on nil object. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.mm:25: if (keyPath) { Is is there a value in passing nil keyPath? Should this be a DCHECK instead? https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_kvo_inttest.mm:36: // Test CWVWebView correctly reports |canGoBack| and |canGoForward| state. s/Test/Tests that ? https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_kvo_inttest.mm:48: GURL page_3 = GetURLForPageWithTitle("Page 3"); nit: page_3_url ? https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.h (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.h:31: GURL GetURLForPageWithTitle(const std::string& title); Consider s/URL/Url to follow C++ Style Guide. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:25: const char kPageHTMLBodyPath[] = "/PageHTMLBody?"; Please add comments for helper constant and methods. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:25: const char kPageHTMLBodyPath[] = "/PageHTMLBody?"; s/HTML/Html https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:96: EXPECT_TRUE(success); ASSERT_TRUE per API comments? https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/web_v... File ios/web_view/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/web_v... ios/web_view/test/web_view_interaction_test_util.h:14: bool TapChromeWebViewElementWithId(CWVWebView* web_view, NSString* element_id); Please add comments
chrome_web_view_test.mm LGTM. A pair of optional comments. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:49: return base::MakeUnique<net::test_server::BasicHttpResponse>(); If you don't handle something, convention is just to return nullptr. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:71: test_server_.reset(); optional: I believe dynamic allocation is discouraged when not needed, and it doesn't seem needed for test_server_. Also not sure you get anything from shutting this down on teardown, though it needed, TestServer does have a method to shut it down.
That is certainly a valid concern. I will talk with them about this test code. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... File ios/web_view/test/boolean_observer.h (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.h:17: // KVO value change. NO if a change has not been observed. On 2017/05/23 19:49:33, Eugene But wrote: > "has not been observed" and "observed NO" are not distinguishable. Should we use > NSNumber instead? Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... File ios/web_view/test/boolean_observer.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.mm:18: if (_object) { On 2017/05/23 19:49:33, Eugene But wrote: > No need for this check, it's perfectly fine to call a method on nil object. Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.mm:25: if (keyPath) { On 2017/05/23 19:49:33, Eugene But wrote: > Is is there a value in passing nil keyPath? Should this be a DCHECK instead? It's the only current way to stop this object from observing, but maybe that isn't a feature that is needed. I can annotate both params with nonnull in header and not worry about nil keyPath. WDYT? https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_kvo_inttest.mm:36: // Test CWVWebView correctly reports |canGoBack| and |canGoForward| state. On 2017/05/23 19:49:33, Eugene But wrote: > s/Test/Tests that ? Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_kvo_inttest.mm:48: GURL page_3 = GetURLForPageWithTitle("Page 3"); On 2017/05/23 19:49:33, Eugene But wrote: > nit: page_3_url ? Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.h (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.h:31: GURL GetURLForPageWithTitle(const std::string& title); On 2017/05/23 19:49:33, Eugene But wrote: > Consider s/URL/Url to follow C++ Style Guide. Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:25: const char kPageHTMLBodyPath[] = "/PageHTMLBody?"; On 2017/05/23 19:49:34, Eugene But wrote: > Please add comments for helper constant and methods. Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:49: return base::MakeUnique<net::test_server::BasicHttpResponse>(); On 2017/05/23 20:48:27, mmenke wrote: > If you don't handle something, convention is just to return nullptr. Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:71: test_server_.reset(); On 2017/05/23 20:48:27, mmenke wrote: > optional: I believe dynamic allocation is discouraged when not needed, and it > doesn't seem needed for test_server_. Also not sure you get anything from > shutting this down on teardown, though it needed, TestServer does have a method > to shut it down. Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:96: EXPECT_TRUE(success); On 2017/05/23 19:49:33, Eugene But wrote: > ASSERT_TRUE per API comments? Done. https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/web_v... File ios/web_view/test/web_view_interaction_test_util.h (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/web_v... ios/web_view/test/web_view_interaction_test_util.h:14: bool TapChromeWebViewElementWithId(CWVWebView* web_view, NSString* element_id); On 2017/05/23 19:49:34, Eugene But wrote: > Please add comments Done.
lgtm https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... File ios/web_view/test/boolean_observer.mm (right): https://codereview.chromium.org/2892193002/diff/10001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.mm:25: if (keyPath) { On 2017/05/24 15:02:34, michaeldo wrote: > On 2017/05/23 19:49:33, Eugene But wrote: > > Is is there a value in passing nil keyPath? Should this be a DCHECK instead? > > It's the only current way to stop this object from observing, but maybe that > isn't a feature that is needed. I can annotate both params with nonnull in > header and not worry about nil keyPath. WDYT? I think if we need API for stopping the observing, it should be separate method. Using |nonnull| sounds great. https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/boole... File ios/web_view/test/boolean_observer.mm (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.mm:35: if (object != _object || keyPath != _keyPath) { Sorry, missed this during the previous round. ![_object isEqualTo:object] || ![_keyPath isEqualTo:keyPath] https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_kvo_inttest.mm:20: class ChromeWebViewKvoTest : public ios_web_view::ChromeWebViewTest { Do you want to explain in the comment what this fixture tests? https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.h (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.h:25: class ChromeWebViewTest : public PlatformTest { Do you want to explain in the comments what this fixture does in Setup? https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.mm (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:69: net::test_server::RegisterDefaultHandlers(test_server_.get()); nit: It is preferable to have SetUp code in constructor and use SetUp only when you need to call virtual functions. You will not be able to use ASSERT_TRUE in constructor though.
https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/boole... File ios/web_view/test/boolean_observer.mm (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/boole... ios/web_view/test/boolean_observer.mm:35: if (object != _object || keyPath != _keyPath) { On 2017/05/24 15:32:14, Eugene But wrote: > Sorry, missed this during the previous round. > > ![_object isEqualTo:object] || ![_keyPath isEqualTo:keyPath] Done. https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_kvo_inttest.mm:20: class ChromeWebViewKvoTest : public ios_web_view::ChromeWebViewTest { On 2017/05/24 15:32:14, Eugene But wrote: > Do you want to explain in the comment what this fixture tests? Done. https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.h (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.h:25: class ChromeWebViewTest : public PlatformTest { On 2017/05/24 15:32:14, Eugene But wrote: > Do you want to explain in the comments what this fixture does in Setup? Done. https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... File ios/web_view/test/chrome_web_view_test.mm (right): https://codereview.chromium.org/2892193002/diff/30001/ios/web_view/test/chrom... ios/web_view/test/chrome_web_view_test.mm:69: net::test_server::RegisterDefaultHandlers(test_server_.get()); On 2017/05/24 15:32:14, Eugene But wrote: > nit: It is preferable to have SetUp code in constructor and use SetUp only when > you need to call virtual functions. You will not be able to use ASSERT_TRUE in > constructor though. Makes sense, I left the Start call here, but the creation moved to constructor.
The CQ bit was checked by michaeldo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, mmenke@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2892193002/#ps50001 (title: "Respond to 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": 50001, "attempt_start_ts": 1495643872745810,
"parent_rev": "e87ca325b7641587a273968c077adac3a3fa5edb", "commit_rev":
"8ffc03d37d7e7f39ead0994db70dcf0b91fffb4f"}
Message was sent while issue was closed.
Description was changed from ========== Add unittest target to test CWVWebView. Add test which validates CWVWebView's canGoBack and canGoForward KVO properties. BUG=719990 ========== to ========== Add unittest target to test CWVWebView. Add test which validates CWVWebView's canGoBack and canGoForward KVO properties. BUG=719990 Review-Url: https://codereview.chromium.org/2892193002 Cr-Commit-Position: refs/heads/master@{#474350} Committed: https://chromium.googlesource.com/chromium/src/+/8ffc03d37d7e7f39ead0994db70d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/8ffc03d37d7e7f39ead0994db70d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
