Closed Bug 820246 Opened 8 years ago Closed 8 years ago

Split nsDisplayCanvasBackground into separate color/image items.

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file, 1 obsolete file)

Sorry bjacob, decided we wanted this done asap.

Also ignore the move of ShouldFixToViewport, i've reverted that change locally.
Sorry, I have been more busy than expected with B2G, security and WebGL bugs that I couldn't de-prioritize. But I still would like very much to try some layout bug. If you can think of another one, please let me know, and sorry that I didn't actually do this one!
Comment on attachment 690700 [details] [diff] [review]
Split them!

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

This will need to be rebased over bug 818643

::: layout/base/nsDisplayList.h
@@ +1875,5 @@
> +    // compositing layer. Since we know their background painting area can't
> +    // change (unless the viewport size itself changes), async scrolling
> +    // will work well.
> +    return mFrame->GetType() == nsGkAtoms::canvasFrame && mBackgroundStyle &&
> +      mBackgroundStyle->mLayers[mLayer].mAttachment == NS_STYLE_BG_ATTACHMENT_FIXED &&

Reorder these to avoid the GetType() call when possible.

But why not just put this on nsDisplayCanvasBackgroundImage?

::: layout/generic/nsCanvasFrame.h
@@ +121,5 @@
>   * PaintBackground, covering the whole overflow area.
>   * We can also paint an "extra background color" behind the normal
>   * background.
>   */
> +class nsDisplayCanvasBackground : public nsDisplayItem {

nsDisplayCanvasBackgroundColor
(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Also ignore the move of ShouldFixToViewport, i've reverted that change
> locally.

OK. But I'd like to see the new patch.
Attached patch Split v2Splinter Review
Fixed review comments and tidied a few things up.

Passes all but a single reftest on android:

https://tbpl.mozilla.org/?tree=Try&rev=9f7187eb2e74
Attachment #690700 - Attachment is obsolete: true
Attachment #690700 - Flags: review?(roc)
Attachment #691233 - Flags: review?(roc)
Comment on attachment 691233 [details] [diff] [review]
Split v2

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

::: layout/generic/nsCanvasFrame.h
@@ +201,4 @@
>   
>    // We still need to paint a background color as well as an image for this item, 
>    // so we can't support this yet.
> +  virtual bool SupportsOptimizingToImage() MOZ_OVERRIDE { return false; }

Remove this comment. In fact, I think in a separate patch you should allow this to return true.
Attachment #691233 - Flags: review?(roc) → review+
Depends on: 821233
https://hg.mozilla.org/mozilla-central/rev/afc699fc7e65
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 832341
Assignee: nobody → matt.woodrow
You need to log in before you can comment on or make changes to this bug.