Move hal's private methods outside of Hal.h

RESOLVED FIXED in mozilla11

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

9.97 KB, patch
Justin Lebar (not reading bugmail)
: review+
cjones
: superreview+
mounir
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 581711 [details] [diff] [review]
Patch v1

I would like to remove HAL_LOG from Hal.h too but I guess we could do that later.
Attachment #581711 - Flags: superreview?(jones.chris.g)
Attachment #581711 - Flags: review?(justin.lebar+bug)
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/
Attachment #581711 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 2

5 years ago
(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.
sgtm.
(Assignee)

Updated

5 years ago
No longer blocks: 710748
Depends on: 710748
(Assignee)

Updated

5 years ago
Blocks: 709584
Attachment #581711 - Flags: superreview?(jones.chris.g) → superreview+
(Assignee)

Updated

5 years ago
Attachment #581711 - Flags: checkin+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/e04774d1a06f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.