Closed
Bug 1158120
Opened 9 years ago
Closed 9 years ago
Remove all occurrences of the gfxIntSize and nsIntSize typedefs in gfx code, replace them by gfx::IntSize
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: nical, Assigned: amanda.sambath, Mentored)
Details
Attachments
(11 files, 2 obsolete files)
13.75 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
4.10 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
1.16 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
12.53 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
9.29 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
19.31 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
77.70 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
5.56 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
16.99 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
Now that gfxIntSize is a typedef of gfx::IntSize and not a separate classe, we can remove all of its occurrences in graphics code (anything under the gfx/ directory). This is purely a cosmetic change, perfect for someone who would like to understand the workflow of code submitting contributions before tackling a real bug or feature. grep -r gfxIntSize gfx/ and manually replace gfxIntSize by gfx::IntSize in code that is under the mozilla namespace, IntSize for code under mozilla::gfx or .cpp files with "using namespace mozilla::gfx;" or mozilla::gfx::IntSize for the few headers with no namespaces. Let's leave out the code outside of gfx/ for now to let each module owner decide if, how and/or when to deal with updating the size classes/typedefs.
Reporter | ||
Comment 1•9 years ago
|
||
Oh, and same treatment for nsIntSize which is also a typedef of gfx::IntSize.
Summary: Remove all occurrences of the gfxIntSize typedef in gfx code, replace it by gfx::IntSize → Remove all occurrences of the gfxIntSize and nsIntSize typedefs in gfx code, replace them by gfx::IntSize
Assignee | ||
Comment 2•9 years ago
|
||
I would like to fix this bug.
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → amanda.sambath
Assignee | ||
Comment 3•9 years ago
|
||
Build succeeded.
Attachment #8612178 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 4•9 years ago
|
||
Build successful.
Attachment #8612193 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 5•9 years ago
|
||
Build successful.
Attachment #8612212 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 6•9 years ago
|
||
Build successful.
Attachment #8612228 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 7•9 years ago
|
||
Build successful.
Attachment #8612244 -
Flags: review?(nical.bugzilla)
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 8612178 [details] [diff] [review] thebesIntSize.patch Review of attachment 8612178 [details] [diff] [review]: ----------------------------------------------------------------- The patch is good, but I'd like that you address the comments below. They are not problems with your changes, but rather leftovers from before and this patch is a good occasion to fix them. ::: gfx/thebes/gfxQtPlatform.cpp @@ +94,5 @@ > { > gfxImageFormat imageFormat = OptimalFormatForContent(contentType); > > nsRefPtr<gfxASurface> newSurface = > + new gfxImageSurface(IntSize(size.width, size.height), imageFormat); Please just pass size directly (you have an IntSize in parameter and the constructor takes an IntSize). The unneeded conversion here is a leftover from the past but lets use patch as an occasion to fix it. ::: gfx/thebes/gfxQuartzImageSurface.cpp @@ +38,1 @@ > cairo_image_surface_get_height(isurf)); Please align the parameters on the extra lines. ::: gfx/thebes/gfxQuartzImageSurface.h @@ +19,5 @@ > virtual ~gfxQuartzImageSurface(); > > already_AddRefed<gfxImageSurface> GetAsImageSurface(); > virtual int32_t KnownMemoryUsed(); > + virtual const mozilla::gfx::IntSize GetSize() const { return mozilla::gfx::IntSize(mSize.width, mSize.height); } Just "return mSize;" here. ::: gfx/thebes/gfxQuartzSurface.cpp @@ +11,5 @@ > > void > gfxQuartzSurface::MakeInvalid() > { > + mSize = mozilla::gfx::IntSize(-1, -1); please remove the trailing spaces (they were already there, but let's use this occasion to remove them) ::: gfx/thebes/gfxSharedImageSurface.h @@ +13,5 @@ > { > typedef gfxBaseSharedMemorySurface<gfxImageSurface, gfxSharedImageSurface> Super; > friend class gfxBaseSharedMemorySurface<gfxImageSurface, gfxSharedImageSurface>; > private: > + gfxSharedImageSurface(const mozilla::gfx::IntSize& aSize, long aStride, Please remove the trailing space on this line and the line below. ::: gfx/thebes/gfxSharedQuartzSurface.h @@ +14,5 @@ > { > typedef gfxBaseSharedMemorySurface<gfxQuartzSurface, gfxSharedQuartzSurface> Super; > friend class gfxBaseSharedMemorySurface<gfxQuartzSurface, gfxSharedQuartzSurface>; > private: > + gfxSharedQuartzSurface(const mozilla::gfx::IntSize& aSize, long aStride, please remove the trailing space. ::: gfx/thebes/gfxWindowsSurface.cpp @@ +282,5 @@ > return NS_ERROR_FAILURE; > #endif > } > > +const mozilla::gfx::IntSize trailing space
Attachment #8612178 -
Flags: review?(nical.bugzilla) → feedback+
Reporter | ||
Updated•9 years ago
|
Attachment #8612193 -
Flags: review?(nical.bugzilla) → review+
Reporter | ||
Comment 9•9 years ago
|
||
Comment on attachment 8612212 [details] [diff] [review] layersIntSize.patch Review of attachment 8612212 [details] [diff] [review]: ----------------------------------------------------------------- Two little comments to address like the first patch. ::: gfx/layers/basic/BasicLayerManager.cpp @@ +777,1 @@ > aDestRect.height), Please align the paramaters ::: gfx/layers/opengl/CompositorOGL.cpp @@ +130,1 @@ > mSurfaceSize.height), Please replace > IntSize(mSurfaceSize.witdh, mSurfaceSize.height) by > mSurfaceSize directly
Attachment #8612212 -
Flags: review?(nical.bugzilla) → feedback+
Reporter | ||
Updated•9 years ago
|
Attachment #8612228 -
Flags: review?(nical.bugzilla) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8612244 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Build successful.
Attachment #8612744 -
Flags: review?(nical.bugzilla)
Reporter | ||
Updated•9 years ago
|
Attachment #8612744 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Build successful.
Attachment #8612812 -
Flags: review?(nical.bugzilla)
Reporter | ||
Updated•9 years ago
|
Attachment #8612812 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Build successful.
Attachment #8612844 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 13•9 years ago
|
||
Build successful.
Attachment #8612845 -
Flags: review?(nical.bugzilla)
Reporter | ||
Updated•9 years ago
|
Attachment #8612844 -
Flags: review?(nical.bugzilla) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8612845 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Build successful.
Assignee | ||
Updated•9 years ago
|
Attachment #8612896 -
Flags: review?(nical.bugzilla)
Reporter | ||
Updated•9 years ago
|
Attachment #8612896 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8612178 -
Attachment is obsolete: true
Attachment #8613421 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8612212 -
Attachment is obsolete: true
Attachment #8613422 -
Flags: review?(nical.bugzilla)
Reporter | ||
Updated•9 years ago
|
Attachment #8613421 -
Flags: review?(nical.bugzilla) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8613422 -
Flags: review?(nical.bugzilla) → review+
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7654212d5a47 https://hg.mozilla.org/integration/mozilla-inbound/rev/4863d5946d1b https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd155ffdc6 https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9a8b700143 https://hg.mozilla.org/integration/mozilla-inbound/rev/49cceccb1a45 https://hg.mozilla.org/integration/mozilla-inbound/rev/a75fce794713 https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7379cd9ddd https://hg.mozilla.org/integration/mozilla-inbound/rev/b66e5680a70d https://hg.mozilla.org/integration/mozilla-inbound/rev/ff36a419222a
Reporter | ||
Updated•9 years ago
|
Whiteboard: [leave-open]
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3eae6ca6d822 https://hg.mozilla.org/mozilla-central/rev/7654212d5a47 https://hg.mozilla.org/mozilla-central/rev/4863d5946d1b https://hg.mozilla.org/mozilla-central/rev/77bd155ffdc6 https://hg.mozilla.org/mozilla-central/rev/0e9a8b700143 https://hg.mozilla.org/mozilla-central/rev/49cceccb1a45 https://hg.mozilla.org/mozilla-central/rev/a75fce794713 https://hg.mozilla.org/mozilla-central/rev/4f7379cd9ddd https://hg.mozilla.org/mozilla-central/rev/b66e5680a70d https://hg.mozilla.org/mozilla-central/rev/ff36a419222a
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8614633 -
Flags: review?(nical.bugzilla)
Reporter | ||
Updated•9 years ago
|
Attachment #8614633 -
Flags: review?(nical.bugzilla) → review+
Reporter | ||
Comment 22•9 years ago
|
||
That does it for nsIntSize and gfxIntSize in the gfx module \o/
Whiteboard: [leave-open]
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 23•9 years ago
|
||
I already landed all of the patches in mozilla-inbound.
Keywords: checkin-needed
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ef8381db1873
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•