Reduce the use of namespaces in Paris bindings

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Created attachment 615623 [details] [diff] [review]
Proposed patch

See long checkin comment for details
Attachment #615623 - Flags: review?(peterv)
Attachment #615623 - Flags: review?(bent.mozilla)
Whiteboard: [need review]
Depends on: 742170
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?
> 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.
The one drawback is that this gives us mozilla/dom/Utils.h.  Do we want to rename it to BindingUtils.h?
Also, mozilla/dom/Common.h is a bit odd...
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.
I suppose we could put Common.h in mozilla::bindings while Utils.h goes into mozilla::dom, too.
> 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...
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?
If we're OK with mozilla::bindings::FooBinding, then yes.
Created attachment 616243 [details] [diff] [review]
Switching dom and bindings

Though maybe we should still put Common.h/cpp under mozilla::bindings and mozilla/bindings?
Attachment #616243 - Flags: review?(peterv)
Attachment #616243 - Flags: review?(bent.mozilla)
Attachment #615623 - Attachment is obsolete: true
Attachment #615623 - Flags: review?(peterv)
Attachment #615623 - Flags: review?(bent.mozilla)
Created attachment 616284 [details] [diff] [review]
With FooBinding_workers instead of FooWorkers_binding
Attachment #616284 - Flags: review?(peterv)
Attachment #616284 - Flags: review?(bent.mozilla)
Attachment #616243 - Attachment is obsolete: true
Attachment #616243 - Flags: review?(peterv)
Attachment #616243 - Flags: review?(bent.mozilla)
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.
Attachment #616243 - Attachment is obsolete: false
> 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.
Attachment #616243 - Attachment is obsolete: true
Blocks: 747827
Created attachment 617389 [details] [diff] [review]
Merged to tip, still waiting on Peter's review before addressing Ben's comments
Attachment #617389 - Flags: review?(peterv)
Attachment #616284 - Attachment is obsolete: true
Attachment #616284 - Flags: review?(peterv)
Attachment #616284 - Flags: review?(bent.mozilla)
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?
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.
r=me for the changes in comment 17.
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.
Whiteboard: [need review] → [need landing]
Created attachment 620417 [details] [diff] [review]
Updated to comments
Attachment #617389 - Attachment is obsolete: true
Attachment #617389 - Flags: review?(peterv)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3576844511
Assignee: nobody → bzbarsky
Flags: in-testsuite-
Whiteboard: [need landing]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/3c3576844511
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.