695287 - Move Navigator declaration to Navigator.h and definition to Navigator.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

That was a bit annoying to see nsNavigator hang out with nsGlobalWindow :)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 (he/him; ⌚ UTC+1/+2)]

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

Flags are hard

Comment 3

[Mounir Lamouri (:mounir)]

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)]

Attachment: Non-exhaustive cleanup diff

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Close to the v1 patch with s/nsNavigator/Navigator/, namespace (mozilla::dom) and some cleanup (see the other diff).

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

https://hg.mozilla.org/mozilla-central/rev/51636ba3e69f