Closed
Bug 1265386
Opened 9 years ago
Closed 9 years ago
Convert widget/ to |UniquePtr|
Categories
(Core :: Widget, defect, P5)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: tzimmermann, Assigned: tzimmermann)
References
Details
(Whiteboard: [good first bug][tpi:+])
Attachments
(2 files, 6 obsolete files)
112.74 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
7.59 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
|nsAutoPtr| is deprecated.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8742817 -
Flags: review?(nfroyd)
![]() |
||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
> 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.
Assignee | ||
Comment 12•9 years ago
|
||
> 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?
![]() |
||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
Only the white-space fixes
Attachment #8743813 -
Flags: review?(nfroyd)
Assignee | ||
Comment 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
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+
![]() |
||
Comment 17•9 years ago
|
||
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)
Assignee | ||
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
(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)
![]() |
||
Comment 20•9 years ago
|
||
(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 21•9 years ago
|
||
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•9 years ago
|
Priority: -- → P5
Whiteboard: [good first bug][tpi:+]
Assignee | ||
Comment 22•9 years ago
|
||
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.
Assignee | ||
Comment 23•9 years ago
|
||
Changes since v1:
- rebased onto latest m-c
Attachment #8743813 -
Attachment is obsolete: true
Attachment #8773238 -
Flags: review+
Assignee | ||
Comment 24•9 years ago
|
||
Changes since v2:
- rebased onto latest m-c
Attachment #8743818 -
Attachment is obsolete: true
Attachment #8773239 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8743824 -
Attachment is obsolete: true
Comment 25•9 years 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
Assignee | ||
Comment 26•9 years ago
|
||
Assignee | ||
Comment 27•9 years ago
|
||
Comment 28•9 years 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
Comment 29•9 years ago
|
||
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32344339&repo=mozilla-inbound
Flags: needinfo?(tzimmermann)
Assignee | ||
Comment 30•9 years ago
|
||
Changes since v3:
- construct |UniquePtr| in |GLPresenter::CreateForWindow|
Attachment #8773239 -
Attachment is obsolete: true
Flags: needinfo?(tzimmermann)
Attachment #8773674 -
Flags: review+
Comment 31•9 years 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
Assignee | ||
Comment 32•9 years ago
|
||
Comment 33•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7d2b159ceb2
https://hg.mozilla.org/mozilla-central/rev/25e7c04db48f
Status: ASSIGNED → RESOLVED
Closed: 9 years 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.
Description
•