1166583 - Move chromium's MakeTuple function into namespace 'base'

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Botond Ballo [:botond]]

Bug 1163328 introduces the class mozilla::Tuple and and the function mozilla::MakeTuple.

The chromium code we have in the tree has a MakeTuple function in the global namespace.

The two can interfere if MakeTuple is called without qualification and the call site in inside namespace mozilla, or mozilla::MakeTuple is found by ADL.

The attached patch resolves this by moving chromium's MakeTuple into the namespace 'base' (where other chromium things live), and adding explicit qualification to call sites where appropriate.

Comment 1

[Botond Ballo [:botond]]

Attachment: Move chromium's MakeTuple function into namespace 'base'

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

We could probably just replace Chromium's tuples with MFBT's throughout ipc/, given that we've already done something similar for the smart-pointer classes (bug 1063318).  It might be worth doing as a followup to this bug.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8607896 [details] [diff] [review]
Move chromium's MakeTuple function into namespace 'base'

At Benjamin's request, asking Nathan to review instead.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8607896 [details] [diff] [review]
Move chromium's MakeTuple function into namespace 'base'

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

This makes that mythical future re-import of chromium code harder, but ideally, by the time we do that, they'll be using std::tuple anyway, so r+ except for the security/sandbox/ part.

::: security/sandbox/chromium/base/tuple.h
@@ +426,1 @@
>  inline Tuple0 MakeTuple() {

Aw, they haven't removed the tuple implementation from chromium/base/ yet?  Boo.

I'm unsure of the procedures for modifying the chromium import under security/sandbox/, so I'm going to bounce this bit to :bobowen.

Comment 5

[Bob Owen (:bobowen)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)

> I'm unsure of the procedures for modifying the chromium import under
> security/sandbox/, so I'm going to bounce this bit to :bobowen.

Thanks Nathan.
I'd rather we didn't change this unless we have to.
Every change gives it a little more pain when we roll the Chromium sandbox code forward (which I hope to do again soon).

Does this really cause conflicts?
The sandbox code is generally compiled into its own binary, although bits are included in xul for integration purposes.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

I agree with comment #5.  If we can make changes outside of security/sandbox/chromium instead (and they aren't truly awful) we should.  We're specifically trying to avoid having it attain the “mythical” status of ipc/chromium/src.

Comment 7

[Botond Ballo [:botond]]

(In reply to Bob Owen (:bobowen) from comment #5)
> Does this really cause conflicts?
> The sandbox code is generally compiled into its own binary, although bits
> are included in xul for integration purposes.

You're right, the MakeTuple function in the sandbox code doesn't cause conflicts, only the one in ipc/chromium/src. I'll update the patch to leave the sandbox code alone.

Comment 8

[Botond Ballo [:botond]]

Attachment: Move chromium's MakeTuple function into namespace 'base'

Updated patch to omit the changes to the sandbox code. Carrying r+ from Nathan.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b539c062b23

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b539c062b23