Bug 47097 – Generate messages sent to the WebProcess class.

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

[Sam Weinig]

Generate messages sent to the WebProcess class.

[Sam Weinig]

Created attachment 69651 [details]
Patch

[Early Warning System Bot]

Attachment 69651 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4146072

[Adam Roben (:aroben)]

Comment on attachment 69651 [details]
Patch

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

> WebKit2/WebKit2.pro:180
>  HEADERS += \
>      Shared/CoreIPCSupport/DrawingAreaMessageKinds.h \
>      Shared/CoreIPCSupport/DrawingAreaProxyMessageKinds.h \
>      Shared/CoreIPCSupport/WebPageProxyMessageKinds.h \
> -    Shared/CoreIPCSupport/WebProcessMessageKinds.h \
>      Shared/CacheModel.h \
>      Shared/DrawingAreaBase.h \
>      Shared/ImmutableArray.h \

Seems like you need to add the new generated files.

> WebKit2/Platform/CoreIPC/HandleMessage.h:48
> +    // NOTE: Return value ignored.
> +    (object->*function)(firstArgument, secondArgument);
> +}

Why throw away the return value? Why not return it?

> WebKit2/Scripts/webkit2/messages.py:232
> +        if type.find("<") != -1:
> +            headers.update(headers_for_type(type))
> +            continue

Please use single quotes.

Please add tests that show that your new code works. (And make sure you didn't break any existing tests.)

> WebKit2/UIProcess/mac/WebProcessProxyMac.mm:42
> +        send(Messages::WebProcess::SetupAcceleratedCompositingPort(CoreIPC::MachPort(renderServerPort, MACH_MSG_TYPE_COPY_SEND)), 0);

Should be "SetUpAcceleratedCompositingPort", with a capital "U".

> WebKit2/WebProcess/WebProcess.cpp:194
> -void WebProcess::setCacheModel(CacheModel cacheModel)
> +void WebProcess::setCacheModel(uint32_t cacheModel)
>  {
> -    if (!m_hasSetCacheModel || cacheModel != m_cacheModel) {
> +    if (!m_hasSetCacheModel || static_cast<CacheModel>(cacheModel) != m_cacheModel) {
>          m_hasSetCacheModel = true;
> -        m_cacheModel = cacheModel;
> -        platformSetCacheModel(cacheModel);
> +        m_cacheModel = static_cast<CacheModel>(cacheModel);
> +        platformSetCacheModel(static_cast<CacheModel>(cacheModel));
>      }
>  }

Doing the cast once and storing the result in a local variable would be nicer.

> WebKit2/win/WebKit2.vcproj:766
>  					>
>  				</File>
>  				<File
> -					RelativePath="..\Shared\CoreIPCSupport\WebProcessMessageKinds.h"
> -					>
> -				</File>
> -				<File
>  					RelativePath="..\Shared\CoreIPCSupport\WebProcessProxyMessageKinds.h"
>  					>
>  				</File>
>  

Seems like you need to add the new generated files.

[Sam Weinig]

Created attachment 69660 [details]
Patch

[WebKit Review Bot]

Attachment 69660 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Scripts/webkit2/messages.py:230:  trailing whitespace  [pep8/W291] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 69660 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4177071

[Adam Roben (:aroben)]

Comment on attachment 69660 [details]
Patch

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

> WebKit2/ChangeLog:13
> +        * Scripts/webkit2/messages.py:
> +        * Scripts/webkit2/messages_unittest.py:
> +        Make this script work for passing a templated type. 

It would be nice to add the names of functions you changed in messages.py.

> WebKit2/WebProcess/WebProcess.cpp:194
> -void WebProcess::setCacheModel(CacheModel cacheModel)
> +void WebProcess::setCacheModel(uint32_t cacheModel)
>  {
> -    if (!m_hasSetCacheModel || cacheModel != m_cacheModel) {
> +    if (!m_hasSetCacheModel || static_cast<CacheModel>(cacheModel) != m_cacheModel) {
>          m_hasSetCacheModel = true;
> -        m_cacheModel = cacheModel;
> -        platformSetCacheModel(cacheModel);
> +        m_cacheModel = static_cast<CacheModel>(cacheModel);
> +        platformSetCacheModel(static_cast<CacheModel>(cacheModel));
>      }
>  }

I still think a local variable with type CacheModel would be better.

[Sam Weinig]

Landed in r69029.

[Csaba Osztrogonác]

(In reply to comment #8)
> Landed in r69029.

Hey, you shouldn't have commit it, because Qt EWS bubble is red.
It wasn't a fair play ...

[Sam Weinig]

(In reply to comment #9)
> (In reply to comment #8)
> > Landed in r69029.
> 
> Hey, you shouldn't have commit it, because Qt EWS bubble is red.
> It wasn't a fair play ...


Sorry, but I couldn't figure out how to make it work on Qt.  We have a DerivedSources.make file that the Qt build does not seem to be using.  If adding stuff to the project is prohibitively hard, it is likely that things will break.

[Csaba Osztrogonác]

> Sorry, but I couldn't figure out how to make it work on Qt.  We have a DerivedSources.make file that the Qt build does not seem to be using.  If adding stuff to the project is prohibitively hard, it is likely that things will break.

I prefer poking our WebKit2 guys and ask them to fix Qt 
build system instead of breaking Qt buildbots and EWS.

Next time, please cc Zoltan ([e-mail]), Balazs 
([e-mail]) and Andras ([e-mail]), they
can help you with fixing Qt-WebKit2 build system.

Fix landed in http://trac.webkit.org/changeset/69046