Bug 143283 – WebKitTestRunner Injected bundle messages should be at the page level

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

All the messaging to the UIProcess from the injected bundle should be page messages instead of context level messages.

[Enrica Casucci]

Created attachment 249895 [details]
Patch

[Tim Horton]

Comment on attachment 249895 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageInjectedBundleClient.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

The years don't look right

> Source/WebKit2/UIProcess/WebPageInjectedBundleClient.cpp:54
> +/*

This commented out thing probably should be deleted (?)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:49
> +// ?

? what did we decide here

> Tools/WebKitTestRunner/TestController.cpp:532
> +//        0 // getInjectedBundleInitializationUserData

ditto on the commented out stuff

[Enrica Casucci]

(In reply to comment #2)
> Comment on attachment 249895 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249895&action=review
> 
> > Source/WebKit2/UIProcess/WebPageInjectedBundleClient.cpp:2
> > + * Copyright (C) 2010 Apple Inc. All rights reserved.
> 
> The years don't look right
> 
There were even more wrong. I fixed them all.

> > Source/WebKit2/UIProcess/WebPageInjectedBundleClient.cpp:54
> > +/*
> 
> This commented out thing probably should be deleted (?)
>Yes, done.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:49
> > +// ?
> 
> ? what did we decide here
> 
Nothing officially. I think it is ok to have it here since it is internal.
I've removed the question mark.

> > Tools/WebKitTestRunner/TestController.cpp:532
> > +//        0 // getInjectedBundleInitializationUserData
> 
> ditto on the commented out stuff
Done.

[Enrica Casucci]

Created attachment 249935 [details]
Patch2

[Alexey Proskuryakov]

Comment on attachment 249935 [details]
Patch2

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4837
> +    auto& webProcess = WebProcess::singleton();

This variable is not needed.

> Tools/WebKitTestRunner/TestController.cpp:532
> +//        0 // getInjectedBundleInitializationUserData

?

> Tools/WebKitTestRunner/InjectedBundle/EventSendingController.cpp:212
> -    auto& injectedBungle = InjectedBundle::singleton();
> -    WKBundlePageRef page = injectedBungle.page()->page();
> +    auto& injectedBundle = InjectedBundle::singleton();
> +    WKBundlePageRef page = injectedBundle.page()->page();

This patch doesn't make it worse, but doesn't the code mean that it's not possible to use EventSendingController in a secondary window? Maybe it's easy to fix now?

[Enrica Casucci]

Created attachment 249949 [details]
Patch3

Trying to fix EFL and GTK builds

[Alexey Proskuryakov]

Comment on attachment 249949 [details]
Patch3

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4844
> +    auto& webProcess = WebProcess::singleton();

I feel kind of uneasy about this one, it's probably slightly better to call WebProcess::singleton() twice than to hold a reference to something over a sync call.

Anything can happen during a sync call, as synchronous messages from UI process can be executed. That's unlikely to invalidate a singleton, of course, but the way the code is structured now is unusual.

[Enrica Casucci]

(In reply to comment #7)
> Comment on attachment 249949 [details]
> Patch3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249949&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4844
> > +    auto& webProcess = WebProcess::singleton();
> 
> I feel kind of uneasy about this one, it's probably slightly better to call
> WebProcess::singleton() twice than to hold a reference to something over a
> sync call.
> 
> Anything can happen during a sync call, as synchronous messages from UI
> process can be executed. That's unlikely to invalidate a singleton, of
> course, but the way the code is structured now is unusual.

I agree with you. I'll change it.

[Enrica Casucci]

Fixed the remaining build issues with elf and gtk before landing.
Committed revision 182260.