1435174 - Remove the renaming 'using namespace workers'

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

After the cleanup of the Worker headers, there is still some wrong namespace 'workers' usage.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: namespace.patch

Currently we have a workers namespace only for some static functions. Better to move them in a workerutils namespace. In this way, any remaining 'workers' namespace is a bug. Here a patch.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947728 [details] [diff] [review]
namespace.patch

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

::: dom/xhr/XMLHttpRequestWorker.cpp
@@ +160,5 @@
>  
>    void
>    Reset()
>    {
> +    workerutils::AssertIsOnMainThread();

I guess this is ok, but honestly not sure why we need this.  `MOZ_ASSERT(NS_IsMainThread())` is shorter.  Or we should just put an AssertIsOnMainThread in mozilla:: namespace somewhere.

Rather than churn the tree twice, what do you think about just making that change now?

Comment 3

[Andrea Marchesini [:baku]]

> Rather than churn the tree twice, what do you think about just making that
> change now?

Yeah, let move AssertIsOnMainThread() in mozilla:: namespace.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: namespace.patch

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947890 [details] [diff] [review]
namespace.patch

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

Thanks, that is better.

I guess I wasn't aware we were still using workers namespace for functions, though.  It would be really great if we could just stick the functions in the mozilla::dom:: namespace.  Most of them have "workers" in the function name somewhere anyway.  For things like "IsDebuggerSandbox()" maybe we could add worker to that function name instead of using a namespace.

Finally, if we have to keep a namespace, I guess maybe we should leave it as workers?  I don't think workeutils is really worth the churn unfortunately.

Sorry for my confusion about the namespace use for the functions.

::: dom/workers/WorkerCommon.h
@@ +17,5 @@
>  namespace dom {
>  
>  class WorkerPrivate;
>  
> +namespace workerutils {

Looking at most of these functions, they mention "workers" in their name somewhere.  Can we just avoid this namespace here?  Or only use it for the ones that don't have workers in their name?

::: dom/xhr/XMLHttpRequestWorker.cpp
@@ +1588,5 @@
>                                  const MozXMLHttpRequestParameters& aParams,
>                                  ErrorResult& aRv)
>  {
>    JSContext* cx = aGlobal.Context();
> +  WorkerPrivate* workerPrivate = workerutils::GetWorkerPrivateFromContext(cx);

Since this function has "WorkerPrivate" in it, can we just put it in mozilla::dom?

::: ipc/glue/IPCStreamSource.cpp
@@ +136,5 @@
>    // worker threads, but not other threads. Main-thread and PBackground thread
>    // do not need anything special in order to be kept alive.
>    WorkerPrivate* workerPrivate = nullptr;
>    if (!NS_IsMainThread()) {
> +    workerPrivate = mozilla::dom::workerutils::GetCurrentThreadWorkerPrivate();

Since this function has "WorkerPrivate" in it, can we just put it in mozilla::dom?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: namespace.patch

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8948362 [details] [diff] [review]
namespace.patch

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

Thanks!

::: dom/base/nsGlobalWindowInner.cpp
@@ +1267,5 @@
>  
>    mInnerObjectsFreed = true;
>  
>    // Kill all of the workers for this window.
> +  mozilla::dom::CancelWorkersForWindow(this);

This file is `using namespace mozilla::dom` so I think you can lose the `mozilla::dom::` prefixes here.

::: dom/events/EventDispatcher.cpp
@@ +918,5 @@
>  
>      if (!dontResetTrusted) {
>        //Check security state to determine if dispatcher is trusted
>        bool trusted = NS_IsMainThread() ? nsContentUtils::LegacyIsCallerChromeOrNativeCode()
> +                                       : mozilla::dom::IsCurrentThreadRunningChromeWorker();

This file is in mozilla namespace and has a `using namespace dom`.  I don't think you need the mozilla::dom:: here.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7818e2f5572c
Remove the renaming 'using namespace workers', r=bkelly

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7818e2f5572c