[webkit-reviews] review granted: [Bug 210013] REGRESSION (r259531?): [iOS] TestWebKitAPI.WebKitLegacy.ScrollingDoesNotPauseMedia is timing out : [Attachment 395634] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 6 19:06:29 PDT 2020
Daniel Bates <dbates at webkit.org> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 210013: REGRESSION (r259531?): [iOS]
TestWebKitAPI.WebKitLegacy.ScrollingDoesNotPauseMedia is timing out
https://bugs.webkit.org/show_bug.cgi?id=210013
Attachment 395634: Patch
https://bugs.webkit.org/attachment.cgi?id=395634&action=review
--- Comment #5 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 395634
--> https://bugs.webkit.org/attachment.cgi?id=395634
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=395634&action=review
Patch looks good.
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:41
> +static bool didFinishLoad = false;
Ok as is. No change is needed. The current code is the optimal solution: no
initialization is needed.
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:42
> +static bool gotMainFrame = false;
Ditto
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:45
> +static bool readyToTest = false;
Ditto for this line and 2 more below.
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:46
> +static bool didReceivePause = false;
Ok as is. No change needed. Look like extra space after before =
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:79
> + RetainPtr<WebPreferences> preferences = [WebPreferences
standardPreferences];
Ok as is. No change needed. The optimal solution stores into raw pointer
because function returns process global singleton.
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:80
> + preferences.get().mediaDataLoadsAutomatically = YES;
Ok as is. No change needed. The optimal solution is to not use .get() and
switch to [preferences serX:....] notation because it aesthetically more
pleasing to read and idiomatic. Same remarks for next two lines below
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:91
> + [uiWebView loadRequest:[NSURLRequest requestWithURL:[NSBundle.mainBundle
URLForResource:@"one-video" withExtension:@"html"
subdirectory:@"TestWebKitAPI.resources"]]];
Ok as is. No change needed. There may be an existing helper method that returns
the resource URL relative to the bundle or a test function that does that +
load it (at least there is one on TestWKWebView$.
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:99
> + DOMHTMLMediaElement* video = (DOMHTMLMediaElement*)[[mainFrame
DOMDocument] querySelector:@"video"];
Ok as is. No change needed. The optimal solution uses RetainPtr because future
proofs this code should in the future is added that causes document
destruction.
> Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:113
> + DOMHTMLMediaElement* video = (DOMHTMLMediaElement*)[[mainFrame
DOMDocument] querySelector:@"video"];
Ok as is. No change needed. The optimal solution uses retainptr
More information about the webkit-reviews
mailing list