Last Comment Bug 710793 - Move hal's private methods outside of Hal.h
: Move hal's private methods outside of Hal.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 710748
Blocks: 709584
  Show dependency treegraph
 
Reported: 2011-12-14 10:44 PST by Mounir Lamouri (:mounir)
Modified: 2011-12-18 15:38 PST (History)
3 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.97 KB, patch)
2011-12-14 10:44 PST, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
cjones.bugs: superreview+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-12-14 10:44:37 PST
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.
Comment 1 Justin Lebar (not reading bugmail) 2011-12-14 12:26:56 PST
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) 2011-12-14 13:36:42 PST
(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) 2011-12-14 13:50:15 PST
sgtm.
Comment 4 Matt Brubeck (:mbrubeck) 2011-12-18 15:38:25 PST
https://hg.mozilla.org/mozilla-central/rev/e04774d1a06f

Note You need to log in before you can comment on or make changes to this bug.