Be able to disable mask layers for Android or backout mask layers

RESOLVED WONTFIX

Status

()

Core
Graphics: Layers
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

15 Branch
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
We need to be able to disable mask layers on Android as a temporary fix for bug 753784. Either we turn off or backout the patches from bug 716439. Hopefully we have a proper fix, but if not, we need this before v15 goes to Aurora.
Blocks: 716439
No longer depends on: 716439
(Assignee)

Comment 2

6 years ago
Created attachment 624205 [details] [diff] [review]
patch to disable mask layers on Android
Attachment #624205 - Flags: review?(roc)
Comment on attachment 624205 [details] [diff] [review]
patch to disable mask layers on Android

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

Let's have a new #define MOZ_ENABLE_MASK_LAYERS (say in Layers.h) and then have all these #ifdefs key off that. Then we can land this. If/when we want to turn things off for Android, we can wrap that define in #ifdef ANDROID.
(Assignee)

Comment 4

6 years ago
Created attachment 624248 [details] [diff] [review]
patch to disable mask layers on Android
Attachment #624205 - Attachment is obsolete: true
Attachment #624205 - Flags: review?(roc)
Attachment #624248 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
applying disable.patch
patching file layout/base/FrameLayerBuilder.cpp
Hunk #2 FAILED at 1192
1 out of 7 hunks FAILED -- saving rejects to file layout/base/FrameLayerBuilder.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh disable.patch
Keywords: checkin-needed
(Assignee)

Comment 6

6 years ago
Created attachment 624578 [details] [diff] [review]
rebased patch

rebased; carrying r=roc
Attachment #624248 - Attachment is obsolete: true
Attachment #624578 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5020492b65
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/9a5020492b65
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 9

6 years ago
Er, just to point out that this doesn't actually disable mask layers for android because MOZ_ENABLE_MASK_LAYERS is always defined.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is why commit message that describe what a patch is doing are preferred instead of leaving it to the person landing the patch to try to infer it. Note that the posted patch's commit message was just the bug title.
(Assignee)

Comment 11

6 years ago
(In reply to Chris Lord [:cwiiis] from comment #9)
> Er, just to point out that this doesn't actually disable mask layers for
> android because MOZ_ENABLE_MASK_LAYERS is always defined.

Sorry, the bug is badly named (due to a change of plan half way through), this bug makes it possible to disable mask layers, in case we can't fix the bug on mobile. If that happens, then we just need to guard the MOZ_ENABLE_MASK_LAYERS define with ifndef ANDROID. But I'm hopeful I'll have a proper fix soon.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Summary: Disable mask layers for Android or backout mask layers → Be able to disable mask layers for Android or backout mask layers
(Assignee)

Comment 12

6 years ago
(In reply to Ryan VanderMeulen from comment #10)
> This is why commit message that describe what a patch is doing are preferred
> instead of leaving it to the person landing the patch to try to infer it.
> Note that the posted patch's commit message was just the bug title.

Sorry about that, the commit message was originally right, but the plan changed with comment 3 and I forgot to update the commit message, my mistake.
I'm still not sure we can mark this as fixed - when I remove the define, invalidation of masked areas doesn't work correctly on http://people.mozilla.org/~mwargers/tests/layout/white_stripes_scrolling_iframe.html - has this been tested?
(Assignee)

Comment 14

6 years ago
Chris - how was it broken? The same as with mask layers or some different way? Do you see rounded corners where there should be some?

I did test with the two pages you posted, and it worked fine, although there was some tiling delays, which I assumed was normal - I saw similar on debug builds pre-mask layers patches on many pages. I also had one crash, but could not reproduce it, so assumed it was a fluke. I tested thoroughly on Windows to make sure mask layers were not being used, and not so thoroughly on my phone to check it worked (there is some level of assumption here, I know). But I don't have a lot of confidence in my Android testing (in general) yet.

Reopening until we confirm that it works.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

6 years ago
OK, I've managed to spoof enough that I can reproduce a problem with the white stripes scrolling test page (but not the green rounded rects test page, is that right?). I'll try to fix...
Status: REOPENED → NEW
Target Milestone: mozilla15 → ---
Given we have a fix for bug 753784 and no other blocking-type regressions from bug 716439, I think once we've safety landed the fix for bug 753784 we should simply WONTFIX this bug.
(Assignee)

Comment 17

6 years ago
Created attachment 629093 [details] [diff] [review]
backout patch
Attachment #629093 - Flags: review?(roc)
Why aren't we WONTFIXing this? 716439 is all good right?
(Assignee)

Comment 19

6 years ago
The new patch is just to backout the changes in the previous patch, then we can WONTFIX it.
Comment on attachment 629093 [details] [diff] [review]
backout patch

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

bingo
Attachment #629093 - Flags: review?(roc) → review+
(Assignee)

Comment 21

6 years ago
checkin-needed for the second (backout) patch to backout the first patch which landed some time ago.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/819b358de00e

Sorry for the delay...
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/819b358de00e

(Not sure if this is "fixed" or not at this point, so leaving open)
(Assignee)

Comment 24

6 years ago
All changes in have been backed out, the reason for having this in the first place is fixed, so marking as WONTFIX
Status: NEW → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.