Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2726243003:
Improve DCHECK error message for web::ExecuteJavaScript. (Closed)
|
Created:
3 years, 9 months ago by michaeldo Modified:
3 years, 9 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, Eugene But (OOO till 7-30) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove DCHECK error message for web::ExecuteJavaScript.
This improved messaging should help to pinpoint appearances of a bug where
WKWebView never calls back it's completion handler. (Webkit bug #149079)
BUG=684024
Review-Url: https://codereview.chromium.org/2726243003
Cr-Commit-Position: refs/heads/master@{#454633}
Committed: https://chromium.googlesource.com/chromium/src/+/082bcf16d886b6cf9cf063d350c213b7f95cc0ff
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use EXPECT_TRUE instead of DCHECK. #
Total comments: 7
Patch Set 3 : Use SysNSStringToUTF8 helper. #Messages
Total messages: 15 (7 generated)
Description was changed from ========== Improve DCHECK error message for web::ExecuteJavaScript. BUG=684024 ========== to ========== Improve DCHECK error message for web::ExecuteJavaScript. This improved messaging should help to pinpoint appearances of a bug where WKWebView never calls back it's completion handler. (Webkit bug #149079) BUG=684024 ==========
Description was changed from ========== Improve DCHECK error message for web::ExecuteJavaScript. This improved messaging should help to pinpoint appearances of a bug where WKWebView never calls back it's completion handler. (Webkit bug #149079) BUG=684024 ========== to ========== Improve DCHECK error message for web::ExecuteJavaScript. This improved messaging should help to pinpoint appearances of a bug where WKWebView never calls back it's completion handler. (Webkit bug #149079) BUG=684024 ==========
michaeldo@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2726243003/diff/1/ios/web/public/test/js_test... File ios/web/public/test/js_test_util.mm (right): https://codereview.chromium.org/2726243003/diff/1/ios/web/public/test/js_test... ios/web/public/test/js_test_util.mm:31: DCHECK(success) Ideally we should not DCHECK in tests. Can we log instead?
https://codereview.chromium.org/2726243003/diff/1/ios/web/public/test/js_test... File ios/web/public/test/js_test_util.mm (right): https://codereview.chromium.org/2726243003/diff/1/ios/web/public/test/js_test... ios/web/public/test/js_test_util.mm:31: DCHECK(success) On 2017/03/02 21:27:26, Eugene But wrote: > Ideally we should not DCHECK in tests. Can we log instead? We can log, however I didn't want to change the current behavior. (This currently DCHECKS.) This will only hit if WKWebView doesn't call back as we expect which isn't technically part of the test. I think it's an assumption that this will complete in time, but of course the Javascript could be the issue causing it not to return in time too. WDYT?
https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... File ios/web/public/test/js_test_util.mm (right): https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... ios/web/public/test/js_test_util.mm:33: EXPECT_TRUE(success) I already have another CL in which I'd like to add these lines. WDYT about adding: void testing::ExpectCondition(NSTimeInterval interval, ConditionBlock block, NSString failureMessage) to ios/testing/wait_util.h Internally, it can still print the stack so the context will be the same in the debug output (with the addition of one extra frame., but it would greatly cleanup the caller code here.
lgtm https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... File ios/web/public/test/js_test_util.mm (right): https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... ios/web/public/test/js_test_util.mm:33: EXPECT_TRUE(success) On 2017/03/03 00:11:00, michaeldo wrote: > I already have another CL in which I'd like to add these lines. WDYT about > adding: > > void testing::ExpectCondition(NSTimeInterval interval, ConditionBlock block, > NSString failureMessage) > > to ios/testing/wait_util.h > > Internally, it can still print the stack so the context will be the same in the > debug output (with the addition of one extra frame., but it would greatly > cleanup the caller code here. I believe we should not have different word ("Expect") for the same method and test assertions should happen in the test, not inside the helper. This CL is an improvement for sure, but I don't think we should add new functions. https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... ios/web/public/test/js_test_util.mm:35: << [[[NSThread callStackSymbols] componentsJoinedByString:@"\n"] Please use sys_string_conversions.h instead of NSString API (which can return nil in many cases). https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... ios/web/public/test/js_test_util.mm:68: cStringUsingEncoding:NSUTF8StringEncoding]; ditto
https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... File ios/web/public/test/js_test_util.mm (right): https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... ios/web/public/test/js_test_util.mm:33: EXPECT_TRUE(success) On 2017/03/03 01:48:06, Eugene But wrote: > On 2017/03/03 00:11:00, michaeldo wrote: > > I already have another CL in which I'd like to add these lines. WDYT about > > adding: > > > > void testing::ExpectCondition(NSTimeInterval interval, ConditionBlock block, > > NSString failureMessage) > > > > to ios/testing/wait_util.h > > > > Internally, it can still print the stack so the context will be the same in > the > > debug output (with the addition of one extra frame., but it would greatly > > cleanup the caller code here. > I believe we should not have different word ("Expect") for the same method and > test assertions should happen in the test, not inside the helper. This CL is an > improvement for sure, but I don't think we should add new functions. Acknowledged. https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... ios/web/public/test/js_test_util.mm:35: << [[[NSThread callStackSymbols] componentsJoinedByString:@"\n"] On 2017/03/03 01:48:06, Eugene But wrote: > Please use sys_string_conversions.h instead of NSString API (which can return > nil in many cases). Of course, thank you. https://codereview.chromium.org/2726243003/diff/20001/ios/web/public/test/js_... ios/web/public/test/js_test_util.mm:68: cStringUsingEncoding:NSUTF8StringEncoding]; On 2017/03/03 01:48:06, Eugene But wrote: > ditto Done.
The CQ bit was checked by michaeldo@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/2726243003/#ps40001 (title: "Use SysNSStringToUTF8 helper.")
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": 40001, "attempt_start_ts": 1488565128127630,
"parent_rev": "e09b87d2ba3bc1a8c616d836fe10b8cb604237cc", "commit_rev":
"082bcf16d886b6cf9cf063d350c213b7f95cc0ff"}
Message was sent while issue was closed.
Description was changed from ========== Improve DCHECK error message for web::ExecuteJavaScript. This improved messaging should help to pinpoint appearances of a bug where WKWebView never calls back it's completion handler. (Webkit bug #149079) BUG=684024 ========== to ========== Improve DCHECK error message for web::ExecuteJavaScript. This improved messaging should help to pinpoint appearances of a bug where WKWebView never calls back it's completion handler. (Webkit bug #149079) BUG=684024 Review-Url: https://codereview.chromium.org/2726243003 Cr-Commit-Position: refs/heads/master@{#454633} Committed: https://chromium.googlesource.com/chromium/src/+/082bcf16d886b6cf9cf063d350c2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/082bcf16d886b6cf9cf063d350c2... |
