Convert widget/ to |UniquePtr|

RESOLVED FIXED in Firefox 50

Status

()

Core
Widget
P5
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [good first bug][tpi:+])

Attachments

(2 attachments, 6 obsolete attachments)

112.74 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
7.59 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
|nsAutoPtr| is deprecated.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70444c4b4ee1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06c5f2c2a049
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8fe09442936
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2a52b0dc343
Created attachment 8742816 [details] [diff] [review]
[01] Bug 1265386: Convert code in widget/ to |UniquePtr|
Created attachment 8742817 [details] [diff] [review]
[02] Bug 1265386: Remove remaining include statements for 'nsAutoPtr.h' in widget/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b3ed61ff2b6
Comment on attachment 8742816 [details] [diff] [review]
[01] Bug 1265386: Convert code in widget/ to |UniquePtr|

The code in widget/ has an awful amount of whitespace errors. Sorry about the noise in this patch set.
Attachment #8742816 - Flags: review?(nfroyd)
Attachment #8742817 - Flags: review?(nfroyd)
Comment on attachment 8742816 [details] [diff] [review]
[01] Bug 1265386: Convert code in widget/ to |UniquePtr|

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

Thanks for doing this.  r=me with the changes below.

::: widget/android/nsAppShell.cpp
@@ -75,3 @@
>  
>  nsAppShell* nsAppShell::sAppShell;
> -StaticAutoPtr<Mutex> nsAppShell::sAppShellLock;

Please leave this as a StaticAutoPtr to avoid the static constructor.

::: widget/cocoa/nsChildView.h
@@ -478,4 @@
>                                                      nsIObserver* aObserver) override;
>  
>    // Mac specific methods
> -  

I'm guessing from the amount of changes to whitespace in this patch that you have your editor configured to remove spaces at the end of lines.  Please undo all those changes; it makes the patch harder to review and makes blame unnecessarily tedious.  (When we do eliminate whitespace, we tend to do it as a separate patch.)

