742217 - Reduce the use of namespaces in Paris bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Two salient review comments:

---------------------------------------------------------
>+  typedef mozilla::dom::bindings::prototypes::XMLHttpRequestResponseType::value

How about mozilla::dom::XMLHttpRequestResponseType or something?  In general, de-namespacify.

>+  mozilla::dom::binding::Register(nameSpaceManager);
>+
>+  // Paris Bindings.
>+  mozilla::dom::bindings::Register(nameSpaceManager);
This is horrible.
Maybe
mozilla::dom::RegisterListBindings(nameSpaceManager);
mozilla::dom::RegisterBindings(nameSpaceManager);
---------------------------------------------------------

We should think about how to actually set this up... This probably blocks other binding work for the moment, since the sooner we do this, the less stuff we have to convert.

Let the bikeshedding begin.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

See long checkin comment for details

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 615623 [details] [diff] [review]
Proposed patch

Review of attachment 615623 [details] [diff] [review]:
-----------------------------------------------------------------

I general I like it.  While we're bikeshedding, I'd actually invert the names (dom for utility code to implement the dom, binding for the bindings).

> "using mozilla::dom" likewise doesn't import any names
> that match the IDL interface names. This allows C++ code to sanely use the IDL
> interface names for concrete classes, which is fairly desirable.

How often do we need to do this though? It seems to me that the main use is to wrap an object, we could get away with |using mozilla| and call |dom::Foo::Wrap| (or |binding::Foo::Wrap| as I'd prefer) and there wouldn't be any conflict?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I'd actually invert the names

So that would get us mozilla::dom::UnwrapObject and mozilla::binding::XMLHttpRequest?

We could do that, I guess.

> How often do we need to do this though?

If we put the utility methods in mozilla::dom, then people would pretty much never have to "using namespace mozilla::bindings", I suspect.  So yeah, then we could have the stuff under there be just the interface names.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The one drawback is that this gives us mozilla/dom/Utils.h.  Do we want to rename it to BindingUtils.h?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Also, mozilla/dom/Common.h is a bit odd...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Another potential problem.  At some point, we'll likely do s/nsXMLHttpRequest/mozilla::dom::XMLHttpRequest/, right?

At that point, we may run into issues with mozilla::bindings::XMLHttpRequest vs mozilla::dom::XMLHttpRequest, because we want a "using namespace mozilla::dom" in the binding .cpp files so we can access the Utils.h stuff.  I guess if it comes to that we can go with mozilla::bindings::XMLHttpRequestBinding at that point.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I suppose we could put Common.h in mozilla::bindings while Utils.h goes into mozilla::dom, too.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Another potential problem.  At some point, we'll likely do
> s/nsXMLHttpRequest/mozilla::dom::XMLHttpRequest/, right?

Actually, we already have this problem with mozilla::dom::workers::EventTarget vs mozilla::bindings::EventTarget if I "using namespace mozilla::bindings".  :(  I'll work around it for the moment...

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The plan we had on IRC was mozilla::dom::FooBinding for generated code, which prevents the name collisions (even after switching to mozilla::bindings), no?

Comment 10

[Boris Zbarsky [:bzbarsky]]

If we're OK with mozilla::bindings::FooBinding, then yes.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Switching dom and bindings

Though maybe we should still put Common.h/cpp under mozilla::bindings and mozilla/bindings?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: With FooBinding_workers instead of FooWorkers_binding

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 616243 [details] [diff] [review]
Switching dom and bindings

Review of attachment 616243 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +65,5 @@
>  #include "nsContentUtils.h"
>  #include "nsCCUncollectableMarker.h"
>  #include "nsDOMJSUtils.h" // for GetScriptContextFromJSContext
>  #include "xpcpublic.h"
> +#include "mozilla/dom/Utils.h"

Woooo. Let's rename this to BindingUtils.h at least. Everyone will make this their new favorite dumping ground.

::: dom/base/nsDOMClassInfo.cpp
@@ +58,5 @@
>  #include "xpcpublic.h"
>  #include "xpcprivate.h"
>  #include "XPCWrapper.h"
>  
> +#include "mozilla/dom/Common.h"

Need to fix this name too.

::: dom/bindings/Makefile.in
@@ +44,5 @@
>    $(filter %.cpp, $(globalgen_targets)) \
>    Utils.cpp \
>    $(NULL)
>  
> +EXPORTS_NAMESPACES = $(binding_include_path) mozilla/dom

Nit: I'd follow the pattern:

  FOO = \
    a \
    b \
    $(NULL)

::: dom/bindings/Utils.cpp
@@ -12,3 @@
>  
>  static bool
>  DefineConstants(JSContext* cx, JSObject* obj, ConstantSpec* cs)

So now we'll just have 'mozilla::dom::DefineConstants'? If someone were to ask review from me when adding such a function I would reject it and request a better name. What other things like this do we need to fix?

::: dom/workers/EventTarget.h
@@ +10,5 @@
>  
>  // I hate having to export this...
>  #include "mozilla/dom/workers/bindings/EventListenerManager.h"
>  
> +#include "mozilla/dom/Nullable.h"

This will need a better name too.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Need to fix this name too.

BindingsCommon?

> So now we'll just have 'mozilla::dom::DefineConstants'? 

Yes.  That's why I initially put the general stuff under mozilla::bindings....

> This will need a better name too.

Why?  That one actually makes total sense as a general DOM thing.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip, still waiting on Peter's review before addressing Ben's comments

Comment 16

[Peter Van der Beken [:peterv]]

Given that we're postfixing binding namespaces with "Binding", why can't we put everything in mozilla::dom? Enums might conflict, but how likely is that?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Per discussion with Peter just now:

1)  Put it all in mozilla::dom.  If enums become a problem, we'll sort it out.
2)  Rename "Utils" to "BindingUtils"
3)  Rename "Common" to "RegisterBindings"; it's only used to register our stuff, and can go
    away once Window moves to new bindings.

Comment 18

[Peter Van der Beken [:peterv]]

r=me for the changes in comment 17.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, so enums are definitely a problem.  Something like:

  typedef mozilla::dom::XMLHttpRequestResponseType XMLHttpRequestResponseType;

doesn't work in code that's inside mozilla::dom or is "using mozilla::dom;".

I talked to Ben, and we decided on the following plan for enums:

  namespace XMLHttpRequestResponseTypeValues {
    enum valuelist {
      // values here
    };
    extern const EnumEntry strings[10];
  } // namespace XMLHttpRequestResponseTypeValues
  typedef XMLHttpRequestResponseTypeValues::valuelist XMLHttpRequestResponseType;

And then consumers would use XMLHttpRequestResponseType for the type and XMLHttpRequestResponseTypeValues::blob for the values.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3576844511

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c3576844511