Closed Bug 1130096 Opened 5 years ago Closed 5 years ago

Convert embedding/ to Gecko style

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(14 files, 1 obsolete file)

39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
39 bytes, text/x-review-board-request
mccr8
: review+
Details
Attached file MozReview Request: bz://1130096/poiru (obsolete) —
/r/8209 - Bug 1130096 - Fix indentation in embedding/browser/
/r/8211 - Bug 1130096 - Fix style in embedding/browser/
/r/8213 - Bug 1130096 - Fix argument names in embedding/browser/
/r/8215 - Bug 1130096 - Fix indentation in embedding/components/commandhandler/
/r/8217 - Bug 1130096 - Fix style in embedding/components/commandhandler/
/r/8219 - Bug 1130096 - Fix argument names in embedding/components/commandhandler/
/r/8221 - Bug 1130096 - Fix indentation in embedding/components/find/
/r/8223 - Bug 1130096 - Fix style in embedding/components/find/
/r/8225 - Bug 1130096 - Fix argument names in embedding/components/find/
/r/8227 - Bug 1130096 - Fix indentation in embedding/components/webbrowserpersist/
/r/8229 - Bug 1130096 - Fix style and argument names in embedding/components/webbrowserpersist/
/r/8231 - Bug 1130096 - Fix indentation in embedding/components/windowwatcher/
/r/8233 - Bug 1130096 - Fix style in embedding/components/windowwatcher/
/r/8235 - Bug 1130096 - Fix argument names in embedding/components/windowwatcher/

Pull down these commits:

hg pull -r a913642e1c05305e3c4fb679ea7af917172f7c3a https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602166 - Flags: review?(continuation)
Benjamin, I think you own this code.  Are you okay with me reviewing these style changes?  I mean, feel free to do them yourself if you want. ;)
Flags: needinfo?(benjamin)
Go ahead. I only own this for lack of a real owner.
Flags: needinfo?(benjamin)
https://reviewboard.mozilla.org/r/8209/#review6977

::: embedding/browser/nsContextMenuInfo.h:58
(Diff revision 1)
>    

This bit of trailing whitespace managed to escape.
https://reviewboard.mozilla.org/r/8211/#review6981

::: embedding/browser/nsCTooltipTextProvider.h:6
(Diff revision 1)
>  #ifndef NSCTOOLTIPTEXTPROVIDER_H

I wonder how much of this code we can just delete...

