Closed Bug 1070710 Opened 10 years ago Closed 8 years ago

Don't use private API _regionForOpaqueDescendants for window dragging

Categories

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

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- verified

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Whiteboard: tpi:+)

Attachments

(4 files, 2 obsolete files)

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.
Attachment #8492740 - Flags: review?(smichaud)
Attachment #8492741 - Flags: review?(smichaud)
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.
Blocks: 1284859
Review commit: https://reviewboard.mozilla.org/r/63550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63550/
Attachment #8769855 - Flags: review?(spohl.mozilla.bugs)
Attachment #8769856 - Flags: review?(spohl.mozilla.bugs)
Attachment #8769857 - Flags: review?(spohl.mozilla.bugs)
Attachment #8492740 - Attachment is obsolete: true
Attachment #8492740 - Flags: review?(smichaud)
Attachment #8492741 - Attachment is obsolete: true
Attachment #8492741 - Flags: review?(smichaud)
This fixes bug 1284859.
Priority: -- → P1
Whiteboard: tpi:+
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/
Attachment #8769855 - Flags: review?(spohl.mozilla.bugs) → review+
Attachment #8769857 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8769856 [details]
Bug 1070710 - Use ViewRegion for vibrant areas in VibrancyManager.

https://reviewboard.mozilla.org/r/63552/#review61168
Attachment #8769856 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed by mstange@themasta.com:
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
https://hg.mozilla.org/mozilla-central/rev/4fa570844827
https://hg.mozilla.org/mozilla-central/rev/c786f596561c
https://hg.mozilla.org/mozilla-central/rev/d8b56f47ac04
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Attachment #8769855 - Flags: approval-mozilla-aurora?
Attachment #8769856 - Flags: approval-mozilla-aurora?
Attachment #8769857 - Flags: approval-mozilla-aurora?
Comment on attachment 8769855 [details]
Bug 1070710 - Add mozilla::ViewRegion which assembles a LayoutDeviceIntRegion as NSViews.

Improve mac os 10.12 support, taking it.
Attachment #8769855 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8769856 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8769857 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(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.
Attachment #8777483 - Flags: approval-mozilla-beta?
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.
Attachment #8777483 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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.
Filed separately Bug 1303011 for the above specified issue (comment 23).
Based on these, I will set the corresponding flags.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mstange)
Blocks: 1306825
Blocks: 1333121
You need to log in before you can comment on or make changes to this bug.