Bug 193881

Summary: Use a load optimizer for some sites
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: aestes, bfulgham, commit-queue, ews, jiewen_tan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
bfulgham: review+, bfulgham: commit-queue-
Patch for landing none

Description Jiewen Tan 2019-01-26 22:18:59 PST
Use a load optimizer for some sites.
Comment 1 Jiewen Tan 2019-01-26 22:22:12 PST
<rdar://problem/46325455>
Comment 2 Jiewen Tan 2019-01-26 22:41:04 PST
Created attachment 360270 [details]
Patch
Comment 3 Jiewen Tan 2019-01-27 00:23:52 PST
Created attachment 360275 [details]
Patch
Comment 4 Jiewen Tan 2019-01-27 01:18:28 PST
Created attachment 360281 [details]
Patch
Comment 5 Build Bot 2019-01-27 01:19:40 PST
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.
Comment 6 Jiewen Tan 2019-01-27 01:28:12 PST
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 :
Comment 7 Brent Fulgham 2019-01-27 11:04:55 PST
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.
Comment 8 youenn fablet 2019-01-27 11:26:59 PST
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.
Comment 9 Jiewen Tan 2019-01-27 11:27:36 PST
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.

: )
Comment 10 Jiewen Tan 2019-01-27 12:00:31 PST
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.
Comment 11 Jiewen Tan 2019-01-27 12:06:21 PST
Created attachment 360304 [details]
Patch for landing
Comment 12 Jiewen Tan 2019-01-27 12:14:48 PST
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>
Comment 13 WebKit Commit Bot 2019-01-27 12:44:10 PST
Comment on attachment 360304 [details]
Patch for landing

Clearing flags on attachment: 360304

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