710793 - Move hal's private methods outside of Hal.h

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I would like to remove HAL_LOG from Hal.h too but I guess we could do that later.

Comment 1

[Justin Lebar (not reading bugmail)]

Comment on attachment 581711 [details] [diff] [review]
Patch v1

Mostly nits on the comments.  Sorry to be pedantic.

>diff --git a/hal/Hal.h b/hal/Hal.h
>--- a/hal/Hal.h
>+++ b/hal/Hal.h
>+ * Hal.h contains the public Hal API.

Nit: Blank line separating paragraphs.

>+ * Hal.h can be included from HalImpl.h, HalSandbox.h or directly.

Nit: Blank line, and comma before 'or' [1].  But I'd get rid of this line (see below).

>+ * When included directly, the methods are going to be available from the hal
>+ * namespace.  When included from HalImpl.h, the methods will be available from
>+ * hal_impl namespace. From HalSandbox.h, it will be hal_sandbox namespace.
>+ * Files including Hal.h are using MOZ_HAL_NAMESPACE to specify the namespace
>+ * they want to have the methods added to.
>+ */

The English here is confusing, mainly because of the passive voice.  If you
change it to active, it might become:

  By default, this file defines its functions in the hal namespace, but if
  MOZ_HAL_NAMESPACE is defined, we'll define our functions in that namespace.

  This is used by HalImpl.h and HalSandbox.h, which define copies of all the
  functions here in the hal_impl and hal_sandbox namespaces.

>+extern PRLogModuleInfo *sHalLog;
>+#define HAL_LOG(msg) PR_LOG(sHalLog, PR_LOG_DEBUG, msg)

You could move this to HalInternal.h, right?  So long as that's included by Hal.cpp?

>diff --git a/hal/HalImpl.h b/hal/HalImpl.h
>--- a/hal/HalImpl.h
>+++ b/hal/HalImpl.h
>@@ -32,17 +32,21 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-#ifndef mozilla_Hal_h
>-# error "This is an internal file, don't include it"
>-#endif
>+#ifndef mozilla_HalImpl_h
>+#define mozilla_HalImpl_h
> 
>+#define MOZ_HAL_NAMESPACE hal_impl
> #undef mozilla_Hal_h
>-#define MOZ_HAL_NAMESPACE hal_impl
>+#undef mozilla_HalInternal_h
> #include "Hal.h"
>+#include "HalInternal.h"
>+#define mozilla_Hal_h 1
>+#define mozilla_HalInternal_h 1
> #undef MOZ_HAL_NAMESPACE

Why do you define mozilla_Hal_h and mozilla_HalInternal_h after you include them?

>diff --git a/hal/HalInternal.h b/hal/HalInternal.h
>new file mode 100644
>--- /dev/null
>+++ b/hal/HalInternal.h

>+#ifndef mozilla_HalInternal_h
>+#define mozilla_HalInternal_h 1
>+
>+/*
>+ * This file is included by HalImpl.h and HalSandbox.h with a mechanism similar
>+ * to Hal.h. That means those header set MOZ_HAL_NAMESPACE to specify on which
>+ * namespace the internal methods should appear.

s/header/headers
s/on which/in which
s/methods/functions

Blank line between paragraphs, please.

>+ * The difference between Hal.h and HalInternal.h is that methods declared in
>+ * HalInternal.h can't appear in the hal namespace. That also means this file
>+ * should not be included directly.

s/can't/don't
s/directly/except by HalInternal.h and HalSandbox.h

>diff --git a/hal/HalSandbox.h b/hal/HalSandbox.h
>--- a/hal/HalSandbox.h
>+++ b/hal/HalSandbox.h
>@@ -32,17 +32,21 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-#ifndef mozilla_Hal_h
>-# error "This is an internal file, don't include it"
>-#endif
>+#ifndef mozilla_hal_HalSandbox_h
>+#define mozilla_hal_HalSandbox_h
> 
>+#define MOZ_HAL_NAMESPACE hal_sandbox
> #undef mozilla_Hal_h
>-#define MOZ_HAL_NAMESPACE hal_sandbox
>+#undef mozilla_HalInternal_h
> #include "Hal.h"
>-#include "mozilla/hal_sandbox/PHal.h"
>+#include "HalInternal.h"
>+#define mozilla_Hal_h 1
>+#define mozilla_HalInternal_h 1
> #undef MOZ_HAL_NAMESPACE
>+
>+#endif // mozilla_hal_HalSandbox_h

Again, I don't understand why we have to define mozilla_hal_H and mozilla_HalInternal_h here.

[1] http://thegloss.com/beauty/why-the-oxford-comma-is-something-you-should-care-about-392/

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #1)
> >+extern PRLogModuleInfo *sHalLog;
> >+#define HAL_LOG(msg) PR_LOG(sHalLog, PR_LOG_DEBUG, msg)
> 
> You could move this to HalInternal.h, right?  So long as that's included by
> Hal.cpp?

So I thought and even did that before reverting the change. moving this to HalInternal.h requires including HalImpl.h or HalSandbox.h to be able to call HAL_LOG. Though, HAL_LOG should be usable by files that doesn't include such headers. For example, SandboxHal.cpp.
I think we could have a Utils.h file for that and maybe other stuff but I don't mind too much if HAL_LOG is in Hal.h for the moment.

Comment 3

[Justin Lebar (not reading bugmail)]

sgtm.

Comment 4

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e04774d1a06f