Closed
Bug 1224403
Opened 9 years ago
Closed 9 years ago
Use LayoutDevicePixel type more in widget/
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(12 files)
9.87 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
18.87 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
15.48 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
7.37 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
12.52 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
4.07 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
6.11 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
3.56 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
6.08 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
I have a bunch of patches to add more coordinate types to widget/.
Assignee | ||
Comment 1•9 years ago
|
||
BTW, some of these might look like they're going backwards -- introducing more ToUnknown/FromUnknown calls than they remove -- but it's sometimes necessary and the general trend is in a good direction.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8686876 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8686878 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8686879 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 5•9 years ago
|
||
This required adding {To,From}UnknownMargin().
Attachment #8686885 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8686887 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8686890 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8686892 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8686893 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8686894 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8686895 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8686896 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 13•9 years ago
|
||
BTW, the comment on WidgetToScreenOffset() mentions screen coordinates. Should it instead mention device coordinates? (Perhaps the terminology used changed over time?)
Attachment #8686898 -
Flags: review?(bugmail.mozilla)
Comment 14•9 years ago
|
||
Comment on attachment 8686876 [details] [diff] [review] (part 1) - Make Configuration::mBounds a LayoutDeviceIntRect Review of attachment 8686876 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsWindow.cpp @@ +6546,5 @@ > // XXX - Workaround for Bug 587508. This will invalidate the part of the > // plugin window that might be touched by moving content somehow. The > // underlying problem should be found and fixed! > nsIntRegion r; > + r.Sub(bounds.ToUnknownRect(), configuration.mBounds.ToUnknownRect()); It might be easier to use LayoutDeviceIntRegion instead of nsIntRegion for |r| here.
Attachment #8686876 -
Flags: review?(bugmail.mozilla) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8686878 [details] [diff] [review] (part 2) - Split GetClientOffset() into typed and untyped versions Review of attachment 8686878 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsWindow.h @@ +119,5 @@ > NS_IMETHOD GetBoundsUntyped(nsIntRect &aRect); > NS_IMETHOD GetScreenBoundsUntyped(nsIntRect &aRect); > NS_IMETHOD GetRestoredBoundsUntyped(nsIntRect &aRect) override; > NS_IMETHOD GetClientBoundsUntyped(nsIntRect &aRect); > + virtual nsIntPoint GetClientOffsetUntyped(); Mark this |override| since you're touching it anyway.
Attachment #8686878 -
Flags: review?(bugmail.mozilla) → review+
Comment 16•9 years ago
|
||
Comment on attachment 8686879 [details] [diff] [review] (part 3) - Make mNonClientOffset a LayoutDeviceIntMargin Review of attachment 8686879 [details] [diff] [review]: ----------------------------------------------------------------- Neat :)
Attachment #8686879 -
Flags: review?(bugmail.mozilla) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8686885 [details] [diff] [review] (part 4) - Make {Get,Set}NonClientMargins() return/take a LayoutDeviceIntMargin Review of attachment 8686885 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsIWidget.h @@ +882,2 @@ > */ > + NS_IMETHOD GetNonClientMargins(mozilla::LayoutDeviceIntMargin &margins) = 0; Since we're going to be having a lot of mozilla::foo types in this file it might be worth sticking in a using statement or typedef somewhere. Probably too much work for this patch series but if you're touching these files again in another patchset it's something to consider. ::: widget/windows/nsWindow.h @@ +197,5 @@ > virtual nsIMEUpdatePreference GetIMEUpdatePreference(); > + NS_IMETHOD GetNonClientMargins( > + mozilla::LayoutDeviceIntMargin &margins); > + NS_IMETHOD SetNonClientMargins( > + mozilla::LayoutDeviceIntMargin &margins); Add |override| to these
Attachment #8686885 -
Flags: review?(bugmail.mozilla) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8686887 [details] [diff] [review] (part 5) - Make OnDefaultButtonLoaded() return/take a LayoutDeviceIntMargin Review of attachment 8686887 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsBaseWidget.h @@ +225,5 @@ > void* aCallbackData) override { return false; } > virtual bool ComputeShouldAccelerate(); > NS_IMETHOD GetToggledKeyState(uint32_t aKeyCode, bool* aLEDState) override { return NS_ERROR_NOT_IMPLEMENTED; } > virtual nsIMEUpdatePreference GetIMEUpdatePreference() override { return nsIMEUpdatePreference(); } > + NS_IMETHOD OnDefaultButtonLoaded(const mozilla::LayoutDeviceIntRect &aButtonRect) override { return NS_ERROR_NOT_IMPLEMENTED; } move the & before the space (pre-existing style nit, but might as well fix it) ::: widget/nsIWidget.h @@ +1865,5 @@ > /* > * Call this method when a dialog is opened which has a default button. > * The button's rectangle should be supplied in aButtonRect. > + */ > + NS_IMETHOD OnDefaultButtonLoaded(const mozilla::LayoutDeviceIntRect &aButtonRect) = 0; move the & ::: widget/windows/nsWindow.cpp @@ +3548,5 @@ > * > **************************************************************/ > > NS_IMETHODIMP > +nsWindow::OnDefaultButtonLoaded(const LayoutDeviceIntRect &aButtonRect) move the & ::: widget/windows/nsWindow.h @@ +155,5 @@ > virtual LayerManager* GetLayerManager(PLayerTransactionChild* aShadowManager = nullptr, > LayersBackend aBackendHint = mozilla::layers::LayersBackend::LAYERS_NONE, > LayerManagerPersistence aPersistence = LAYER_MANAGER_CURRENT, > bool* aAllowRetaining = nullptr); > + NS_IMETHOD OnDefaultButtonLoaded(const mozilla::LayoutDeviceIntRect &aButtonRect); move the &, add |override|
Attachment #8686887 -
Flags: review?(bugmail.mozilla) → review+
Updated•9 years ago
|
Attachment #8686890 -
Flags: review?(bugmail.mozilla) → review+
Updated•9 years ago
|
Attachment #8686892 -
Flags: review?(bugmail.mozilla) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8686893 [details] [diff] [review] (part 8) - Use LayoutDeviceIntMargin more in HyperTextAccessible Review of attachment 8686893 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/generic/HyperTextAccessible.cpp @@ +1519,4 @@ > rect.MoveBy(offset); > > + LayoutDeviceIntRect caretRect; > + caretRect = LayoutDeviceIntRect::FromUnknownRect( I would have combined the assignment with the declaration
Attachment #8686893 -
Flags: review?(bugmail.mozilla) → review+
Updated•9 years ago
|
Attachment #8686894 -
Flags: review?(bugmail.mozilla) → review+
Comment 20•9 years ago
|
||
Comment on attachment 8686895 [details] [diff] [review] (part 10) - Remove a GetNatrualBoundsUntyped() calls Review of attachment 8686895 [details] [diff] [review]: ----------------------------------------------------------------- s/Natrual/Natural/ in the commit message. Also s/calls/call/ :)
Attachment #8686895 -
Flags: review?(bugmail.mozilla) → review+
Comment 21•9 years ago
|
||
Comment on attachment 8686896 [details] [diff] [review] (part 11) - Remove a GetBoundsUntyped() call Review of attachment 8686896 [details] [diff] [review]: ----------------------------------------------------------------- Out of curiosity, did you build the qt widget to check compilation? I wouldn't even know how to go about doing that. ::: widget/qt/nsWindow.cpp @@ +1370,5 @@ > } > > void > +nsWindow::DispatchResizeEvent(LayoutDeviceIntRect &aRect, > + nsEventStatus &aStatus) move the &s ::: widget/qt/nsWindow.h @@ +178,5 @@ > void DispatchDeactivateEvent(void); > void DispatchActivateEventOnTopLevelWindow(void); > void DispatchDeactivateEventOnTopLevelWindow(void); > + void DispatchResizeEvent(mozilla::LayoutDeviceIntRect &aRect, > + nsEventStatus &aStatus); move the &s
Attachment #8686896 -
Flags: review?(bugmail.mozilla) → review+
Comment 22•9 years ago
|
||
Comment on attachment 8686898 [details] [diff] [review] (part 12) - Remove WidgetToScreenOffsetUntyped() Review of attachment 8686898 [details] [diff] [review]: ----------------------------------------------------------------- Thanks so much for doing this! This was a really nice patchset and the code is much better as a result too :) As for the "screen coordinates" referred to in the comment, I believe that refers to a coordinate system on the physical display which is probably distinct from all the other typed coordinate systems that we have. I'm not really sure if we should call it something else but we can leave it as-is for now.
Attachment #8686898 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 23•9 years ago
|
||
> > + LayoutDeviceIntRect caretRect; > > + caretRect = LayoutDeviceIntRect::FromUnknownRect( > > I would have combined the assignment with the declaration Me too, but it causes a compile error. Something about a temporary not being allowed in that position. > Out of curiosity, did you build the qt widget to check compilation? I > wouldn't even know how to go about doing that. No. If I was doing this on cross-platform code I'd lean heavily on the compiler to find all the places that need changing. But because lots of this widget code isn't built by default (Qt, iOS, maybe others) I've been using a grep-heavy approach in an attempt to minimize the breakage of those non-default platforms. The results with the Windows-specific code have been pretty good so far. More patches coming, BTW, but I'll do them in another bug.
Assignee | ||
Comment 24•9 years ago
|
||
I forgot to say: thank you for the super-fast reviews.
Assignee | ||
Comment 25•9 years ago
|
||
One question: do you understand the units in nsBaseWidget::GetScaledScreenBounds()? AFAICT it should return a LayoutDeviceIntRect, and the GetScreenBoundsUntyped() call inside it also gives a LayoutDeviceIntRect, but there's a CSSToLayoutDeviceScale used to convert between those two. So I'm confused about that one.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bugmail.mozilla)
Comment 26•9 years ago
|
||
AFAICT GetScaledScreenBounds should return a CSSIntRect. At least the ScreenForRect function looks like it's expecting CSS pixel values (e.g. [1]). [1] http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsScreenManagerGtk.cpp?rev=e8c7dfe727cd#240
Flags: needinfo?(bugmail.mozilla)
Comment 27•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #23) > > > + LayoutDeviceIntRect caretRect; > > > + caretRect = LayoutDeviceIntRect::FromUnknownRect( > > > > I would have combined the assignment with the declaration > > Me too, but it causes a compile error. Something about a temporary not being > allowed in that position. Curious. What compiler version are you using? It compiles fine for me with clang 3.5.
Assignee | ||
Comment 28•9 years ago
|
||
> Curious. What compiler version are you using? It compiles fine for me with
> clang 3.5.
My mistake; I mixed this one up with a different case. Combining here works fine.
Assignee | ||
Comment 29•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c6d723b1b2a74dd26cfbabbd8d50eba7c698ee Bug 1224403 (part 1) - Make Configuration::mBounds a LayoutDeviceIntRect. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/cfde91a29f0357a661370fb2742b8d209856a76d Bug 1224403 (part 2) - Split GetClientOffset() into typed and untyped versions. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/b89c50c056eb4375ed5c779816fee7075eacb96f Bug 1224403 (part 3) - Make mNonClientOffset a LayoutDeviceIntMargin. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/42eb9ee261f8f26db7b1176cded83093ec150ab4 Bug 1224403 (part 4) - Make {Get,Set}NonClientMargins() return/take a LayoutDeviceIntMargin. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/28e01a5a581945e7e448c1243f0c6bc3ff974685 Bug 1224403 (part 5) - Make OnDefaultButtonLoaded() return/take a LayoutDeviceIntMargin. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/6c83f9b94af271ce17f3f4c7ec31317a9029dd87 Bug 1224403 (part 6) - Remove one WidgetToScreenOffsetUntyped() call. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/c62ae26405f4577d022a80fad1b1f131e8493f58 Bug 1224403 (part 7) - Use LayoutDeviceIntMargin more in IMMHandler. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bddc993c53e9beefdd7995dfd195576e54faf2 Bug 1224403 (part 8) - Use LayoutDeviceIntMargin more in HyperTextAccessible. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/2b32dd022204978ced3dc039b432caf387f295de Bug 1224403 (part 9) - Remove GetRestoredBoundsUntyped(). r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/3bf56b21189d3443b935eaf1993a389492701f14 Bug 1224403 (part 10) - Make nsScreenGonk::GetNaturalBoundsUntyped() typed. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/f6561880170a45f40c429b9df8bd2cc98b671f82 Bug 1224403 (part 11) - Remove a GetBoundsUntyped() call. r=kats. https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f961230ac369373b03693e837372c1d0f6d589 Bug 1224403 (part 12) - Remove WidgetToScreenOffsetUntyped(). r=kats.
Comment 30•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c8c6d723b1b2 https://hg.mozilla.org/mozilla-central/rev/cfde91a29f03 https://hg.mozilla.org/mozilla-central/rev/b89c50c056eb https://hg.mozilla.org/mozilla-central/rev/42eb9ee261f8 https://hg.mozilla.org/mozilla-central/rev/28e01a5a5819 https://hg.mozilla.org/mozilla-central/rev/6c83f9b94af2 https://hg.mozilla.org/mozilla-central/rev/c62ae26405f4 https://hg.mozilla.org/mozilla-central/rev/c7bddc993c53 https://hg.mozilla.org/mozilla-central/rev/2b32dd022204 https://hg.mozilla.org/mozilla-central/rev/3bf56b21189d https://hg.mozilla.org/mozilla-central/rev/f6561880170a https://hg.mozilla.org/mozilla-central/rev/a7f961230ac3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•