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

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

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.

Comment 1

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a3acefa1f548

Comment 2

[Nick Cameron [:nrc]]

Attachment: patch to disable mask layers on Android

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Nick Cameron [:nrc]]

Attachment: patch to disable mask layers on Android

Comment 5

[Joe Drew (not getting mail)]

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

Comment 6

[Nick Cameron [:nrc]]

Attachment: rebased patch

rebased; carrying r=roc

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5020492b65

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9a5020492b65

Comment 9

[Chris Lord [:cwiiis]]

Er, just to point out that this doesn't actually disable mask layers for android because MOZ_ENABLE_MASK_LAYERS is always defined.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Nick Cameron [:nrc]]

(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.

Comment 12

[Nick Cameron [:nrc]]

(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.

Comment 13

[Chris Lord [:cwiiis]]

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?

Comment 14

[Nick Cameron [:nrc]]

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.

Comment 15

[Nick Cameron [:nrc]]

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...

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 17

[Nick Cameron [:nrc]]

Attachment: backout patch

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why aren't we WONTFIXing this? 716439 is all good right?

Comment 19

[Nick Cameron [:nrc]]

The new patch is just to backout the changes in the previous patch, then we can WONTFIX it.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 629093 [details] [diff] [review]
backout patch

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

bingo

Comment 21

[Nick Cameron [:nrc]]

checkin-needed for the second (backout) patch to backout the first patch which landed some time ago.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/819b358de00e

Sorry for the delay...

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/819b358de00e

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

Comment 24

[Nick Cameron [:nrc]]

All changes in have been backed out, the reason for having this in the first place is fixed, so marking as WONTFIX