If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up dead code related to missing screen managers in widget

RESOLVED FIXED in Firefox 48

Status

()

Core
Widget: Win32
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: jimm, Assigned: mattoc, Mentored)

Tracking

Trunk
mozilla48
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

7 years ago
Created attachment 458558 [details] [diff] [review]
patch
Assignee: nobody → jmathies
(Reporter)

Comment 2

7 years ago
Comment on attachment 458558 [details] [diff] [review]
patch

Looks like that's all of it - when you have the time.
Attachment #458558 - Flags: review?(tellrob)
Comment on attachment 458558 [details] [diff] [review]
patch

Isn't it the case that ScreenForRect always returns a screen? We can prune a bit more code out of this I think :)
(Reporter)

Updated

7 years ago
Attachment #458558 - Flags: review?(tellrob) → review-
(Reporter)

Updated

4 years ago
Assignee: jmathies → nobody
(Reporter)

Updated

3 years ago
Mentor: jmathies@mozilla.com
Whiteboard: [good first bug][lang=c++]

Comment 4

3 years ago
I would like to take this bug.
Flags: needinfo?(jmathies)
(Reporter)

Comment 5

3 years ago
Ok! The original patch needed additional cleanup per comment 3.
Assignee: nobody → akashs2795
Flags: needinfo?(jmathies)

Comment 6

3 years ago
Hello Jim if akash is not working on this bug then can i take it..??

Comment 7

3 years ago
(In reply to Rob Arnold [:robarnold] from comment #3)
> Comment on attachment 458558 [details] [diff] [review]
> patch
> 
> Isn't it the case that ScreenForRect always returns a screen? We can prune a
> bit more code out of this I think :)

Hey Rob!
Just wanted a clarification, since RectForScreen returns a screen always, I think the only clipping would be removing the "if(screen)" part, right?

Comment 8

2 years ago
Is this bug currently being worked on/does it need more polish before it goes out?

Comment 9

2 years ago
Hi, can I work on this bug please and can you give me pointers to get started?
Flags: needinfo?(jmathies)
(Reporter)

Comment 10

2 years ago
(In reply to Sunny Sidhu from comment #9)
> Hi, can I work on this bug please and can you give me pointers to get
> started?

If you would like to work on this, here are some suggested steps to follow:

1) get the existing patch applied to mozilla-central. it's probably pretty bitrotten by now.
2) build and test the patch, make sure it doesn't break anything.
3) update the patch to include the changes requested in comment 3
4) post a new patch and ask for review.

I can help test and post to try once you get this far.

Thanks!
Assignee: akashs2795 → sidhus11
Flags: needinfo?(jmathies)

Comment 11

2 years ago
Created attachment 8681931 [details] [diff] [review]
rev1 - Applies changes in original patch

Hi Jim. Sorry for the delay, university assignments have been taking up time!

I found nsWindow.cpp has changed since when you originally wrote the existing patch. What I've done is try to implement the changes you made originally into the current version of nsWindow.cpp. The build is successful. 

Please can you review my changes to make sure they're fine? I also don't know how to go about testing them, and if the changes requested in comment 3 still need addressing?

Please can you advise. Thanks.
Attachment #8681931 - Flags: feedback?(jmathies)
(Reporter)

Comment 12

2 years ago
Comment on attachment 8681931 [details] [diff] [review]
rev1 - Applies changes in original patch

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

::: widget/windows/nsWindow.cpp
@@ +1728,5 @@
>    RECT screenRect;
>  
>    nsCOMPtr<nsIScreenManager> screenmgr = do_GetService(sScreenManagerContractID);
> +  if (!screenmgr)
> +	return NS_ERROR_NOT_AVAILABLE;

this should be :

if (!screengr) {
  return ..;
}


You also didn't update the indentation of the if block. Here's a coding style doc for help:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +1729,5 @@
>  
>    nsCOMPtr<nsIScreenManager> screenmgr = do_GetService(sScreenManagerContractID);
> +  if (!screenmgr)
> +	return NS_ERROR_NOT_AVAILABLE;
> +	

nit - you added whitespace here.

@@ +1747,5 @@
>        screenRect.left = left;
>        screenRect.right = left + width;
>        screenRect.top = top;
>        screenRect.bottom = top + height;
> +	}

indentation is off here.
Attachment #8681931 - Flags: feedback?(jmathies) → feedback-
(Reporter)

Comment 13

