Closed Bug 1046841 Opened 5 years ago Closed 5 years ago

Convert rest of xpcom/ to Gecko style

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: poiru, Assigned: poiru)

Details

Attachments

(14 files, 1 obsolete file)

132.29 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
92.51 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
54.69 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
57.51 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
105.70 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
173.79 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
102.72 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
15.01 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
123.74 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
39.11 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
31.32 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
69.41 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
38.14 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
12.91 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Comment on attachment 8465547 [details] [diff] [review]
Fix indentation in xpcom/components/

Review of attachment 8465547 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/components/nsComponentManager.cpp
@@ +427,5 @@
> +  if (aModule->mCategoryEntries) {
> +    const mozilla::Module::CategoryEntry* entry;
> +    for (entry = aModule->mCategoryEntries; entry->category; ++entry)
> +      nsCategoryManager::GetSingleton()->
> +      AddCategoryEntry(entry->category,

This isn't the correct indentation.  Maybe introduce a temporary variable for GetSingleton here and hoist it out of the loop?
Attachment #8465547 - Flags: review?(nfroyd) → review+
Attachment #8465548 - Flags: review?(nfroyd) → review+
Comment on attachment 8465548 [details] [diff] [review]
Fix style violations in xpcom/components/

Review of attachment 8465548 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/components/ManifestParser.cpp
@@ +300,2 @@
>      if (testdata.Equals(aValue))
>        aResult = comparison ? eOK : eBad;

Actually, forgot to ask: are you fixing up bracing in all these files, too?  Because there are...well, a lot of places that need fixed.
Attachment #8465549 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #5)
> Comment on attachment 8465548 [details] [diff] [review]
> Fix style violations in xpcom/components/
> 
> Review of attachment 8465548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/components/ManifestParser.cpp
> @@ +300,2 @@
> >      if (testdata.Equals(aValue))
> >        aResult = comparison ? eOK : eBad;
> 
> Actually, forgot to ask: are you fixing up bracing in all these files, too? 
> Because there are...well, a lot of places that need fixed.

Yup, here you go.
Attachment #8466262 - Flags: review?(nfroyd)
Comment on attachment 8466262 [details] [diff] [review]
Brace one-line control statements in xpcom/components/

Review of attachment 8466262 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thank you!
Attachment #8466262 - Flags: review?(nfroyd) → review+
(In reply to Birunthan Mohanathas [:poiru] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ce357b802532
> 
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=12c029f7de77

Sorry, I had to back this out due to having some major merge conflicts with bug 977026 that I wasn't comfortable trying to rebase around on the fly :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/b31b3ee2b42f
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> (In reply to Birunthan Mohanathas [:poiru] from comment #8)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/ce357b802532
> > 
> > Try push: https://tbpl.mozilla.org/?tree=Try&rev=12c029f7de77
> 
> Sorry, I had to back this out due to having some major merge conflicts with
> bug 977026 that I wasn't comfortable trying to rebase around on the fly :(

No worries! Rebased and relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5a957b1c510

Try push: https://tbpl.mozilla.org/?tree=Try&rev=259ccfc19547
Attached patch Fix indentation in xpcom/build/ (obsolete) — Splinter Review
Attachment #8471684 - Flags: review?(nfroyd)
Attachment #8471684 - Attachment is obsolete: true
Attachment #8471684 - Flags: review?(nfroyd)
Attachment #8471690 - Flags: review?(nfroyd)
Attachment #8471690 - Flags: review?(nfroyd) → review+
Attachment #8471869 - Flags: review?(nfroyd) → review+
Comment on attachment 8471867 [details] [diff] [review]
Fix style violations in xpcom/build/

Review of attachment 8471867 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/build/IOInterposer.cpp
@@ +224,5 @@
>        newLists->mStageObservers.push_back(aObserver);
>      }
>      mObserverLists = newLists;
> +    mObservedOperations = (IOInterposeObserver::Operation)(
> +      mObservedOperations | aOp);

I think this looks less unusual if formatted like:

    mObservedOperations =
      (IOInterposeObserver::Operation)(mObservedOperations | aOp);

@@ +245,5 @@
>      if (aOp & IOInterposeObserver::OpCreateOrOpen) {
>        VectorRemove(newLists->mCreateObservers, aObserver);
>        if (newLists->mCreateObservers.empty()) {
> +        mObservedOperations = (IOInterposeObserver::Operation)(
> +          mObservedOperations & ~IOInterposeObserver::OpCreateOrOpen);

...and similarly for this and all similar operations below, which can be done like so:

        mObservedOperations =
          (IOInterposeObserver::Operation)(mObservedOperations &
                                           ~IOInterposeObserver::OpRead);

::: xpcom/build/perfprobe.cpp
@@ +197,4 @@
>    }
>    ULONG result =
>      RegisterTraceGuids(&ControlCallback
> +                       /*RequestAddress: Sets mSessions appropriately.*/,

This is such bizarre formatting.
Attachment #8471867 - Flags: review?(nfroyd) → review+
Attachment #8471868 - Flags: review?(nfroyd) → review+
Attachment #8478068 - Flags: review?(nfroyd)
Attachment #8478070 - Flags: review?(nfroyd)
Attachment #8478071 - Flags: review?(nfroyd)
Attachment #8478072 - Flags: review?(nfroyd)
Comment on attachment 8478068 [details] [diff] [review]
Fix comment style in nsCOMPtr.h

Review of attachment 8478068 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsCOMPtr.h
@@ +115,5 @@
>  
>  } // namespace mozilla
>  
> +/**
> + * Cooperates with nsCOMPtr to allow you to assign in a pointer _without_

I think it's worth saying "already_AddRefed cooperates with..." for clarity.

@@ +333,5 @@
>  do_QueryInterface(already_AddRefed<T>&)
>  {
>    // This signature exists solely to _stop_ you from doing the bad thing.
> +  // Saying |do_QueryInterface()| on a pointer that is not otherwise owned by
> +  // someone else is an automatic leak. See <http://bugzilla.mozilla.org/show_bug.cgi?id=8221>.

Optional: just 'bug 8221' here would be sufficient.

@@ +342,5 @@
>  do_QueryInterface(already_AddRefed<T>&, nsresult*)
>  {
>    // This signature exists solely to _stop_ you from doing the bad thing.
> +  // Saying |do_QueryInterface()| on a pointer that is not otherwise owned by
> +  // someone else is an automatic leak. See <http://bugzilla.mozilla.org/show_bug.cgi?id=8221>.

Likewise.

@@ +409,5 @@
> +/**
> + * Factors implementation for all template versions of nsCOMPtr.
> + *
> + * This should really be an |nsCOMPtr<nsISupports>|, but this wouldn't work
> + * because unlike the

I don't think this bit adds anything; it should be deleted.

@@ +411,5 @@
> + *
> + * This should really be an |nsCOMPtr<nsISupports>|, but this wouldn't work
> + * because unlike the
> + *
> + * Here's the way people normally do things like this

Nit: ':' at the end of this sentence.

@@ +1354,5 @@
>    nsCOMPtr<nsISupports>& mTargetSmartPtr;
>  };
>  
> +// Used around a |nsCOMPtr| when
> +// ...makes the class |nsGetterAddRefs<T>| invisible.

This comment makes no sense; I'd just delete it.
Attachment #8478068 - Flags: review?(nfroyd) → review+
Attachment #8478070 - Flags: review?(nfroyd) → review+
Attachment #8478071 - Flags: review?(nfroyd) → review+
Attachment #8478072 - Flags: review?(nfroyd) → review+
Attachment #8478069 - Flags: review?(nfroyd) → review+
Comment on attachment 8478067 [details] [diff] [review]
Fix more style violations in xpcom/**.cpp

Review of attachment 8478067 [details] [diff] [review]:
-----------------------------------------------------------------

Please fold these patches as appropriate prior to landing.

::: xpcom/io/nsLocalFileWin.cpp
@@ +566,5 @@
>  // of NSPR so that fields of |PRFilePrivate| #ifdef'd by them are not copied.
>  // Similarly, |_MDFileDesc| is taken from nsprpub/pr/include/md/_win95.h.
>  // In an unlikely case we switch to 'NT'-dependent NSPR AND this temporary
>  // workaround last beyond the switch, |PRFilePrivate| and |_MDFileDesc|
>  // need to be changed to match the definitions for WinNT.

Maybe someday some brave soul will investigate whether we still need all this garbage.

::: xpcom/threads/ThreadStackHelper.cpp
@@ +362,5 @@
>    virtual ~ThreadContext() {}
>  
> +  virtual uint64_t GetBase() const { return uint64_t(mStackBase); }
> +  virtual uint32_t GetSize() const { return mStackSize; }
> +  virtual bool GetMemoryAtAddress(uint64_t address, uint8_t*  value) const

Nit: extra space before |value|.

@@ +368,3 @@
>      return GetMemoryAtAddressInternal(address, value);
>    }
> +  virtual bool GetMemoryAtAddress(uint64_t address, uint16_t*  value) const

Here too.

@@ +372,3 @@
>      return GetMemoryAtAddressInternal(address, value);
>    }
> +  virtual bool GetMemoryAtAddress(uint64_t address, uint32_t*  value) const

Here too.

@@ +376,3 @@
>      return GetMemoryAtAddressInternal(address, value);
>    }
> +  virtual bool GetMemoryAtAddress(uint64_t address, uint64_t*  value) const

Here too.
Attachment #8478067 - Flags: review?(nfroyd) → review+
Closing this. I'll file separate bugs for the remaining bits (mainly tests and macros).
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.