1070710 - Don't use private API _regionForOpaqueDescendants for window dragging

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Markus Stange [:mstange]]

I tried this just to see what it would look like, and it doesn't look all that bad to me. I also haven't found any problems caused by the additional NSViews.

Comment 1

[Markus Stange [:mstange]]

Attachment: part 1: Pull NSView region management out of VibrancyManager

Comment 2

[Markus Stange [:mstange]]

Attachment: part 2: Use ViewRegion for window dragging

Comment 3

[Steven Michaud [:smichaud] (Retired)]

As I mentioned in another bug, a big pile of [something] has just been dumped on my plate.  So it'll be a while before I get to this.  Later this week?  Next week?

Fortunately bug 1070038 takes care of the immediate problem.

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1070710 - Add mozilla::ViewRegion which assembles a LayoutDeviceIntRegion as NSViews.

Review commit: https://reviewboard.mozilla.org/r/63550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63550/

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1070710 - Use ViewRegion for vibrant areas in VibrancyManager.

Review commit: https://reviewboard.mozilla.org/r/63552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63552/

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1070710 - Use ViewRegion for window dragging.

Review commit: https://reviewboard.mozilla.org/r/63554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63554/

Comment 7

[Markus Stange [:mstange]]

This fixes bug 1284859.

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8769855 [details]
Bug 1070710 - Add mozilla::ViewRegion which assembles a LayoutDeviceIntRegion as NSViews.

https://reviewboard.mozilla.org/r/63550/#review61170

::: widget/cocoa/ViewRegion.h:32
(Diff revision 1)
> +   * Update the region.
> +   * @param aRegion  The new region.
> +   * @param aCoordinateConverter  The nsChildView to use for converting
> +   *   LayoutDeviceIntRect device pixel coordinates into Cocoa NSRect coordinates.
> +   * @param aContainerView  The view that's going to be the superview of the
> +   *   NSViews which will be created for this regions.

nit: s/regions/region/

::: widget/cocoa/ViewRegion.h:34
(Diff revision 1)
> +   * @param aCoordinateConverter  The nsChildView to use for converting
> +   *   LayoutDeviceIntRect device pixel coordinates into Cocoa NSRect coordinates.
> +   * @param aContainerView  The view that's going to be the superview of the
> +   *   NSViews which will be created for this regions.
> +   * @param aViewCreationCallback  A block that instantiates new NSViews.
> +   * @return  Whether the region changed.

nit: s/Whether/Whether or not/

Comment 9

[Stephen A Pohl [:spohl]]

Comment on attachment 8769857 [details]
Bug 1070710 - Use ViewRegion for window dragging.

https://reviewboard.mozilla.org/r/63554/#review61166

Comment 10

[Stephen A Pohl [:spohl]]

Comment on attachment 8769856 [details]
Bug 1070710 - Use ViewRegion for vibrant areas in VibrancyManager.

https://reviewboard.mozilla.org/r/63552/#review61168

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa570844827
Add mozilla::ViewRegion which assembles a LayoutDeviceIntRegion as NSViews. r=spohl
https://hg.mozilla.org/integration/mozilla-inbound/rev/c786f596561c
Use ViewRegion for vibrant areas in VibrancyManager. r=spohl
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b56f47ac04
Use ViewRegion for window dragging. r=spohl

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fa570844827
https://hg.mozilla.org/mozilla-central/rev/c786f596561c
https://hg.mozilla.org/mozilla-central/rev/d8b56f47ac04

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8769855 [details]
Bug 1070710 - Add mozilla::ViewRegion which assembles a LayoutDeviceIntRegion as NSViews.

Approval Request Comment
[Feature/regressing bug #]: macOS 10.12 Sierra (+ bug 944836)
[User impact if declined]: The user will not be able to drag windows on 10.12 starting with Firefox version 49 or in Gecko-based apps that are not Firefox, due to Apple's "checkfix", see bug 1284859 comment 11.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low to medium, but this patch set has been on Nightly for more than a week and people drag windows all the time, so they probably would already have found the problems if there were any.
[String/UUID change made/needed]: none

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8769855 [details]
Bug 1070710 - Add mozilla::ViewRegion which assembles a LayoutDeviceIntRegion as NSViews.

Improve mac os 10.12 support, taking it.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8ff1d96126d
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ffed1cedd42
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1d798dd3579

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=3146538&repo=mozilla-aurora#L4961

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a2331a741876542f14ad82f4c5264d70f98c832

Comment 17

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=205a750474b2

Comment 18

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5ba9b9ef7dd

Comment 19

[Markus Stange [:mstange]]

Attachment: rollup patch for beta

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8777483 [details] [diff] [review]
rollup patch for beta

Approval Request Comment: See comment 13. This was approved for aurora but failed to land before the merge to beta.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8777483 [details] [diff] [review]
rollup patch for beta

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

Per comment #14, improve mac os 10.12 support, take it in 49 beta.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1a9c61ffe9ab

Comment 23

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I investigated this bug on latest Aurora 50.0a2 (2016-09-01) and 49.0b8 build1 (20160829102229), using Mac OS X 10.12. 
The fix looks good, I didn't encountered any windows dragging related problems. Instead there is another Sierra specific issue (and I'm not sure if it related with this bug or not): if you have an active tab with a playing youtube video and minimize this window, you will observe some visual issues when restoring it. Please confirm if this issue needs to be filed separately.

Comment 24

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Filed separately Bug 1303011 for the above specified issue (comment 23).
Based on these, I will set the corresponding flags.