Clean up formatting in nsScreenWin.cpp and nsScreenManagerWin.cpp

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: lewis, Mentored)

Tracking

Trunk
mozilla33
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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)
{
(Assignee)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
Cool! Let me know if it sounds like something you want to do, and I'll assign you the bug.
(Assignee)

Comment 3

5 years ago
Didn't get a chance last night, will check it out tonight, and work on a patch over the weekend.
(Assignee)

Comment 4

5 years ago
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. :)
(Reporter)

Comment 5

5 years ago
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!
Assignee: nobody → lewis
(Assignee)

Comment 6

5 years ago
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.
(Reporter)

Comment 7

5 years ago
(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?
Flags: needinfo?(lewis)
(Assignee)

Comment 8

5 years ago
I might not have. My hg diff had all my changes. I'll try again.
Flags: needinfo?(lewis)
(Assignee)

Comment 9

5 years ago
Created attachment 8455215 [details] [diff] [review]
Second attempt at doing the cleanup of whitespace.

Hopefully this works now. Same comment as before.
(Reporter)

Updated

5 years ago
Attachment #8455068 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
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.
Attachment #8455215 - Flags: feedback+
(Assignee)

Comment 11

5 years ago
Created attachment 8455322 [details] [diff] [review]
Cleaning up formatting across two files

Third time's the charm hopefully.
(Reporter)

Updated

5 years ago
Attachment #8455215 - Attachment is obsolete: true
(Reporter)

Comment 12

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

Looks good to me! Requesting review from a widget/windows peer.
Attachment #8455322 - Flags: review?(jmathies)
Attachment #8455322 - Flags: feedback+
(Reporter)

Comment 13

5 years ago
Created attachment 8455422 [details] [diff] [review]
De-bitrotted patch

Bug 1002354 is going to bitrot this when it lands and sticks, so I've pre-emptively de-bitrotted this patch.
Attachment #8455322 - Attachment is obsolete: true
Attachment #8455322 - Flags: review?(jmathies)
Attachment #8455422 - Flags: feedback+
(Reporter)

Comment 14

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

And re-requesting review from jimm for Lewis.
Attachment #8455422 - Flags: review?(jmathies)
Comment on attachment 8455422 [details] [diff] [review]
De-bitrotted patch

Looks good, thanks!
Attachment #8455422 - Flags: review?(jmathies) → review+
(Reporter)

Updated

5 years ago
Depends on: 1002354
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
(Reporter)

Comment 17

5 years ago
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.
(Reporter)

Comment 18

5 years ago
Build built OK. Is that enough information, Tomcat?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/71bc8f4a5bb7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.