::: widget/cocoa/nsChildView.mm
@@ +2620,4 @@
>    // should have created the GLPresenter in InitCompositor.
>    MOZ_ASSERT(mGLPresenter);
>    if (!mGLPresenter) {
> +    mGLPresenter.reset(GLPresenter::CreateForWindow(this));

Please change GLPresenter::CreateForWindow to return a UniquePtr<GLPresenter>, so we can avoid using reset().

::: widget/cocoa/nsMenuX.h
@@ +82,4 @@
>    void           LoadSubMenu(nsIContent* inMenuContent);
>    GeckoNSMenu*   CreateMenuWithGeckoString(nsString& menuTitle);
>  
> +  nsTArray< UniquePtr<nsMenuObjectX> > mMenuObjectsArray;

Nit: this can be nsTArray<UniquePtr<nsMenuObjectX>> without the spaces.

::: widget/gonk/nsWindow.h
@@ +36,4 @@
>  
>  class nsScreenGonk;
>  
> +using mozilla::UniquePtr;

No |using| statements in headers, please.  Qualify the names when they are used in classes or use class-internal typedefs.

::: widget/windows/nsWindowBase.cpp
@@ +9,4 @@
>  #include "KeyboardLayout.h"
>  #include "WinUtils.h"
>  #include "npapi.h"
> +#include "nsAutoPtr.h" // still required by |nsClassHashtable| iterator

This surprises me; nsClassHashtable.h already includes nsAutoPtr.h, so we shouldn't need to include this here?
Attachment #8742816 - Flags: review?(nfroyd) → review+
Comment on attachment 8742817 [details] [diff] [review]
[02] Bug 1265386: Remove remaining include statements for 'nsAutoPtr.h' in widget/

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

r=me with the changes below.

My one concern is that nsAutoPtr.h does quite a bit more than just defining nsAutoPtr; it includes nsCOMPtr, RefPtr, and a couple other things that may or may not be useful in these contexts.  But figuring out all the bootlegging would be a tedious thing, and perhaps not one to block this patch.  Let's just do this for the headers in widget/, as those are probably included by a wider amount of code; if you are feeling particularly motivated, checking headers in platform-specific subdirectories would be splendid, but I'm not going to have it block this patch.

::: widget/GfxInfoBase.cpp
@@ -737,4 @@
>    for (; i < info.Length(); i++) {
>      // Do the operating system check first, no point in getting the driver
>      // info if we won't need to use it.  If the OS of the system we are running
> -    // on is unknown, we still let DRIVER_OS_ALL catch and disable it; 

No whitespace changes in this patch either, please.  You should be able to do something like:

find widget/ -type f | xargs sed -i -e '/#include "nsAutoPtr.h"/d'

to recreate the patch without whitespace changes.

::: widget/TextEventDispatcher.h
@@ -6,4 @@
>  #ifndef mozilla_textcompositionsynthesizer_h_
>  #define mozilla_textcompositionsynthesizer_h_
>  
> -#include "nsAutoPtr.h"

This should include mozilla/RefPtr.h.

::: widget/TouchEvents.h
@@ -10,4 @@
>  
>  #include "mozilla/dom/Touch.h"
>  #include "mozilla/MouseEvents.h"
> -#include "nsAutoPtr.h"

This should include mozilla/RefPtr.h.
Attachment #8742817 - Flags: review?(nfroyd) → review+
> I'm guessing from the amount of changes to whitespace in this patch that you
> have your editor configured to remove spaces at the end of lines.  Please
> undo all those changes; it makes the patch harder to review and makes blame
> unnecessarily tedious.  (When we do eliminate whitespace, we tend to do it
> as a separate patch.)

OK, I'll remove these changes.
 
> ::: widget/windows/nsWindowBase.cpp
> @@ +9,4 @@
> >  #include "KeyboardLayout.h"
> >  #include "WinUtils.h"
> >  #include "npapi.h"
> > +#include "nsAutoPtr.h" // still required by |nsClassHashtable| iterator
> 
> This surprises me; nsClassHashtable.h already includes nsAutoPtr.h, so we
> shouldn't need to include this here?

I had seen this in the case of Bluetooth, but maybe I misremember. I'll remove the line if possible.
> My one concern is that nsAutoPtr.h does quite a bit more than just defining
> nsAutoPtr; it includes nsCOMPtr, RefPtr, and a couple other things that may
> or may not be useful in these contexts.  But figuring out all the
> bootlegging would be a tedious thing, and perhaps not one to block this
> patch.  Let's just do this for the headers in widget/, as those are probably
> included by a wider amount of code; if you are feeling particularly
> motivated, checking headers in platform-specific subdirectories would be
> splendid, but I'm not going to have it block this patch.

I sent quite a few pushes to try to make sure all targets still build. I think should be fine now. So can I leave the platform-specific changes in the patch?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> > My one concern is that nsAutoPtr.h does quite a bit more than just defining
> > nsAutoPtr; it includes nsCOMPtr, RefPtr, and a couple other things that may
> > or may not be useful in these contexts.  But figuring out all the
> > bootlegging would be a tedious thing, and perhaps not one to block this
> > patch.  Let's just do this for the headers in widget/, as those are probably
> > included by a wider amount of code; if you are feeling particularly
> > motivated, checking headers in platform-specific subdirectories would be
> > splendid, but I'm not going to have it block this patch.
> 
> I sent quite a few pushes to try to make sure all targets still build. I
> think should be fine now. So can I leave the platform-specific changes in
> the patch?

Platform-specific changes are OK, but for avoidance of doubt, please change the two files mentioned in comment 10.
Created attachment 8743813 [details] [diff] [review]
[00] Bug 1265386: Fix white-space errors in widget/

Only the white-space fixes
Attachment #8743813 - Flags: review?(nfroyd)
Created attachment 8743818 [details] [diff] [review]
[01] Bug 1265386: Convert code in widget/ to |UniquePtr| (v2)

Changes since v1:

  - removed white-space fixes
  - keep |StaticAutoPtr| in Android's |nsAppShell|
  - return |RefPtr| from |GLPresenter::CreateForWindow|
  - spaces
  - no 'using' in headers
  - don't include "nsAutoPtr.h" in Windows' 'nsWindowBase.cpp'
Attachment #8742816 - Attachment is obsolete: true
Attachment #8743818 - Flags: review+
Created attachment 8743824 [details] [diff] [review]
[02] Bug 1265386: Remove remaining include statements for 'nsAutoPtr.h' in widget/ (v2)

Changes since v1:

  - removed white-space fixes
  - include 'RefPtr.h' in files that require it
Attachment #8742817 - Attachment is obsolete: true
Attachment #8743824 - Flags: review+
Are you aware of bug 1265953?  It looks like jwatt has replicated a lot of this work, judging by his patches.  I guess we could repurpose this bug to fix whitespace errors?
Flags: needinfo?(tzimmermann)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=188149db42b9
(In reply to Nathan Froyd [:froydnj] from comment #17)
> Are you aware of bug 1265953?  It looks like jwatt has replicated a lot of

Fantastic. So I wasted 3 days on this.

> this work, judging by his patches.  I guess we could repurpose this bug to
> fix whitespace errors?

Let's see what's left after bug 1265953 landed.
Flags: needinfo?(tzimmermann)
Depends on: 1265953
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #19)
> Fantastic. So I wasted 3 days on this.

Crap. Really sorry about that. :( I did check for a bug just before writing patches and filing my own, but it must have been just before you filed this. It's unfortunate we both looked at this at roughly the same time.
Comment on attachment 8743813 [details] [diff] [review]
[00] Bug 1265386: Fix white-space errors in widget/

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

OK by me.
Attachment #8743813 - Flags: review?(nfroyd) → review+

Updated

a year ago
Priority: -- → P5
Whiteboard: [good first bug][tpi:+]
There isn't much to do here. I'll land the whitespace cleanups and the 3 remaining changes in patch [01]. Should all be r+'ed.
Created attachment 8773238 [details] [diff] [review]
[00] Bug 1265386: Fix white-space errors in widget/ (v2)

Changes since v1:

  - rebased onto latest m-c
Attachment #8743813 - Attachment is obsolete: true
Attachment #8773238 - Flags: review+
Created attachment 8773239 [details] [diff] [review]
[01] Bug 1265386: Convert code in widget/ to |UniquePtr| (v3)

Changes since v2:

  - rebased onto latest m-c
Attachment #8743818 - Attachment is obsolete: true
Attachment #8773239 - Flags: review+
Attachment #8743824 - Attachment is obsolete: true

Comment 25

a year ago
Pushed by tdz@users.sourceforge.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bea1e1e403d
Fix white-space errors in widget/, r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/c402d9e91897
Convert code in widget/ to |UniquePtr|, r=nfroyd
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c402d9e91897
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60618cbbe23c

Comment 28

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8798ff34e070
Backed out changeset c402d9e91897 
https://hg.mozilla.org/integration/mozilla-inbound/rev/34fab997a0a1
Backed out changeset 2bea1e1e403d for bustage on a CLOSED TREE
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32344339&repo=mozilla-inbound
Flags: needinfo?(tzimmermann)
Created attachment 8773674 [details] [diff] [review]
[01] Bug 1265386: Convert code in widget/ to |UniquePtr| (v4)

Changes since v3:

  - construct |UniquePtr| in |GLPresenter::CreateForWindow|
Attachment #8773239 - Attachment is obsolete: true
Flags: needinfo?(tzimmermann)
Attachment #8773674 - Flags: review+

Comment 31

a year ago
Pushed by tdz@users.sourceforge.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d2b159ceb2
Fix white-space errors in widget/, r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e7c04db48f
Convert code in widget/ to |UniquePtr|, r=nfroyd
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=25e7c04db48f

Comment 33

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7d2b159ceb2
https://hg.mozilla.org/mozilla-central/rev/25e7c04db48f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.