1036003 - Clean up formatting in nsScreenWin.cpp and nsScreenManagerWin.cpp

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

The spacing in widget/windows/nsScreenWin.cpp and widget/windows/nsScreenManagerWin.cpp is pretty gross.

Example:

NS_IMETHODIMP
nsScreenManagerWin :: ScreenForRect ( int32_t inLeft, int32_t inTop, int32_t inWidth, int32_t inHeight,
                                        nsIScreen **outScreen )
{

should be something like:

NS_IMETHODIMP
nsScreenManagerWin::ScreenForRect (int32_t inLeft, int32_t inTop, int32_t inWidth, int32_t inHeight,
                                        nsIScreen **outScreen)
{

Comment 1

[lewis]

I'd love to have a crack at this, though I've not done much C++. I'll check it out in a few hours when I get home.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Cool! Let me know if it sounds like something you want to do, and I'll assign you the bug.

Comment 3

[lewis]

Didn't get a chance last night, will check it out tonight, and work on a patch over the weekend.

Comment 4

[lewis]

This sounds exactly like something I'd love to be assigned to, and although something's come up that'll take up Saturday, I've got the code, and I'm building FF now, so I should have a patch ready Sunday night. :)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Great! I've assigned you the bug. Let me know if you have any questions on how to proceed on it. Make sure to "needinfo" me with the checkbox at the bottom of this page.

Click it, and then choose "Mentor" from the dropdown list, and it'll be added to my notification queue.

Thanks!

Comment 6

[lewis]

Attachment: Cleaned up whitespace across two files.

I'm pretty sure I covered all the parts that I could safely change, (although I may have missed something, I'm not familiar with C++. I've run a build with the changes in, and nothing has broken as far as I can see. I'm not sure if I've submitted this patch the right way, I've not used mercurial before. Let me know if anything else needs doing to get it merged.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to lewis from comment #6)
> Created attachment 8455068 [details] [diff] [review]
> Cleaned up whitespace across two files.
> 
> I'm pretty sure I covered all the parts that I could safely change,
> (although I may have missed something, I'm not familiar with C++. I've run a
> build with the changes in, and nothing has broken as far as I can see. I'm
> not sure if I've submitted this patch the right way, I've not used mercurial
> before. Let me know if anything else needs doing to get it merged.

Hm - this patch is empty. Are you sure you qref'd?

Comment 8

[lewis]

I might not have. My hg diff had all my changes. I'll try again.

Comment 9

[lewis]

Attachment: Second attempt at doing the cleanup of whitespace.

Hopefully this works now. Same comment as before.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8455215 [details] [diff] [review]
Second attempt at doing the cleanup of whitespace.

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

Looking pretty good! Thanks for the patch, Lewis!

I've noticed some things to fix up a bit. After you post a new patch, I'll get a widget/windows peer to give an actual review.

Thanks!

::: widget/windows/nsScreenManagerWin.cpp
@@ +8,5 @@
>  #include "gfxWindowsPlatform.h"
>  #include "nsIWidget.h"
>  
>  
> +BOOL CALLBACK CountMonitors(HMONITOR, HDC, LPRECT, LPARAM ioCount) ;

We can probably get rid of that space before the semicolon too.

@@ +37,5 @@
>  // NOTE: For this "single-monitor" impl, we just always return the cached primary
>  //        screen. This should change when a multi-monitor impl is done.
>  //
>  nsIScreen* 
> +nsScreenManagerWin::CreateNewScreenObject (HMONITOR inScreen)

Get rid of the space before (HMONITOR inScreen)

@@ +68,5 @@
>  //
>  // The coordinates are in pixels (not twips) and in logical screen coordinates.
>  //
>  NS_IMETHODIMP
> +nsScreenManagerWin::ScreenForRect(int32_t inLeft, int32_t inTop, int32_t inWidth, int32_t inHeight,

We might want to clean up the alignment in the parameter list as well - specifically, the outScreen param.

@@ +105,5 @@
>  // The screen with the menubar/taskbar. This shouldn't be needed very
>  // often.
>  //
>  NS_IMETHODIMP 
> +nsScreenManagerWin::GetPrimaryScreen(nsIScreen** aPrimaryScreen) 

Might as well get rid of the trailing whitespace on this line and 111 while you're at it.

::: widget/windows/nsScreenWin.cpp
@@ +61,5 @@
>  } // GetRect
>  
>  
>  NS_IMETHODIMP
>  nsScreenWin :: GetAvailRect(int32_t *outLeft, int32_t *outTop, int32_t *outWidth, int32_t *outHeight)

No spaces between nsScreenWin, GetAvailRect and the ::

@@ +157,5 @@
>  } // GetPixelDepth
>  
>  
>  NS_IMETHODIMP 
> +nsScreenWin :GetColorDepth(int32_t *aColorDepth)

This looks busted - we need :: between nsScreenWin and GetColorDepth.

Comment 11

[lewis]

Attachment: Cleaning up formatting across two files

Third time's the charm hopefully.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8455322 [details] [diff] [review]
Cleaning up formatting across two files

Looks good to me! Requesting review from a widget/windows peer.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: De-bitrotted patch

Bug 1002354 is going to bitrot this when it lands and sticks, so I've pre-emptively de-bitrotted this patch.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8455422 [details] [diff] [review]
De-bitrotted patch

And re-requesting review from jimm for Lewis.

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 8455422 [details] [diff] [review]
De-bitrotted patch

Looks good, thanks!

Comment 16

[Carsten Book [:Tomcat]]

Hi,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Pushed to try:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=4821a5708c76

Only testing that it builds, since this is simply a C++ formatting change.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Build built OK. Is that enough information, Tomcat?

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71bc8f4a5bb7

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/71bc8f4a5bb7