::: embedding/browser/nsDocShellTreeOwner.h:160
(Diff revision 1)
> -    kTooltipAutoHideTime = 5000,       // 5000ms = 5 seconds
> +  {

Maybe just change this comment to "in ms"...

::: embedding/browser/nsDocShellTreeOwner.cpp:240
(Diff revision 1)
> -                                                   nsIDocShellTreeItem* aOriginalRequestor,
> +    nsIDocShellTreeItem* aRequestor, nsIDocShellTreeItem* aOriginalRequestor,

put aRequestor and aOriginalRequestor on separate lines maybe?

::: embedding/browser/nsDocShellTreeOwner.cpp:1427
(Diff revision 1)
> -//
> +      nsWebBrowser* inBrowser,

The indentation of the arguments to the ChromeContextMenuListener ctor looks odd to me, but maybe it is right.

::: embedding/browser/nsWebBrowser.h:134
(Diff revision 1)
> -  nsCOMPtr<nsITextScroll>    mDocShellAsTextScroll;
> +  nsCOMPtr<nsITextScroll> mDocShellAsTextScroll;

Maybe add some blank lines to separate these various pointers to the docshell (mDocShell* fields) from the rest.

::: embedding/browser/nsWebBrowser.cpp:593
(Diff revision 1)
> -                                              bool aRecurse, bool aSameType,
> +nsWebBrowser::FindChildWithName(const char16_t* aName, bool aRecurse,

Seems a little odd for aName and aRecurse to be on the same line.
https://reviewboard.mozilla.org/r/8215/#review6999

::: embedding/components/commandhandler/nsBaseCommandController.h:22
(Diff revision 1)
>  //   and all other text and html editing

Maybe unindent the comment on this line.

::: embedding/components/commandhandler/nsCommandGroup.cpp:38
(Diff revision 1)
>    

This bit of trailing whitespace didn't get fixed.
https://reviewboard.mozilla.org/r/8217/#review7005

::: embedding/components/commandhandler/nsBaseCommandController.cpp:39
(Diff revision 1)
> -    mCommandTable = aCommandTable;    // owning addref
> +    mCommandTable = aCommandTable; // owning addref

The comment here could be removed.

::: embedding/components/commandhandler/nsCommandGroup.h:29
(Diff revision 1)
> -  typedef nsClassHashtable<nsCStringHashKey, nsTArray<nsCString>> GroupsHashtable;
> +  typedef nsClassHashtable<nsCStringHashKey, nsTArray<nsCString> >

I think >> to close nested templates is the preferred style now that all compilers we use support this.

::: embedding/components/commandhandler/nsCommandGroup.h:39
(Diff revision 1)
> -                               // This could be made more space-efficient, maybe with atoms
> +  // maybe with atoms

Add a . to end the second sentence please.

::: embedding/components/commandhandler/nsCommandGroup.cpp:125
(Diff revision 1)
> -  mGroupNames = new char*[mHashTable.Count()];
> +  mGroupNames = new char* [mHashTable.Count()];

No space between the * and the [ please. Every instance of this I found in the tree with new char* has no space (so |new char*[mHash|| etc.).

::: embedding/components/commandhandler/nsCommandGroup.cpp:228
(Diff revision 1)
>  #if 0

You could remove these #if 0 #pragma blocks if you want.

::: embedding/components/commandhandler/nsCommandGroup.cpp:238
(Diff revision 1)
> -  if ((commandList = mGroupsHash.Get(groupKey)) == nullptr)
> +  if ((commandList = mGroupsHash.Get(groupKey)) == nullptr) {

While you are here, please move the assignment up to the declaration, rather than having this weird assignment inside the if (which is fine in some cases, but seems odd here).

::: embedding/components/commandhandler/nsCommandGroup.cpp:245
(Diff revision 1)
> -  nsCString *appended =
> +  nsCString* appended =

This could be DebugOnly<nsCString> I think but feel free to leave it alone.

::: embedding/components/commandhandler/nsCommandManager.h:30
(Diff revision 1)
>    // nsISupports

Feel free to remove these // nsIFoo comments too if you'd like.

::: embedding/components/commandhandler/nsCommandParams.h:74
(Diff revision 1)
> -          mISupports = aRHS.mISupports.get();    // additional addref
> +          mISupports = aRHS.mISupports.get(); // additional addref

This comment can be removed.

::: embedding/components/commandhandler/nsCommandParams.h:102
(Diff revision 1)
> -          mISupports = nullptr;   // clear the nsCOMPtr
> +          mISupports = nullptr; // clear the nsCOMPtr

This comment can be removed.

::: embedding/components/commandhandler/nsCommandParams.cpp:211
(Diff revision 1)
>    // PL_DHashTableRemove doesn't tell us if the entry was really removed, so we

PL_DHashTableRemove doesn't return anything any more, so you can remove this whole comment and the (void).

::: embedding/components/commandhandler/nsCommandParams.cpp:269
(Diff revision 1)
> -  fromEntry->~HashEntry();      // call dtor explicitly
> +  fromEntry->~HashEntry(); // call dtor explicitly

I think you can just delete this comment.

::: embedding/components/commandhandler/nsCommandParams.cpp:276
(Diff revision 1)
> -  thisEntry->~HashEntry();      // call dtor explicitly
> +  thisEntry->~HashEntry(); // call dtor explicitly

I think you can just delete this comment.

::: embedding/components/commandhandler/nsControllerCommandTable.cpp:94
(Diff revision 1)
>  #if DEBUG

I think the #if DEBUG can be removed here. Also the trailing -- in the WARNING is weird, plus you can delete the "we don't handle this command" comment because that's redundant with the WARNING. Same thing at least one other place below.

::: embedding/components/commandhandler/nsControllerCommandTable.cpp:231
(Diff revision 1)
> -  if (! newCommandTable)
> +  if (!newCommandTable) {

This null check can be deleted if you feel like it.
In IRC, shu pointed out that the comments "// find the command" before calling a function called FindCommandHandler are pretty terrible, in nsControllerCommandTable.cpp, so feel free to remove those if you'd like.
https://reviewboard.mozilla.org/r/8223/#review7011

::: embedding/components/find/nsFind.cpp:676
(Diff revision 1)
> -  while (1)
> +  while (1) {

Change this to |while (true)| while you are here maybe?

::: embedding/components/find/nsFind.cpp:1041
(Diff revision 1)
> -    if (mIterNode == endNode &&
> +    if (mIterNode == endNode && ((mFindBackward && (findex < endOffset)) ||

Maybe leave the things split across three lines?  This condition is pretty terrifying, and I think it is clearer if it is split across 3 lines instead of 2.

::: embedding/components/find/nsWebBrowserFind.cpp:157
(Diff revision 1)
> -    if (NS_FAILED(rv)) break;
> +    if (NS_FAILED(rv)) {

I think these single lines for NS_FAILED checking boilerplate are acceptable, but I guess it doesn't matter much.

::: embedding/components/find/nsWebBrowserFind.cpp:731
(Diff revision 1)
> -  (void) find->SetWordBreaker(0);
> +  (void)find->SetWordBreaker(0);

This could be nullptr instead of 0.
https://reviewboard.mozilla.org/r/8227/#review7017

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:348
(Diff revision 1)
>      nsIInputStream *aPostData, const char *aExtraHeaders, 

trailing space

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:565
(Diff revision 1)
> -                nsIWebProgressListener::STATE_START | addToStateFlags, NS_OK);
> +          nsIWebProgressListener::STATE_START | addToStateFlags, NS_OK);

2 indent this?  Hmm, there are a few other places like this in the file, so maybe it gets fixed in another patch?

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:1614
(Diff revision 1)
> -                nsIDOMNodeFilter::SHOW_DOCUMENT |
> +            nsIDOMNodeFilter::SHOW_DOCUMENT |

Something went wrong with the indentation for this line and the next.
https://reviewboard.mozilla.org/r/8229/#review7023

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:270
(Diff revision 1)
> -NS_IMETHODIMP nsWebBrowserPersist::SetPersistFlags(uint32_t aPersistFlags)
> +NS_IMETHODIMP

Insert a blank line before this one?

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:599
(Diff revision 1)
> -  return (mURIMap.Count()
> +  return (mURIMap.Count() || mUploadList.Count() || mDocList.Length() ||

No need for the outermost parens.

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:879
(Diff revision 1)
> -  // Store the progress of this request
> +  // Store the progress of this aRequest

aRequest in comment should remain as "request"

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp:1884
(Diff revision 1)
> -    while (1)
> +    while (1) {

while (true)
https://reviewboard.mozilla.org/r/8233/#review7115

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:133
(Diff revision 1)
>  

This blank line could go.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:199
(Diff revision 1)
> -  *retval = mCurrentPosition? true : false;
> +  *retval = mCurrentPosition ? true : false;

Maybe !!mCurrentPosition instead of this weird trinary operator?

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:458
(Diff revision 1)
> -  bool                            nameSpecified,
> +  bool nameSpecified;

Maybe move the declarations of nameSpecified and featuresSpecified down to where they are first initialized, so that this looks less scary.  The rest of them look kind of weird, so leaving them here is fine.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:770
(Diff revision 1)
> -      }
> +      } else

The body of this else should get braced.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1317
(Diff revision 1)
> -  uint32_t  ctr,
> +  uint32_t ctr, count = mEnumeratorList.Length();

The declaration of ctr could get moved into the for loop. At least, it should be split out from the declaration of count.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1485
(Diff revision 1)
> -    if(aDialog)
> +    if (aDialog)

This if block needs to be braced.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1571
(Diff revision 1)
> -                 ? nsIWebBrowserChrome::CHROME_WINDOW_POPUP : 0; 
> +    nsIWebBrowserChrome::CHROME_WINDOW_POPUP : 0; 

Please remove the trailing whitespace.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1978
(Diff revision 1)
> -  int32_t left = 0,
> +  int32_t left = 0, top = 0, width = 100, height = 100;

Should these be split into separate declarations? If you have it like this intentionally (because they aren't pointers or whatever) that's fine.
https://reviewboard.mozilla.org/r/8235/#review7117

::: embedding/components/windowwatcher/nsWindowWatcher.h:63
(Diff revision 1)
> -  nsresult RemoveWindow(nsWatcherWindowEntry* inInfo);
> +  nsresult RemoveWindow(nsWatcherWindowEntry* afo);

|afo| should probably be |aInfo|

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:250
(Diff revision 1)
> -  if (mCurrentPosition == inInfo)
> +  if (mCurrentPosition == aInfo)

In a previous patch, the blank line should get deleted, and the if block needs to be braced.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp:1165
(Diff revision 1)
> -  mWindowCreator = creator; // it's an nsCOMPtr, so this is an ownership ref
> +  mWindowCreator = aCreator; // it's an nsCOMPtr, so this is an ownership ref

You can delete that comment if you want.
Comment on attachment 8602166 [details]
MozReview Request: bz://1130096/poiru

r=me with that.  Thanks for fixing this!
Attachment #8602166 - Flags: review?(continuation) → review+
Did not land embedding/components/webbrowserpersist/ yet. Still a few other directories left to do as well.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e82d40166987
Keywords: leave-open
Attachment #8602166 - Attachment is obsolete: true
Attachment #8619335 - Flags: review+
Attachment #8619336 - Flags: review+
Attachment #8619337 - Flags: review+
Attachment #8619338 - Flags: review+
Attachment #8619339 - Flags: review+
Attachment #8619340 - Flags: review+
Attachment #8619341 - Flags: review+
Attachment #8619342 - Flags: review+
Attachment #8619343 - Flags: review+
Attachment #8619344 - Flags: review+
Attachment #8619345 - Flags: review+
Attachment #8619346 - Flags: review+
Attachment #8619347 - Flags: review+
Attachment #8619348 - Flags: review+
Still a few subdirectories left, but I'll file a new bug when I get to them.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.