Closed Bug 1435174 Opened 2 years ago Closed 2 years ago

Remove the renaming 'using namespace workers'

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

After the cleanup of the Worker headers, there is still some wrong namespace 'workers' usage.
Attached patch namespace.patch (obsolete) — Splinter Review
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.
Attachment #8947728 - Flags: review?(bkelly)
Depends on: 1432963
Blocks: 1435263
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?
Attachment #8947728 - Flags: review?(bkelly)
> Rather than churn the tree twice, what do you think about just making that
> change now?

Yeah, let move AssertIsOnMainThread() in mozilla:: namespace.
Attached patch namespace.patch (obsolete) — Splinter Review
Attachment #8947728 - Attachment is obsolete: true
Attachment #8947890 - Flags: review?(bkelly)
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?
Attachment #8947890 - Flags: review?(bkelly) → review-
Attached patch namespace.patchSplinter Review
Attachment #8947890 - Attachment is obsolete: true
Attachment #8948362 - Flags: review?(bkelly)
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.
Attachment #8948362 - Flags: review?(bkelly) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7818e2f5572c
Remove the renaming 'using namespace workers', r=bkelly
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/7818e2f5572c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.