Last Comment Bug 742217 - Reduce the use of namespaces in Paris bindings
: Reduce the use of namespaces in Paris bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz] (TPAC)
:
Mentors:
Depends on: 742170
Blocks: ParisBindings 740069 747827
  Show dependency treegraph
 
Reported: 2012-04-03 22:27 PDT by Boris Zbarsky [:bz] (TPAC)
Modified: 2012-05-04 02:29 PDT (History)
7 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (64.80 KB, patch)
2012-04-17 00:02 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
Switching dom and bindings (69.86 KB, patch)
2012-04-18 12:25 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
With FooBinding_workers instead of FooWorkers_binding (70.45 KB, patch)
2012-04-18 14:03 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
Merged to tip, still waiting on Peter's review before addressing Ben's comments (71.70 KB, patch)
2012-04-22 22:19 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
Updated to comments (74.45 KB, patch)
2012-05-02 12:05 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (TPAC) 2012-04-03 22:27:44 PDT
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 [:bz] (TPAC) 2012-04-17 00:02:54 PDT
Created attachment 615623 [details] [diff] [review]
Proposed patch

See long checkin comment for details
Comment 2 Peter Van der Beken [:peterv] 2012-04-18 07:27:50 PDT
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 [:bz] (TPAC) 2012-04-18 07:38:04 PDT
> 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 [:bz] (TPAC) 2012-04-18 09:26:56 PDT
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 [:bz] (TPAC) 2012-04-18 09:27:38 PDT
Also, mozilla/dom/Common.h is a bit odd...
Comment 6 Boris Zbarsky [:bz] (TPAC) 2012-04-18 09:39:08 PDT
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 [:bz] (TPAC) 2012-04-18 09:40:00 PDT
I suppose we could put Common.h in mozilla::bindings while Utils.h goes into mozilla::dom, too.
Comment 8 Boris Zbarsky [:bz] (TPAC) 2012-04-18 11:08:33 PDT
> 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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-18 11:48:02 PDT
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 [:bz] (TPAC) 2012-04-18 11:54:59 PDT
If we're OK with mozilla::bindings::FooBinding, then yes.
Comment 11 Boris Zbarsky [:bz] (TPAC) 2012-04-18 12:25:47 PDT
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?
Comment 12 Boris Zbarsky [:bz] (TPAC) 2012-04-18 14:03:34 PDT
Created attachment 616284 [details] [diff] [review]
With FooBinding_workers instead of FooWorkers_binding
Comment 13 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-18 14:50:25 PDT
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 [:bz] (TPAC) 2012-04-18 16:21:44 PDT
> 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 [:bz] (TPAC) 2012-04-22 22:19:44 PDT
Created attachment 617389 [details] [diff] [review]
Merged to tip, still waiting on Peter's review before addressing Ben's comments
Comment 16 Peter Van der Beken [:peterv] 2012-05-02 06:50:42 PDT
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 [:bz] (TPAC) 2012-05-02 07:31:46 PDT
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] 2012-05-02 07:33:22 PDT
r=me for the changes in comment 17.
Comment 19 Boris Zbarsky [:bz] (TPAC) 2012-05-02 11:07:55 PDT
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 [:bz] (TPAC) 2012-05-02 12:05:12 PDT
Created attachment 620417 [details] [diff] [review]
Updated to comments
Comment 21 Boris Zbarsky [:bz] (TPAC) 2012-05-02 22:24:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3576844511
Comment 22 Ed Morley [:emorley] 2012-05-04 02:29:22 PDT
https://hg.mozilla.org/mozilla-central/rev/3c3576844511

Note You need to log in before you can comment on or make changes to this bug.