[webkit-reviews] review granted: [Bug 210013] REGRESSION (r259531?): [iOS] TestWebKitAPI.WebKitLegacy.ScrollingDoesNotPauseMedia is timing out : [Attachment 395634] Patch

[bugzilla-daemon at webkit.org]

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