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

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
7 years ago
7 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)

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.
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.
Attachment #624205 - Attachment is obsolete: true
Attachment #624205 - Flags: review?(roc)
Attachment #624248 - Flags: review?(roc)
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
Posted patch rebased patchSplinter Review
rebased; carrying r=roc
Attachment #624248 - Attachment is obsolete: true
Attachment #624578 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5020492b65
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/9a5020492b65
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
(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
Closed: 7 years ago7 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
(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?
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 → ---
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.
Posted patch backout patchSplinter Review
Attachment #629093 - Flags: review?(roc)
Why aren't we WONTFIXing this? 716439 is all good right?
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+
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)
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
Closed: 7 years ago7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.