2 years ago
(In reply to Sunny Sidhu from comment #11)
> Please can you review my changes to make sure they're fine? I also don't
> know how to go about testing them, and if the changes requested in comment 3
> still need addressing?

You have to spend some time to understand what this code does, and then find a way to test it. Look at the method name and figure out what calls it. Search dxr for clues as to what this code does. You have to put the effort into understanding the code you're working on.

https://dxr.mozilla.org/mozilla-central/source/

Updated

2 years ago
Assignee: sidhus11 → nobody

Comment 14

2 years ago
Hello, I would like to work on this bug please.

Comment 15

2 years ago
Created attachment 8697973 [details] [diff] [review]
BugNo580165.patch

Hi, I've made the changes. Can you have a look and give me some feedback?
Attachment #8697973 - Flags: review?(jmathies)
(Reporter)

Comment 16

2 years ago
Comment on attachment 8697973 [details] [diff] [review]
BugNo580165.patch

something went wrong in generating your patch, this file doesn't contain any changes.
Attachment #8697973 - Flags: review?(jmathies)
(Assignee)

Comment 17

2 years ago
Created attachment 8702369 [details] [diff] [review]
bug580165_removeDeadCode.diff

Hi Jim, I've followed your instructions in comment #10 and applied the changes in the initial patch to the latest version. I then updated the patch to include the changes from comment #3. I built successfully after each step.
Attachment #8702369 - Flags: review?(jmathies)
(Reporter)

Comment 18

2 years ago
Comment on attachment 8702369 [details] [diff] [review]
bug580165_removeDeadCode.diff

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

almost there, sorry for the dealy in review.

::: widget/windows/nsWindow.cpp
@@ +1734,5 @@
> +  if (!screenmgr) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  
> +  nsCOMPtr<nsIScreen> screen;

nit - whitespace above this line

@@ +1741,5 @@
> +  screenmgr->ScreenForRect(*aX, *aY, logWidth, logHeight,
> +                           getter_AddRefs(screen));
> +  if (mSizeMode != nsSizeMode_Fullscreen) {
> +    // For normalized windows, use the desktop work area.
> +    screen->GetAvailRectDisplayPix(&left, &top, &width, &height);

This can fail, in which case these variables would be left uninitialized. Lets check for failure here and return an error in the off chance this happens.
Attachment #8702369 - Flags: review?(jmathies) → review-
(Assignee)

Comment 19

2 years ago
Created attachment 8720439 [details] [diff] [review]
Remove dead code in nsWindow.cpp - v1.1

Hi Jim, sorry for the delayed response.

I've fixed the whitespace, but I need some guidance as I can't figure out what funciton can fail.
Assignee: nobody → mattsdesktop
Attachment #8702369 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(jmathies)
(Reporter)

Comment 20

2 years ago
(In reply to Matt O'Connor [:mattoc] from comment #19)
> Created attachment 8720439 [details] [diff] [review]
> Remove dead code in nsWindow.cpp - v1.1
> 
> Hi Jim, sorry for the delayed response.
> 
> I've fixed the whitespace, but I need some guidance as I can't figure out
> what funciton can fail.

screen->GetAvailRectDisplayPix can fail, see - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsScreenWin.h#30
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsScreenWin.cpp#127

So what you want to do is catch the nsresult and check it for failure after the call, and return that result if it does.

nsresult rv = screen->GetAvailRectDisplayPix(...

if (NS_FAILED(rv)) {
  return rv;
}
Flags: needinfo?(jmathies)
(Assignee)

Comment 21

2 years ago
Created attachment 8727176 [details] [diff] [review]
Remove dead code in nsWindow.cpp - v1.2

Thanks for pointing me in the right direction Jim.

As you advised, I've made the code check the return value of GetAvailRectDisplayPix for failure, and return this value if necessary.

I've also done the same for GetRectDisplayPix, although I'm not sure if this is necessary.
Attachment #8720439 - Attachment is obsolete: true
Attachment #8727176 - Flags: review?(jmathies)
(Reporter)

Comment 22

2 years ago
Comment on attachment 8727176 [details] [diff] [review]
Remove dead code in nsWindow.cpp - v1.2

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

::: widget/windows/nsWindow.cpp
@@ +1835,5 @@
>  
>    nsCOMPtr<nsIScreenManager> screenmgr = do_GetService(sScreenManagerContractID);
> +  if (!screenmgr) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }  

nit - little bit of whitespace here, mind cleaning that up and posting a final? I'll push this to try.
Attachment #8727176 - Flags: review?(jmathies) → review+
(Reporter)

Comment 23

2 years ago
Also please update your commit message -

# HG changeset patch
# User Matthew O'Connor <mattsdesktop@gmail.com>
# Parent  46210f3ae07855edfe1383210d78433e38a6b564
Remove dead code in nsWindow.cpp

Something like - 

Bug 580165 - Clean up dead code related to missing screen managers in widget. r=jimm
(Reporter)

Comment 24

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccdafadec673
(Assignee)

Comment 25

2 years ago
Created attachment 8728302 [details] [diff] [review]
Remove dead code in nsWindow.cpp - FINAL

Hi Jim, I've removed the whitespace and updated the commit message as advised.

I want to ensure that I'm using the flags correctly; is it acceptable to mark this revision as 'review+' myself, or should I have marked it as 'review?' again?

Also, would it have been wrong to use the checkin flag instead of needinfo, because it needs to pass try first, or is pushing to try a part of the checkin process?
Attachment #458558 - Attachment is obsolete: true
Attachment #8681931 - Attachment is obsolete: true
Attachment #8697973 - Attachment is obsolete: true
Attachment #8727176 - Attachment is obsolete: true
Flags: needinfo?(jmathies)
Attachment #8728302 - Flags: review+
(Reporter)

Comment 26

2 years ago
(In reply to Matt O'Connor [:mattoc] from comment #25)
> Created attachment 8728302 [details] [diff] [review]
> Remove dead code in nsWindow.cpp - FINAL
> 
> Hi Jim, I've removed the whitespace and updated the commit message as
> advised.
> 
> I want to ensure that I'm using the flags correctly; is it acceptable to
> mark this revision as 'review+' myself, or should I have marked it as
> 'review?' again?

If the reviewer gave you r+, it's up to you to decide if a new review is needed. For minor updates like whitespace you mark the new patch r+ yourself. If you end up making larger changes to address review comments, it's considered good practice to flag for another review.

> Also, would it have been wrong to use the checkin flag instead of needinfo,
> because it needs to pass try first, or is pushing to try a part of the
> checkin process?

We don't do auto-try pushes yet which is why I pushed it for you. We're working on this feature using the new mozreview web site which you might take a look at.

https://reviewboard.mozilla.org/
Flags: needinfo?(jmathies)
Keywords: checkin-needed

Comment 27

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/308616f183c9
Keywords: checkin-needed

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/308616f183c9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Reporter)

Comment 29

2 years ago
Thanks for the help Matt, are you planning on tacking any other bugs?
You need to log in before you can comment on or make changes to this bug.