Closed Bug 815591 Opened 11 years ago Closed 10 years ago

Avoid building unnecessary nsDisplayBackgroundColors

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
18 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Tracking Flags:
Tracking Status
b2g18 --- fixed

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(2 files)

fix
1.74 KB, patch
mattwoodrow
: review+
akeybl
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
Disable failing reftests
1.77 KB, patch
bajaj
: approval-mozilla-b2g18+
Details | Diff | Splinter Review 
      No description provided.
Attached patch fix — — Splinter Review 

        Attachment #685584 -
        Flags: review?(matt.woodrow)

    
    

    
  
Comment on attachment 685584 [details] [diff] [review]
fix

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

Weird, I have the exact same patch in my b2g tree.

I think we can also skip building the nsDisplayBackgroundImage if bg->mLayers[i].mImage.IsEmpty().

        Attachment #685584 -
        Flags: review?(matt.woodrow) → review+

    
    

    
  
The backout also fixed Windows mochitest-4 failures of the form:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17441404&tree=Mozilla-Inbound
{
16615 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug450930.xhtml | Didn't get subdoc invalidation while we were privileged (iframe2) 
}

    
    

    
  
The backout also seemed to have fixed Talos regressions:
http://mzl.la/VfEIP2
http://mzl.la/VfI84c
I don't understand it at all!

    
    

    
  
Well, the fact that you're branching on an uninitialized |color| isn't great -- you should probably put drawBackgroundColor ? color : NS_RGBA(0, 0, 0, 0) in a variable and test that.

It doesn't explain failures/regressions, though.

    
    

    
  
This Mac reftest failure is still happening.

    
    

    
  
Mac reftest failure looks like a gradient rasterization issue. This patch has reduced the bounds of some display items and maybe some temporary surfaces causing the gradient to get rendered slightly differently after invalidation. I'll fuzz it.

    
    

    
  
The Tp results look OK now.

    
    

    
  
Since I just fuzzed the test, I'll reland.

    
    

    
  
Er, scratch that. Let me wait for the extra mochitest-4 runs that I triggered to come back.

    
    

    
  
Those try results still look good. I'll reland this when the tree isn't burning.

    
    

    
  
Apparently you didn't fuzz the OSX reftests enough, because they're still failing. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8f5013110e

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e941e0df729c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

    
  
Depends on: 846144

    
    

    
  
Comment on attachment 685584 [details] [diff] [review]
fix

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: failure to get tiny improvement in scrolling/rendering performance (see bug 846901)
Testing completed: been on trunk for a while
Risk to taking this patch (and alternatives if risky): pretty low risk given trunk landing. Will need to uplift regression fix in bug 846144 as well.
String or UUID changes made by this patch: none.

        Attachment #685584 -
        Flags: approval-mozilla-b2g18?

    
    

    
  
Comment on attachment 685584 [details] [diff] [review]
fix

Approving for scrolling perf win.

        Attachment #685584 -
        Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

    
    

    
  
Backed out for reftest failures. And a reminder, the tree rules still state that people who push to release branches are expected to watch and star their pushes...
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7508c5a1026b

    
    

    
  
Landing this patch on b2g18 causes a couple of reftest failures because of bug 828146, which is not fixed on b2g18.

    
    

    
  
Landing bug 828146 on the b2g18 branch causes some new test failures. So I think the best solution is to just disable the failing tests on the b2g18 branch:
gfx/tests/reftest/709477-1.html
layout/reftests/scrolling/opacity-mixed-scrolling-2.html

(Note that bug 828146 only affects elements using -moz-appearance which we don't actually use on B2G.)

    
    

    
  
Comment on attachment 728808 [details] [diff] [review]
Disable failing reftests

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): existing bug revealed by patch for bug 815591
User impact if declined: won't be able to fix bug 815591 on b2g18
Testing completed: none
Risk to taking this patch (and alternatives if risky): No risk, just disabling tests
String or UUID changes made by this patch: none

        Attachment #728808 -
        Flags: approval-mozilla-b2g18?

        Attachment #728808 -
        Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

    
  
Depends on: 922228

          You need to log in
          before you can comment on or make changes to this bug.