Last Comment Bug 695287 - Move Navigator declaration to Navigator.h and definition to Navigator.cpp
: Move Navigator declaration to Navigator.h and definition to Navigator.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 678694
  Show dependency treegraph
 
Reported: 2011-10-18 02:26 PDT by Mounir Lamouri (:mounir)
Modified: 2011-11-03 13:04 PDT (History)
4 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (57.73 KB, patch)
2011-10-18 02:26 PDT, Mounir Lamouri (:mounir)
Ms2ger: review+
Details | Diff | Review
Non-exhaustive cleanup diff (23.37 KB, patch)
2011-11-01 17:06 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Patch v2 (65.47 KB, patch)
2011-11-01 17:08 PDT, Mounir Lamouri (:mounir)
Ms2ger: review+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2011-10-18 02:26:31 PDT
Created attachment 567695 [details] [diff] [review]
Patch v1

That was a bit annoying to see nsNavigator hang out with nsGlobalWindow :)
Comment 1 :Ms2ger 2011-10-18 02:52:45 PDT
Comment on attachment 567695 [details] [diff] [review]
Patch v1

My first thought when I saw you opened this bug was "yes, thank you!".. And then I noticed the review request :(

>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -954,34 +954,32 @@ nsGlobalWindow::nsGlobalWindow(nsGlobalW
> /* static */
> void
> nsGlobalWindow::Init()
> {
>+  nsNavigator::Init();

I wouldn't put this here, but with the caller of nsGlobalWindow::Init (LayoutStatics?).

>--- /dev/null
>+++ b/dom/base/nsNavigator.cpp

>+nsNavigator::Init()
>+{
>+  mozilla::Preferences::AddBoolVarCache(&sDoNotTrackEnabled,

Just Preferences::...

>+                                        "privacy.donottrackheader.enabled",
>+                                        PR_FALSE);

false

>+nsNavigator::SetDocShell(nsIDocShell *aDocShell)
>+{
>+  if (mGeolocation)
>+  {
>+  if (mNotification)
>+  {

Make those if () {, while you're taking blame?

>+nsNavigator::GetLanguage(nsAString& aLanguage)

And a couple here too

>+NS_IMETHODIMP
>+nsNavigator::GetVendor(nsAString& aVendor)
>+{
>+  aVendor.Truncate();
>+  return NS_OK;
>+}
>+
>+

One newline. (I may have missed a few more; bonus points if you check.)

>+nsNavigator::GetMimeTypes(nsIDOMMimeTypeArray **aMimeTypes)
>+{
>+  if (!mMimeTypes) {
>+    mMimeTypes = new nsMimeTypeArray(this);
>+    if (!mMimeTypes) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }

Drop this check.

>+nsNavigator::GetPlugins(nsIDOMPluginArray **aPlugins)
>+{
>+  if (!mPlugins) {
>+    mPlugins = new nsPluginArray(this, mDocShell);
>+    if (!mPlugins) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }

Same

>+nsNavigator::JavaEnabled(bool *aReturn)
>+{
>+  if (!mMimeTypes) {
>+    mMimeTypes = new nsMimeTypeArray(this);
>+    if (!mMimeTypes)
>+      return NS_ERROR_OUT_OF_MEMORY;

.

>+  for (PRUint32 i = 0; i < count; i++) {
>+    nsresult rv;
>+    nsIDOMMimeType* type = mMimeTypes->GetItemAt(i, &rv);

Could you file a bug to check rv here and add a // TODO?

>+nsNavigator::LoadingNewDocument()
>+{
>+  if (mGeolocation)
>+  {
>+  if (mNotification)
>+  {

As above

>+NS_IMETHODIMP nsNavigator::GetMozNotification(nsIDOMDesktopNotificationCenter **aRetVal)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window(do_GetInterface(mDocShell));

nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(mDocShell);

>+nsNavigator::SizeOf() const
>+{
>+}
>+

Drop the newline.

>--- /dev/null
>+++ b/dom/base/nsNavigator.h

>+#ifndef nsNavigator_h__

"nsNavigator_h", please. Double underscores are reserved for the compiler.

r=me with those comments addressed, assuming it's a straight copy otherwise. (If you want to add {}s around single-line ifs and switch from Foo *foo to Foo* foo, please do, but you don't have to.)
Comment 2 :Ms2ger 2011-10-18 02:53:13 PDT
Comment on attachment 567695 [details] [diff] [review]
Patch v1

Flags are hard
Comment 3 Mounir Lamouri (:mounir) 2011-11-01 17:03:50 PDT
A new patch is coming with a lot of code cleaning, moving files to Navigator.{h,cpp} instead of nsNavigator.{h,cpp} and moving Navigator to mozilla::dom namespace.
Comment 4 Mounir Lamouri (:mounir) 2011-11-01 17:06:40 PDT
Created attachment 571191 [details] [diff] [review]
Non-exhaustive cleanup diff
Comment 5 Mounir Lamouri (:mounir) 2011-11-01 17:08:58 PDT
Created attachment 571196 [details] [diff] [review]
Patch v2

Close to the v1 patch with s/nsNavigator/Navigator/, namespace (mozilla::dom) and some cleanup (see the other diff).
Comment 6 :Ms2ger 2011-11-02 02:56:06 PDT
Comment on attachment 571196 [details] [diff] [review]
Patch v2

--- /dev/null
+++ b/dom/base/Navigator.cpp

+#include <mozilla/Preferences.h>
+#include <mozilla/Telemetry.h>

Just use "" for these. (Also elsewhere.)

+
+using namespace mozilla;
+using namespace mozilla::dom;

Wrap this file in

namespace mozilla {
namespace dom {

} // namespace dom
} // namespace mozilla

and drop the 'using'. (DOMCI_DATA and the NS_* stuff probably need to be outside.)

+Navigator::GetLanguage(nsAString& aLanguage)
+{
+  while (localeTokenizer.hasMoreTokens()) {
+      ::ToUpperCase(upper);

I don't think you need the '::'.

+    first = false;

:)

+NS_IMETHODIMP
+Navigator::RegisterContentHandler(const nsAString& aMIMEType,
+                                    const nsAString& aURI,
+                                    const nsAString& aTitle)

Reindent.

+{
+  nsCOMPtr<nsIWebContentHandlerRegistrar> registrar =
+    do_GetService(NS_WEBCONTENTHANDLERREGISTRAR_CONTRACTID);
+  if (registrar && mDocShell) {
+    nsCOMPtr<nsIDOMWindow> contentDOMWindow = do_GetInterface(mDocShell);
+    if (contentDOMWindow) {
+      return registrar->RegisterContentHandler(aMIMEType, aURI, aTitle,
+                                               contentDOMWindow);
+    }
+  }

I'd use early returns here.

+Navigator::RegisterProtocolHandler(const nsAString& aProtocol,

Same two comments.

+Navigator::MozIsLocallyAvailable(const nsAString &aURI,
+                                   bool aWhenOffline,
+                                   bool* aIsAvailable)

Reindent.

+{
+  // if the cache is busy, assume that it is not yet available rather

'If'.

+NS_IMETHODIMP Navigator::GetMozNotification(nsIDOMDesktopNotificationCenter** aRetVal)
+{
+  mNotification = new nsDesktopNotificationCenter(window->GetCurrentInnerWindow(),
+                                                  scx);
+  if (!mNotification) {
+    return NS_ERROR_FAILURE;
+  }

Can go too.

diff --git a/dom/base/Navigator.h b/dom/base/Navigator.h
new file mode 100644
--- /dev/null
+++ b/dom/base/Navigator.h
+#ifndef mozilla_dom_Navigator_h__
+#define mozilla_dom_Navigator_h__

I still don't want those trailing underscores :)

--- a/dom/base/nsDOMClassInfo.cpp
+++ b/dom/base/nsDOMClassInfo.cpp
-  nsNavigator *nav = (nsNavigator *)safeNav.get();
+  Navigator *nav = (Navigator *)safeNav.get();

Make this a static_cast while you're here?

--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -3004,17 +3000,17 @@ nsGlobalWindow::GetSelf(nsIDOMWindow** a
 NS_IMETHODIMP
 nsGlobalWindow::GetNavigator(nsIDOMNavigator** aNavigator)
 {
-    mNavigator = new nsNavigator(mDocShell);
+    mNavigator = new Navigator(mDocShell);
     if (!mNavigator) {
       return NS_ERROR_OUT_OF_MEMORY;
     }

Remove the null-check, please.

--- a/dom/base/nsGlobalWindow.h
+++ b/dom/base/nsGlobalWindow.h
+namespace mozilla {
+namespace dom {
+class Navigator;
+} // dom
+} // mozilla

Usual style is '// namespace foo', please fix throughout the patch.

--- a/dom/base/nsPluginArray.cpp
+++ b/dom/base/nsPluginArray.cpp
-nsPluginArray::nsPluginArray(nsNavigator* navigator,
+nsPluginArray::nsPluginArray(mozilla::dom::Navigator* navigator,

Please use

using namespace mozilla;
using namespace mozilla::dom;

here. (And in general in all not-yet-namespaced .cpp files in content/ and dom/.)

r=me with those comments addressed.
Comment 7 Ed Morley [:emorley] 2011-11-03 13:04:44 PDT
https://hg.mozilla.org/mozilla-central/rev/51636ba3e69f

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