Bug 169156 - [iOS] The web process should inherit application state from UI process
Summary: [iOS] The web process should inherit application state from UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-03 16:47 PST by Eric Carlson
Modified: 2017-03-14 13:03 PDT (History)
5 users (show)

See Also:

Attachments
Proposed patch. (7.20 KB, patch)
2017-03-04 08:25 PST, Eric Carlson 		ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (7.32 KB, patch)
2017-03-04 14:00 PST, Eric Carlson 		no flags Details | Formatted Diff | Diff
Patch for landing. (4.68 KB, patch)
2017-03-14 08:58 PDT, Eric Carlson 		beidson: review+
Details | Formatted Diff | Diff
Patch for landing. (4.41 KB, patch)
2017-03-14 10:08 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-03-03 16:47:09 PST 
The web process should inherit application state from UI process
Comment 1 Radar WebKit Bug Importer 2017-03-03 16:49:26 PST 
<rdar://problem/30845473>
Comment 2 Eric Carlson 2017-03-04 08:25:08 PST 
Created attachment 303394 [details]
Proposed patch.
Comment 3 Jon Lee 2017-03-04 10:56:51 PST 
Comment on attachment 303394 [details]
Proposed patch.

R=me but needs WK2 reviewer.
Comment 4 Alexey Proskuryakov 2017-03-04 11:34:53 PST 
Comment on attachment 303394 [details]
Proposed patch.

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

Only a shallow review, as I don't know about the overall design.

> Source/WebKit2/Platform/spi/ios/CelestialSPI.h:25
> +

#pragma once

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:118
> +SOFT_LINK_CONSTANT_MAY_FAIL(Celestial, AVSystemController_PIDToInheritApplicationStateFrom, NSString*)

Misplaced star.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:135
> +        NSError* error;

Please initialize the variable to nil. Also, misplaced star.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:138
> +            WTFLogAlways("Failed to set up PID proxying");

It may be useful to log the error.
Comment 5 Eric Carlson 2017-03-04 13:59:50 PST 
Comment on attachment 303394 [details]
Proposed patch.

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

>> Source/WebKit2/Platform/spi/ios/CelestialSPI.h:25
>> +
> 
> #pragma once

Fixed.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:118
>> +SOFT_LINK_CONSTANT_MAY_FAIL(Celestial, AVSystemController_PIDToInheritApplicationStateFrom, NSString*)
> 
> Misplaced star.

Fixed.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:135
>> +        NSError* error;
> 
> Please initialize the variable to nil. Also, misplaced star.

Fixed.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:138
>> +            WTFLogAlways("Failed to set up PID proxying");
> 
> It may be useful to log the error.

Good idea, fixed.
Comment 6 Eric Carlson 2017-03-04 14:00:16 PST 
Created attachment 303406 [details]
Updated patch.
Comment 7 WebKit Commit Bot 2017-03-04 14:39:50 PST 
Comment on attachment 303406 [details]
Updated patch.

Clearing flags on attachment: 303406

Committed r213430: <http://trac.webkit.org/changeset/213430>
Comment 8 mitz 2017-03-04 14:48:07 PST 
Comment on attachment 303406 [details]
Updated patch.

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

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:139
> +    if (canLoadAVSystemController_PIDToInheritApplicationStateFrom()) {
> +        pid_t pid = WebProcess::singleton().presenterApplicationPid();
> +        NSError *error = nil;
> +        [[getAVSystemControllerClass() sharedAVSystemController] setAttribute:@(pid) forKey:AVSystemController_PIDToInheritApplicationStateFrom error:&error];
> +        if (error)
> +            WTFLogAlways("Failed to set up PID proxying: %s", [[error localizedDescription] UTF8String]);
> +    }

Why are we doing global, pre-process setup in WebPage initialization rather than in web process initialization?
Comment 9 Eric Carlson 2017-03-14 08:58:44 PDT 
Created attachment 304384 [details]
Patch for landing.

Address Dan's comments.
Comment 10 Brady Eidson 2017-03-14 09:22:46 PDT 
Comment on attachment 304384 [details]
Patch for landing.

After bots are happy (they appear to not be!)
Comment 11 Eric Carlson 2017-03-14 10:08:27 PDT 
Created attachment 304389 [details]
Patch for landing.
Comment 12 WebKit Commit Bot 2017-03-14 13:03:36 PDT 
Comment on attachment 304389 [details]
Patch for landing.

Clearing flags on attachment: 304389

Committed r213933: <http://trac.webkit.org/changeset/213933>
Comment 13 WebKit Commit Bot 2017-03-14 13:03:40 PDT 
All reviewed patches have been landed.  Closing bug.