Closed
Bug 1022456
Opened 10 years ago
Closed 10 years ago
Convert more of xpcom/ to Gecko style
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: poiru, Assigned: poiru)
References
Details
Attachments
(9 files, 1 obsolete file)
913.41 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
129.24 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
483.49 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
104.58 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
112.43 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
331.02 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
269.88 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
122.34 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
206.97 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
See bug 995730.
Assignee | ||
Comment 1•10 years ago
|
||
This is a folded patch of attachment 8433333 [details] [diff] [review], attachment 8433334 [details] [diff] [review], attachment 8436366 [details] [diff] [review], attachment 8436367 [details] [diff] [review], and attachment 8436368 [details] [diff] [review] from bug 995730 with review comments addressed.
Attachment #8443578 -
Flags: review+
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8443703 -
Flags: review?(nfroyd)
Comment 3•10 years ago
|
||
Comment on attachment 8443703 [details] [diff] [review] Brace one-line control statements in xpcom/glue/ Review of attachment 8443703 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/DeadlockDetector.h @@ +492,5 @@ > * > * @precondition |aStart != aTarget| > */ > + ResourceAcquisitionArray* GetDeductionChain(const PLHashEntry* aStart, > + const PLHashEntry* aTarget) Hunk in the wrong patch? ::: xpcom/glue/nsTextFormatter.cpp @@ -900,5 @@ > * the various sprintf() implementations are inconsistent > * on this feature. > */ > while ((c == '-') || (c == '+') || (c == ' ') || (c == '0')) { > - if (c == '-') flags |= _LEFT; Whoa, whoa, no separate patch for converting single-line statements to multi-line statements? ;) ::: xpcom/glue/nsVoidArray.cpp @@ +645,5 @@ > } > > bool > nsVoidArray::EnumerateForwards(nsVoidArrayEnumFuncConst aFunc, > + void* aData) const Hunk in the wrong patch?
Attachment #8443703 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8446094 -
Flags: review?(nfroyd)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8446095 -
Flags: review?(nfroyd)
Comment 6•10 years ago
|
||
Comment on attachment 8446095 [details] [diff] [review] Fix more style violations in xpcom/glue/ Review of attachment 8446095 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes below. ::: xpcom/glue/BlockingResourceBase.cpp @@ +96,5 @@ > > void > BlockingResourceBase::CheckAcquire(const CallStack& aCallContext) > { > + if (mDDEntry->mType == eCondVar) { FWIW, I don't know that we would consider these style violations. ::: xpcom/glue/EnumeratedArrayCycleCollection.h @@ +27,1 @@ > MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(IndexType) SizeAsEnumValue, If you're going to fix the spacing for the opening template argument, you need to fix the indentation of the remaining template arguments. ::: xpcom/glue/nsCycleCollectionNoteChild.h @@ +57,1 @@ > bool IsXPCOM = mozilla::IsBaseOf<nsISupports, T>::value> Likewise here on the indentation. ::: xpcom/glue/nsIInterfaceRequestorUtils.cpp @@ +12,5 @@ > nsresult status; > > if (mSource) { > + nsCOMPtr<nsIInterfaceRequestor> factoryPtr = > + do_QueryInterface(mSource, &status); Seems like we could drop the &status parameter (since we'll assign it regardless below) and then maybe we wouldn't have to wrap this line? ::: xpcom/glue/nsStringAPI.h @@ +1444,5 @@ > > inline const nsDependentSubstring > Substring(const char16_t* aStart, const char16_t* aEnd) > { > + NS_ABORT_IF_FALSE(uint32_t(aEnd - aStart) == uintptr_t(aEnd - aStart), Nit: trailing whitespace. ::: xpcom/glue/nsThreadUtils.h @@ +251,1 @@ > typename ReturnType = void, Indentation.
Attachment #8446095 -
Flags: review?(nfroyd) → review+
Updated•10 years ago
|
Attachment #8446094 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8446869 -
Flags: review?(nfroyd)
Assignee | ||
Comment 8•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=fca263070a0c https://hg.mozilla.org/integration/mozilla-inbound/rev/909655c3ec14
Keywords: leave-open
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/909655c3ec14
Updated•10 years ago
|
Attachment #8446869 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fdb8eb0faac
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8451413 -
Flags: review?(nfroyd)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8451414 -
Flags: review?(nfroyd)
Updated•10 years ago
|
Attachment #8451413 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Fixed mistake in previous patch.
Attachment #8451414 -
Attachment is obsolete: true
Attachment #8451414 -
Flags: review?(nfroyd)
Attachment #8451599 -
Flags: review?(nfroyd)
Comment 16•10 years ago
|
||
Comment on attachment 8451599 [details] [diff] [review] Fix style violations in xpcom/ds/ Review of attachment 8451599 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just a bunch of followup bugs to file...likely because this is old code. ::: xpcom/ds/StringBuilder.h @@ +73,5 @@ > free(mBuffer); > } > > private: > + char* mBuffer; Followup bug to make this a mozilla::ScopedFreePtr and remove the destructor? ::: xpcom/ds/nsCharSeparatedTokenizer.h @@ +189,5 @@ > + , mSeparatorChar(aSeparatorChar) > + , mWhitespaceBeforeFirstToken(false) > + , mWhitespaceAfterCurrentToken(false) > + , mSeparatorAfterCurrentToken(false) > + , mSeparatorOptional(aFlags & SEPARATOR_OPTIONAL) This is weird that we have duplicate classes like this; I wonder if anything keeps them from being templated appropriately. File a followup bug? ::: xpcom/ds/nsSupportsArray.cpp @@ +67,3 @@ > } > printf("Max Size of SupportsArray:\n"); > + for (i = 0; i < (int)(sizeof(MaxElements) / sizeof(MaxElements[0])); ++i) { Feel free to move the declaration into these loops and/or make it unsigned. ::: xpcom/ds/nsSupportsPrimitives.cpp @@ +426,3 @@ > NS_ASSERTION(_retval, "Bad pointer"); > > + char* result = (char*)nsMemory::Alloc(2 * sizeof(char)); Thank you! ::: xpcom/ds/nsVariant.cpp @@ +539,5 @@ > /***************************************************************************/ > // These expand into full public methods... > > +NUMERIC_CONVERSION_METHOD_NORMAL(VTYPE_INT8, uint8_t, Int8, (-127 - 1), 127) > +NUMERIC_CONVERSION_METHOD_NORMAL(VTYPE_INT16, int16_t, Int16, (-32767 - 1), 32767) Can you file a bug to make all of these use inttypes.h macros? @@ +1681,5 @@ > // For all the data getters we just forward to the static (and sharable) > // 'ConvertTo' functions. > > /* readonly attribute uint16_t dataType; */ > +NS_IMETHODIMP nsVariant::GetDataType(uint16_t* aDataType) All the NS_IMETHODIMP in this file should be on separate lines. If you're going to fix that up in another patch, that's fine. ::: xpcom/ds/nsWhitespaceTokenizer.h @@ +92,5 @@ > nsCWhitespaceTokenizerTemplate(const nsCSubstring& aSource) > + : mIter(aSource.Data(), aSource.Length()) > + , mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length()) > + , mWhitespaceBeforeFirstToken(false) > + , mWhitespaceAfterCurrentToken(false) Please file a followup bug for these, too, similar to the CharTokenizer.h bug.
Attachment #8451599 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8452686 -
Flags: review?(nfroyd)
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #16) > Comment on attachment 8451599 [details] [diff] [review] > Fix style violations in xpcom/ds/ > > Review of attachment 8451599 [details] [diff] [review]: > ----------------------------------------------------------------- > Thank you for the quick reviews!
Attachment #8452687 -
Flags: review?(nfroyd)
Updated•10 years ago
|
Attachment #8452686 -
Flags: review?(nfroyd) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8452687 [details] [diff] [review] Fix argument names in xpcom/ds/ Review of attachment 8452687 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the |aResult| change. ::: xpcom/ds/nsHashPropertyBag.cpp @@ +12,5 @@ > #include "nsVariant.h" > #include "mozilla/Attributes.h" > > nsresult > +NS_NewHashPropertyBag(nsIWritablePropertyBag** aRetVal) grep suggests that |aRetval| is slightly preferred, but |aResult| is more the norm. Please use |aResult| here and throughout. ::: xpcom/ds/nsPersistentProperties.cpp @@ +131,2 @@ > // of this block > + nsAString& aOldValue); // when duplicate property is found, new value Please fix the comment alignment here. @@ +223,5 @@ > // but don't allow another '\n' to be skipped > mMultiLineCanSkipN = false; > // Now there is nothing to append to the mValue since we are skipping > // whitespaces at the beginning of the new line of the multiline > + // property. Set aTokenStart properly to ensure that nothing is appended ++ for fixing the comments, too!
Attachment #8452687 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 20•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=799ad1b16ac7 https://hg.mozilla.org/integration/mozilla-inbound/rev/57038b3a8c97
Assignee | ||
Comment 22•10 years ago
|
||
Work will continue in bug 1046841.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•