| Summary: | REGRESSION (r141051): Broke plugin support on non-Mac WebKit2 Ports | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Thiago Marcos P. Santos <tmpsantos> | ||||||
| Component: | WebKit2 | Assignee: | Thiago Marcos P. Santos <tmpsantos> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abecsi, andersca, cgarcia, cmarcelo, gustavo, hausmann, jturcotte, kenneth, menard, mrobinson, ossy, sam, webkit.review.bot, zarvai | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 107456, 79668, 108098 | ||||||||
| Attachments: |
|
||||||||
|
Description
Thiago Marcos P. Santos
2013-01-29 05:21:50 PST
Also on Qt5 http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/r141093%20%2813137%29/results.html Failing tests on Qt5-WK2: plugins/destroy-during-npp-new-object-with-fallback-content.html plugins/destroy-during-npp-new.html plugins/destroy-plugin-from-callback.html plugins/fullscreen-plugins-dont-reload.html plugins/get-empty-url.html plugins/get-user-agent-with-null-npp-from-npp-new.html plugins/inner-html-display-none.html plugins/instance-available-before-stylesheets-loaded-object.html plugins/instance-available-before-stylesheets-loaded.html plugins/multiple-plugins.html plugins/nested-plugin-objects.html plugins/netscape-destroy-plugin-script-objects.html plugins/netscape-plugin-map-data-to-src.html plugins/netscape-plugin-property-access-exception.html plugins/npruntime/construct.html plugins/npruntime/embed-property.html plugins/npruntime/enumerate.html plugins/npruntime/evaluate.html plugins/npruntime/get-int-identifier-special-values.html plugins/npruntime/get-property-return-value.html plugins/npruntime/identifier-conversion.html plugins/npruntime/invoke-browserfuncs.html plugins/npruntime/invoke-default.html plugins/npruntime/invoke-failure.html plugins/npruntime/invoke.html plugins/npruntime/npruntime-calls-with-null-npp.html plugins/npruntime/npruntime.html plugins/npruntime/overrides-all-properties.html plugins/npruntime/remove-property.html plugins/npruntime/round-trip-npobject.html plugins/npruntime/throw-exception.html plugins/object-embed-plugin-scripting.html plugins/override-node-method.html plugins/plugin-remove-subframe.html plugins/private-browsing-mode-2.html plugins/private-browsing-mode.html plugins/reloadplugins-no-pages.html plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html plugins/return-negative-one-from-write.html plugins/return-npobject.html plugins/script-object-invoke.html plugins/update-widgets-crash.html plugins/window-open.html Timeouting tests on Qt5-WK2: plugins/get-targeted-javascript-url.html plugins/get-url-with-javascript-destroying-plugin.html plugins/destroy-stream-twice.html plugins/get-url-with-javascript-url.html plugins/get-javascript-url.html plugins/get-url-notify-with-url-that-fails-to-load.html plugins/get-file-url.html plugins/get-url-with-iframe-target.html plugins/geturl-replace-query.html Fix is on the way. Created attachment 185255 [details]
Patch
Comment on attachment 185255 [details]
Patch
Can any of the WebKit2 owners review this patch? It's a test regression fix.
This is one workable approach, the other would be to introduce a way of passing extra initialization data for the other ports.
(In reply to comment #4) > (From update of attachment 185255 [details]) > Can any of the WebKit2 owners review this patch? It's a test regression fix. > > This is one workable approach, the other would be to introduce a way of passing extra initialization data for the other ports. The extra initialization data is only needed (so far) by Mac. This patch also moves it to inside the PLATFORM(MAC) boundaries. Comment on attachment 185255 [details]
Patch
This isn't the right approach. ProcessLauncher should support passing the extra initialization data along to the child process. See the part in createProcess in ProcessLauncherMac, where we iterate over the extraInitializationData map and create arguments.
(In reply to comment #6) > (From update of attachment 185255 [details]) > This isn't the right approach. ProcessLauncher should support passing the extra initialization data along to the child process. See the part in createProcess in ProcessLauncherMac, where we iterate over the extraInitializationData map and create arguments. That could be done (and was my initial approach to be honest), but wont solve the problem for all the !mac platforms at once, since each port has its own ProcessLauncher. This is what motivated me to basically restoring the original behavior on this patch. It is also worth mentioning that extraInitializationData is only needed by Mac. Could you please reconsider? Maybe we could land this patch and give time to each port migrate their ProcessLauncher to the solution you are suggesting. (In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 185255 [details] [details]) > > This isn't the right approach. ProcessLauncher should support passing the extra initialization data along to the child process. See the part in createProcess in ProcessLauncherMac, where we iterate over the extraInitializationData map and create arguments. > > That could be done (and was my initial approach to be honest), but wont solve the problem for all the !mac platforms at once, since each port has its own ProcessLauncher. > > This is what motivated me to basically restoring the original behavior on this patch. It is also worth mentioning that extraInitializationData is only needed by Mac. > > Could you please reconsider? Maybe we could land this patch and give time to each port migrate their ProcessLauncher to the solution you are suggesting. You should not feel compelled to fix this for all the ports. As mentioned in the past, the intent of our new review model is to allow these exact type of change, so I we are not going to reconsider this patch. (In reply to comment #8) > You should not feel compelled to fix this for all the ports. I think that we should try to make an effort to fix this for all the ports, because there is an opportunity of reducing or avoiding duplicated code. Thiago, what if for Gtk/Efl/Qt we translate the extra initialization data dictionary into environment variables to the launched process and then in the launched process iterate over the environment and get the values back? I guess that could be shared code because only PLATFORM(MAC) is using XPC and everyone else is using a regular child process. (In reply to comment #9) > (In reply to comment #8) > > You should not feel compelled to fix this for all the ports. > > I think that we should try to make an effort to fix this for all the ports, because there is an opportunity of reducing or avoiding duplicated code. > > Thiago, what if for Gtk/Efl/Qt we translate the extra initialization data dictionary into environment variables to the launched process and then in the launched process iterate over the environment and get the values back? > > I guess that could be shared code because only PLATFORM(MAC) is using XPC and everyone else is using a regular child process. Oops, I just found out that on Windows environment variable names are case insensitive. That could be an issue. Perhaps we can agree on only using lower-case strings for the keys? Created attachment 185507 [details]
Patch
Committed r141275: <http://trac.webkit.org/changeset/141275> |