Bug 85835 – [chromium] Duplicated CppBoundClass under webkit/glue and DumpRenderTree/chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Xianzhu Wang]

They are duplicated code. And they cause errors when linking DumpRenderTree as a shared library (required on chromium-android to run DumpRenderTree within an apk):

ld: error: out/Release/obj.target/webkit/support/libglue.a(.../webkit/glue/cpp_bound_class.o): multiple definition of 'CppNPObject::deallocate(NPObject*)'
ld: .../third_party/WebKit/Tools/DumpRenderTree/chromium/CppBoundClass.o: previous definition here

I think we should combine the two sets of implementations. WebKit/chromium/public and WebKit/chromium/src seem the place, with CppBoundClass and CppVariant renamed to WebBoundClass and WebVariant.

[Tony Chang]

Where do we use src/webkit/glue/cpp_bound_class.cc? Can we just remove that version of the file?

[Tony Chang]

Oh, I bet it's used by the old layoutTestController.waitUntilDone/notifyDone implementation in test_shell used by a few test_shell_tests.

I'm planning on migrating those tests out as part of https://code.google.com/p/chromium/issues/detail?id=126514 .

[Xianzhu Wang]

(In reply to comment #2)
> Oh, I bet it's used by the old layoutTestController.waitUntilDone/notifyDone implementation in test_shell used by a few test_shell_tests.
> 
> I'm planning on migrating those tests out as part of https://code.google.com/p/chromium/issues/detail?id=126514 .

It's also used in several chromium source files to expose some JavaScript APIs:

./chrome/renderer/plugins/plugin_placeholder.h:12:#include "webkit/glue/cpp_bound_class.h"
./chrome/renderer/external_host_bindings.h:10:#include "webkit/glue/cpp_bound_class.h"
./content/renderer/dom_automation_controller.h:10:#include "webkit/glue/cpp_bound_class.h"
./content/renderer/web_intents_host.cc:21:#include "webkit/glue/cpp_bound_class.h"
./content/renderer/web_ui_bindings.h:11:#include "webkit/glue/cpp_bound_class.h"

[Tony Chang]

(In reply to comment #3)
> (In reply to comment #2)
> > Oh, I bet it's used by the old layoutTestController.waitUntilDone/notifyDone implementation in test_shell used by a few test_shell_tests.
> > 
> > I'm planning on migrating those tests out as part of https://code.google.com/p/chromium/issues/detail?id=126514 .
> 
> It's also used in several chromium source files to expose some JavaScript APIs:

That's too bad.  I thought there was some newer V8 extension system that we're supposed to be using for newer javascript APIs?  E.g., how do chrome extensions add APIs?

[Xianzhu Wang]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Oh, I bet it's used by the old layoutTestController.waitUntilDone/notifyDone implementation in test_shell used by a few test_shell_tests.
> > > 
> > > I'm planning on migrating those tests out as part of https://code.google.com/p/chromium/issues/detail?id=126514 .
> > 
> > It's also used in several chromium source files to expose some JavaScript APIs:
> 
> That's too bad.  I thought there was some newer V8 extension system that we're supposed to be using for newer javascript APIs?  E.g., how do chrome extensions add APIs?

I saw some using V8 API (e.g. chrome/renderer/extensions), some using WebKit::WebBindings (e.g. webkit/plugins/ppapi etc., and used by CppBoundClass in webkit/glue).

Wondering if we should keep WebKit::WebBindings or convert all of them to use V8 API.

[Xianzhu Wang]

Created attachment 140628 [details]
patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Tools/DumpRenderTree/chromium/LayoutTestController.h:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 19 files


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

[Tony Chang]

(In reply to comment #5)
> Wondering if we should keep WebKit::WebBindings or convert all of them to use V8 API.

I think we want to convert all of them to use V8 API.  I worry that adding WebBoundClass/WebVariant encourages the usage of the old API.

To solve your linker problem, can we just namespace either implementation?

[Xianzhu Wang]

(In reply to comment #9)
> (In reply to comment #5)
> > Wondering if we should keep WebKit::WebBindings or convert all of them to use V8 API.
> 
> I think we want to convert all of them to use V8 API.  I worry that adding WebBoundClass/WebVariant encourages the usage of the old API.
> 
> To solve your linker problem, can we just namespace either implementation?

Agreed. Filed crbug.com/127238 for that.

[Xianzhu Wang]

Name conflict resolved in http://src.chromium.org/viewvc/chrome?view=rev&revision=136112.