Bug 129188 – [iOS][WebKit2] Adopt SPI for managing tabs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Pratik Solanki]

Adopt assertion SPI for managing tab lifecycle.

[Pratik Solanki]

<rdar://problem/15939827>

[Pratik Solanki]

Created attachment 224933 [details]
Patch

[Pratik Solanki]

Member variables and functions could do with a better name but I couldn't come up with anything. Let me know if this is fitting in well architecturally or if it needs to be somewhere else.

[Sam Weinig]

Comment on attachment 224933 [details]
Patch

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

> Source/WebKit2/Configurations/WebKit2.xcconfig:35
> +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework WebKit -lMobileGestalt -lassertion_extension;

Why is this only needed in the simulator?

> Source/WebKit2/Platform/IPC/Connection.h:283
> +public:
> +    xpc_connection_t xpcConnection() { return m_xpcConnection; }
> +

I would move this up to just below the line that reads: static bool identifierIsNull(Identifier identifier) { return identifier.port == MACH_PORT_NULL; }

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1031
> +#if PLATFORM(IOS)
> +    if (mayHaveChanged & ViewState::IsInWindow)
> +        process().setProcessState(m_viewState & ViewState::IsInWindow ? WebProcessProxy::Foreground : WebProcessProxy::Background);
> +#endif

Is this accounting for the case where a process has multiple WebPageProxies?

> Source/WebKit2/UIProcess/WebProcessProxy.h:228
> +#if PLATFORM(IOS)
> +public:
> +    enum ProcessState {
> +        Background,
> +        Foreground
> +    };
> +
> +    void setProcessState(ProcessState);
> +
> +private:
> +    void updateProcessState();
> +
> +    ProcessState m_processState;
> +#endif

If possible, I would love to keep this not iOS specific (with other ports just always being in the Foreground state). I would bet Gavin has opinions on this as well, given all the time he has spent lately, cleaning up ViewState stuff.

[Pratik Solanki]

Comment on attachment 224933 [details]
Patch

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

>> Source/WebKit2/Configurations/WebKit2.xcconfig:35
>> +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework WebKit -lMobileGestalt -lassertion_extension;
> 
> Why is this only needed in the simulator?

FRAMEWORK_AND_LIBRARY_LDFLAGS_iphoneos (below) includes FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator. iphonesimucator seems to be for frameworks applicable to both. iphoneos is hardware specific.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1031
>> +#endif
> 
> Is this accounting for the case where a process has multiple WebPageProxies?

Hmm.. So you mean when we have one web process handling multiple WebPageProxy. I don't think this is handling that. I see we have a vector of WebPageProxy objects in WebProcessProxy. Should we be going through this list every time so we don't mark a process as being background inadvertently?

>> Source/WebKit2/UIProcess/WebProcessProxy.h:228
>> +#endif
> 
> If possible, I would love to keep this not iOS specific (with other ports just always being in the Foreground state). I would bet Gavin has opinions on this as well, given all the time he has spent lately, cleaning up ViewState stuff.

Ok. I can do that and have empty implementations for other ports.

[Pratik Solanki]

Created attachment 225165 [details]
Patch

[Pratik Solanki]

Simpler patch that doesn't keep the state in WebProcessProxy. We don't really need to. Though it might be advantageous to have a local state for debugging purposes.

[Gavin Barraclough]

Comment on attachment 225165 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1028
> +    if (mayHaveChanged & ViewState::IsInWindow)

Hey Pratik, I think you should be able to check "changed" instead of "mayHaveChanged" – you should probably only need to update the process state if visibility actually has changed, right?
(I'm probably going to remove mayHaveChanged soon).

> Source/WebKit2/UIProcess/ios/WebProcessProxyIOS.mm:83
> +    copyValuesToVector(m_pageMap, pages);

I think you could just iterate the page map directly, instead of copying to a vector, using:
        for (const auto& page : m_pageMap.values())

[Pratik Solanki]

Committed r164737: <http://trac.webkit.org/changeset/164737>