694706 - Get rid of checkerboard for remote viewport

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Oleg Romashin (:romaxa)]

I think we should remove checkerboard code, because it just make rendering slower, does not make any sense, and because of checkerboard enabled we rendering slower and that makes checkerboard more visible....
Also it make better tab switching with bug 693930 fixed

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Remove checkerboard

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 567219 [details] [diff] [review]
Remove checkerboard

This should be reviewed by someone else (I assume a Layout peer because it touches code in /layout).

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not wedded to the checkerboard (in fact I don't like it very much either), but this is a UI design question.

UX folks: the reason we added the checkerboard in the first place is to give a visual indicator of movement while we're asynchronously scrolling and waiting for content to repaint.  The checkerboard is drawn in the areas of the screen that we're waiting for content to repaint.  What's a better way for us to more subtly indicate scrolling?  ISTR that newer iOSes do something that takes the page's background color into consideration.

Then we can horse-trade over perf tradeoffs; this is a very critical hot path.

Comment 4

[Chris Lord [:cwiiis]]

The moving scroll-indicator ought to serve well enough to show that scrolling is happening, and checker-boarding is infrequent and small enough that it's a rare circumstance to have the entire page as a checker-board.

For reference, Android >=3.01 just draws the background colour. Not sure what iOS does now...

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The scroll indicator works well on small pages, but not large ones.  Using the background colo(u)r is an option, and a performant one.

Comment 6

[Alex Limi (:limi) — Firefox UX Team]

Yeah, my instinct is to just pick up the page's background color.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

I was thinking pages's background color plus a very light dashed grid.  Conveying that the surface isn't intentionally blank or broken is important, but my main concern with the checkerboard is that it's visually really strong, especially when you have your phone close to your face in an entirely dark room.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

>checker-boarding is infrequent and small enough that it's a rare circumstance to have the >entire page as a checker-board

I'm seeing the checkerboard really often in beta.  Better in nightly?

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Extract default page background color and share it to UI process

roc Is it ok to shared default page background color using Widget Background color API?

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: Extract default page background color and share it to UI process

Forgot puppetWidget background color forward to UI process

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Remove checkerboard and replace with default page bgcolor layer

Ok, here is pretty straight replacement of checkerboard image with simple color layer which gets color value from remote layout.

This just works and looks good

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: Remove checkerboard and replace with default page bgcolor layer

Ok, I think it make sense just send new color directly to TabChild... and don't change widget background behavior

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: linux.org.ru zoom out screenshoot

Here you can see background color (darker area around content) which you will see when you zoom out quickly (before there are was checkerboard)

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: engadget.com (white background)

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 567564 [details] [diff] [review]
Remove checkerboard and replace with default page bgcolor layer

This implementation looks mostly OK, but ui-r and tests are needed.  r- for no tests.

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: Remove checkerboard and replace with default page bgcolor layer

Added reftest

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 567648 [details]
linux.org.ru zoom out screenshoot

I tested this patch on a device and it looks good.  The UX team here in Toronto gave the go-ahead.  Let's get this into Nightly and see how it works.

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Disable fixed pos test for remote browser, bug 656167. part2

Found some weird problem with test-bg-attachment-fixed.html (which IIUC supposed to be disabled for remote browser, but for now it somehow magically passes tests)
If I'm adding new test before that, then it start permanently failing... and works only with MOZ_ENABLE_FIXED_POSITION_LAYERS=1
Suggesting to mark that test as fail, until MOZ_POS_FIXED_LAYERS enabled, or maybe disable it to same as test above, or skip it only for remote browser

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 567684 [details] [diff] [review]
Remove checkerboard and replace with default page bgcolor layer

In general, need to decide between nscolor and gfxRGBA here.  I don't
care which you use, just be consistent.  roc, do you have a
preference?

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp

> bool
>+TabParent::RecvSetBackgroundColor(const PRUint32& aColor)
>+{
>+  nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>+  if (frameLoader) {

  if (nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader())

>+    RenderFrameParent* frame = frameLoader->GetCurrentRemoteFrame();
>+    if (frame) {

  if (RenderFrameParent* frame = frameLoader->GetCurrentRemoteFrame())

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp

>+      TabChild* tabChild = GetTabChildFrom(this);
>+      if (tabChild) {

  if (TabChild* tabChild = GetTabChildFrom(this))

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp

>+static void
>+BuildBackgroundPatternFor(ContainerLayer* aContainer,
>+                          ContainerLayer* aShadowRoot,
>+                          const ViewConfig& aConfig,
>+                          const gfxRGBA& aColor,
>+                          LayerManager* aManager,
>+                          nsIFrame* aFrame)
>+{

What does moving this function accomplish?  A lot of the code looks
the same.  You'll get a smaller patch and cleaner blame by keeping it
in place.

>+  // The origin of the tiling plane, top-left of the tile source rect,
>+  // is at layer-space point <0,0>.  Set up a translation from that
>+  // origin to the frame top-left, with the little nudge included.

There's no tiling anymore.

>diff --git a/layout/reftests/reftest-sanity/test-displayport-bg.html b/layout/reftests/reftest-sanity/test-displayport-bg.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/reftest-sanity/test-displayport-bg.html
>@@ -0,0 +1,7 @@
>+<!DOCTYPE HTML>
>+<html reftest-viewport-w="100" reftest-viewport-h="100"
>+      reftest-displayport-w="400" reftest-displayport-h="500"
>+      reftest-async-scroll=true>
>+<body bgcolor=green style="position: absolute; left:0px; top:0px;width:800px; height:1000px;"></body>
>+</html>
>+

Where's the reference file?

>diff --git a/layout/tools/reftest/reftest.js b/layout/tools/reftest/reftest.js

>+function RecvEnableAsyncScroll()
>+{
>+    gBrowser.QueryInterface(CI.nsIFrameLoaderOwner).frameLoader.renderMode =
>+        CI.nsIFrameLoader.RENDER_MODE_ASYNC_SCROLL
>+}
>+

You need to turn off async scroll mode when the test is over.  If it's
not turned off, a tremendous number of tests will fail.  Did you send
this patch through try?

r- for reftest harness breakage.  Need to fix that and decide on
nscolor vs. gfxRGBA for the next patch.

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Remove checkerboard and replace with default page bgcolor layer

Layout operate with nscolor, layers with gfxRGBA...
it does not make sense to convert nscolor to gfxRGBA on every BuildBackgroundPattern call, or UpdateBackgroundColor
point of current implementation is - send just uint32 via IPC, and convert it once when it needed.
In current implementation also fixed problem with color update when we navigate through SHistory..

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: Remove checkerboard and replace with default page bgcolor layer

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 568168 [details] [diff] [review]
Remove checkerboard and replace with default page bgcolor layer

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl

>+    SetBackgroundColor(PRUint32 color);

nscolor.  Marking all of the ones I see explicitly because they'll
pass the type checker without being changed.

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>+void
>+TabChild::SetBackgroundColor(const PRUint32& aColor)

nscolor

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h

>+    void SetBackgroundColor(const PRUint32& aColor);

nscolor

>+    PRUint32 mLastBackgroundColor;

nscolor

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp

> bool
>+TabParent::RecvSetBackgroundColor(const PRUint32& aColor)

nscolor

>diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h

>+    virtual bool RecvSetBackgroundColor(const PRUint32& aValue);

nscolor

>diff --git a/layout/ipc/RenderFrameParent.h b/layout/ipc/RenderFrameParent.h

>+  gfxRGBA mBackgroundColor;

Add a comment that this is gfxRGBA because that's what ColorLayer
wants.

>diff --git a/layout/tools/reftest/reftest.js b/layout/tools/reftest/reftest.js

> function RecvTestDone(runtimeMs)
> {
>     RecordResult(runtimeMs, '', [ ]);
>+    SetAsyncScroll(false);

Do this in FinishTestItem().  Otherwise you'll miss cases like
timeouts.

r=me with those changes.

Comment 23

[Oleg Romashin (:romaxa)]

Attachment: Remove checkerboard and replace with default page bgcolor layer. to Push

Fixed comments

Comment 24

[Oleg Romashin (:romaxa)]

Comment on attachment 567763 [details] [diff] [review]
Disable fixed pos test for remote browser, bug 656167. part2

Ok, sounds like this is working fine with updated reftest change

Comment 25

[Oleg Romashin (:romaxa)]

Comment on attachment 568467 [details] [diff] [review]
Remove checkerboard and replace with default page bgcolor layer. to Push

Try build
https://tbpl.mozilla.org/?tree=Try&rev=6b68911ad2af

Comment 26

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36d12bbf310a

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/36d12bbf310a

Comment 28

[Andreea Pod]

Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:10.0a1) Gecko/20111104 Firefox/10.0a1 Fennec/10.0a1
Device: Samsung Galaxy S II (Android 2.3.4)

Comment 29

[Oleg Romashin (:romaxa)]

Comment on attachment 568467 [details] [diff] [review]
Remove checkerboard and replace with default page bgcolor layer. to Push

this bug is blocking 693930#c39 (checkerboard issue), should we approve this too?

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 568467 [details] [diff] [review]
Remove checkerboard and replace with default page bgcolor layer. to Push

this made the aurora merge over, approving for beta. Please get it landed quickly

Comment 31

[LegNeato]

This doesn't seem to apply cleanly to beta. Please land it yourself as soon as possible.

Comment 32

[Matt Brubeck (:mbrubeck)]

This already landed on beta but wasn't marked as fixed:
http://hg.mozilla.org/releases/mozilla-beta/rev/0ffb24f7c34a