Bug 193881 – Use a load optimizer for some sites

• Status: NEW
• Component: WebKit Misc.

[Jiewen Tan]

Use a load optimizer for some sites.

[Jiewen Tan]

<rdar://problem/46325455>

[Jiewen Tan]

Created attachment 360270 [details]
Patch

[Jiewen Tan]

Created attachment 360275 [details]
Patch

[Jiewen Tan]

Created attachment 360281 [details]
Patch

[EWS Watchlist]

Attachment 360281 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:21:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Comment on attachment 360281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360281&action=review

> Source/WebKit/ChangeLog:21
> +        * UIProcess/Cocoa/MediaCaptureUtilities.h

Added :

[Brent Fulgham]

Comment on attachment 360281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360281&action=review

R=me after fixing the ChangeLog and style warning.

> Source/WebKitLegacy/ChangeLog:9
> +        * WebKitLegacy.xcodeproj/project.pbxproj:

I don’t think this changelog is right. He load optimizer isn’t relevant to WebKitLegacy.

Did you mean to make this Changelog for the WebKit directory?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestLoadOptimizer.mm:29
> +#import <WebKitAdditions/TestLoadOptimizerAdditions.mm>

Not much of a test! :-)

I assume we’ll add some over the coming weeks.

[youenn fablet]

Comment on attachment 360281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360281&action=review

> Source/WebKit/UIProcess/Cocoa/LoadOptimizer.mm:27
> +#import "LoadOptimizer.h"

I would rename LoadOptimizer to NavigationLoadOptimizer since this is navigation load specific.
Maybe there is a more specific term than Optimizer as well, not sure.

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:472
> +        auto* localCompletionHandler = new WTF::Function<void (bool)>(WTFMove(completionHandler));

This preexisting code could be modernized.
Maybe we should use a BlockPtr?
Also, if there is a bug in LSAppLink which makes it call the completion handler twice, we would delete twice localCompletionHandler.

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:483
> +#if HAVE(LOAD_OPTIMIZER)

Ditto for LOAD_OPTIMIZER -> NAVIGATION_LOAD_OPTIMIZER

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:490
> +    completionHandler(false);

This could be written this way:
#if HAVE(LOAD_OPTIMIZER)
    page.websiteDataStore().loadOptimizer().tryLoading(navigationAction->request(), page, WTFMove(completionHandler));
#else
    completionHandler(false);
#endif
That way it is up to the loadOptimizer instance to know when it can or not optimize the load.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:242
> +    LoadOptimizer& loadOptimizer() { return m_loadOptimizer.get(); }

Ditto here, renaming loadOptimizer to navigationLoadOptimizer, might be better.

[Jiewen Tan]

Comment on attachment 360281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360281&action=review

Thanks Brent for r+ the patch.

>> Source/WebKitLegacy/ChangeLog:9
>> +        * WebKitLegacy.xcodeproj/project.pbxproj:
> 
> I don’t think this changelog is right. He load optimizer isn’t relevant to WebKitLegacy.
> 
> Did you mean to make this Changelog for the WebKit directory?

Sure, removed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestLoadOptimizer.mm:29
>> +#import <WebKitAdditions/TestLoadOptimizerAdditions.mm>
> 
> Not much of a test! :-)
> 
> I assume we’ll add some over the coming weeks.

: )

[Jiewen Tan]

Comment on attachment 360281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360281&action=review

Thanks Youenn and Brent for reviewing the patch.

>> Source/WebKit/UIProcess/Cocoa/LoadOptimizer.mm:27
>> +#import "LoadOptimizer.h"
> 
> I would rename LoadOptimizer to NavigationLoadOptimizer since this is navigation load specific.
> Maybe there is a more specific term than Optimizer as well, not sure.

I don't think we should focus too much on the naming.

>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:472
>> +        auto* localCompletionHandler = new WTF::Function<void (bool)>(WTFMove(completionHandler));
> 
> This preexisting code could be modernized.
> Maybe we should use a BlockPtr?
> Also, if there is a bug in LSAppLink which makes it call the completion handler twice, we would delete twice localCompletionHandler.

You are right. Could we modernize the code in a separate patch since it is unrelated to what I have done here? Bug 193886.

>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:490
>> +    completionHandler(false);
> 
> This could be written this way:
> #if HAVE(LOAD_OPTIMIZER)
>     page.websiteDataStore().loadOptimizer().tryLoading(navigationAction->request(), page, WTFMove(completionHandler));
> #else
>     completionHandler(false);
> #endif
> That way it is up to the loadOptimizer instance to know when it can or not optimize the load.

Good catch. I would keep that in mind. Since we have this lock step development process for things involving internal. I will address this later.

[Jiewen Tan]

Created attachment 360304 [details]
Patch for landing

[Jiewen Tan]

Comment on attachment 360281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360281&action=review

>>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:490
>>> +    completionHandler(false);
>> 
>> This could be written this way:
>> #if HAVE(LOAD_OPTIMIZER)
>>     page.websiteDataStore().loadOptimizer().tryLoading(navigationAction->request(), page, WTFMove(completionHandler));
>> #else
>>     completionHandler(false);
>> #endif
>> That way it is up to the loadOptimizer instance to know when it can or not optimize the load.
> 
> Good catch. I would keep that in mind. Since we have this lock step development process for things involving internal. I will address this later.

<rdar://problem/47582894>

[WebKit Commit Bot]

Comment on attachment 360304 [details]
Patch for landing

Clearing flags on attachment: 360304

Committed r240555: <https://trac.webkit.org/changeset/240555>