Closed Bug 950386 Opened 7 years ago Closed 7 years ago

Move mozilla::Selection to mozilla::dom::Selection

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: ehsan, Assigned: ayg)

Details

Attachments

(1 file)

Follow-up for bug 949445.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
It seems like unified builds make this annoying, because files that refer to just "Selection" incorrectly compile locally due to stealing a "using namespace mozilla::dom;" from a neighbor and then break on try.  I don't suppose there's any clever way to get unified builds to not do this, is there?
Oh, boy.  Arguably, with unified builds the same rules that apply to "using namespace" in headers should apply to .cpp files...

But anyway, --disable-unified-compilation locally might do what you want here?
(In reply to comment #2)
> Oh, boy.  Arguably, with unified builds the same rules that apply to "using
> namespace" in headers should apply to .cpp files...
> 
> But anyway, --disable-unified-compilation locally might do what you want here?

Well, we still need to make sure that the code builds in both configurations.
Attached patch patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=38db6ccc77b2

Will do an extra try before pushing to make sure it hasn't bitrotted.  I wasn't sure when to do "using" vs. explicitly invoking the namespace, so tell me if you want me to change it:

* In accessible/ and layout/ I didn't use "using namespace mozilla::dom;", but in one selection-heavy file I did "using mozilla::dom::Selection;".  Obviously in nsSelection.cpp itself I used "using namespace mozilla::dom;".
* In editor/ I used "using namespace mozilla::dom;" because of how heavily it uses DOM stuff.
* In editor/libeditor/html/nsWSRunObject.h I had to explicitly qualify ::DOMPoint for it to compile.  I suspect this is because I added "using namespace mozilla::dom;" to some file that got unified with a file including this one, so it became ambiguous in the unified file.  Has anyone noted that unified builds are a bit of a hack?
Attachment #8411018 - Flags: review?(ehsan)
(In reply to :Aryeh Gregor from comment #4)
> Has anyone noted that unified builds are a bit of a hack?

Yes, indeed (and they're mostly my fault.)  The trade-off is between these kinds of hacks and making everybody more productive when they're not fighting these kinds of issues by giving them 2x faster builds.

Also, note that the try push only builds in unified mode, in order to test non-unified compilation you need to modify http://mxr.mozilla.org/mozilla-central/source/build/mozconfig.common.override and add ac_add_options --disable-unified-compilation there.
Comment on attachment 8411018 [details] [diff] [review]
patch

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

::: dom/bindings/Bindings.conf
@@ +973,5 @@
>      'nativeType': 'nsScreen',
>  },
>  
>  'Selection': {
> +    'nativeType': 'mozilla::dom::Selection',

I think you can just drop this, this is the name that the WebIDL compiler picks by default.
Attachment #8411018 - Flags: review?(ehsan) → review+
Trying a non-unified compilation:

https://tbpl.mozilla.org/?tree=Try&rev=724388ac3070

And yes, I know unified compilation gives huge speed wins, and am totally in favor.  Just saying it's a hack.  :)  (I guess there's no way to magically scope "using" statements.)
(In reply to comment #7)
> And yes, I know unified compilation gives huge speed wins, and am totally in
> favor.  Just saying it's a hack.  :)  (I guess there's no way to magically
> scope "using" statements.)

Not that I know of, unfortunately!
https://hg.mozilla.org/mozilla-central/rev/be6f50f0f583
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.