Closed Bug 1224403 Opened 4 years ago Closed 4 years ago

Use LayoutDevicePixel type more in widget/

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: njn, Assigned: njn)

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/.
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.
This required adding {To,From}UnknownMargin().
Attachment #8686885 - Flags: review?(bugmail.mozilla)
Attachment #8686894 - Flags: review?(bugmail.mozilla)
Attachment #8686896 - Flags: review?(bugmail.mozilla)
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 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 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 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 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 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+
Attachment #8686890 - Flags: review?(bugmail.mozilla) → review+
Attachment #8686892 - Flags: review?(bugmail.mozilla) → review+
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+
Attachment #8686894 - Flags: review?(bugmail.mozilla) → review+
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 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 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+
> > +  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.
I forgot to say: thank you for the super-fast reviews.
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.
Flags: needinfo?(bugmail.mozilla)
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)
(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.
> 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.
Blocks: 1224482
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.
You need to log in before you can comment on or make changes to this bug.