Bug 193881 - Use a load optimizer for some sites
Summary: Use a load optimizer for some sites
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-26 22:18 PST by Jiewen Tan
Modified: 2019-01-27 12:44 PST (History)
7 users (show)

See Also:


Attachments
Patch (25.72 KB, patch)
2019-01-26 22:41 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (24.74 KB, patch)
2019-01-27 00:23 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (25.13 KB, patch)
2019-01-27 01:18 PST, Jiewen Tan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (24.57 KB, patch)
2019-01-27 12:06 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>