Get rid of checkerboard for remote viewport

VERIFIED FIXED in Firefox 9

Status

defect
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
Firefox 10
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox9 fixed)

Details

Attachments

(3 attachments, 9 obsolete attachments)

54.82 KB, image/png
mbrubeck
: ui-review+
Details
56.22 KB, image/png
Details
21.58 KB, patch
Details | Diff | Splinter Review
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
Posted patch Remove checkerboard (obsolete) — Splinter Review
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #567219 - Flags: review?(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).
Attachment #567219 - Flags: review?(mbrubeck)
Attachment #567219 - Flags: review?(jones.chris.g)
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.
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...
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.
Yeah, my instinct is to just pick up the page's background color.
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.
>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?
roc Is it ok to shared default page background color using Widget Background color API?
Attachment #567495 - Flags: feedback?(roc)
Forgot puppetWidget background color forward to UI process
Attachment #567495 - Attachment is obsolete: true
Attachment #567495 - Flags: feedback?(roc)
Attachment #567497 - Flags: feedback?(roc)
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
Attachment #567219 - Attachment is obsolete: true
Attachment #567219 - Flags: review?(jones.chris.g)
Attachment #567552 - Flags: review?(jones.chris.g)
Ok, I think it make sense just send new color directly to TabChild... and don't change widget background behavior
Attachment #567497 - Attachment is obsolete: true
Attachment #567552 - Attachment is obsolete: true
Attachment #567497 - Flags: feedback?(roc)
Attachment #567552 - Flags: review?(jones.chris.g)
Attachment #567564 - Flags: review?(jones.chris.g)
Attachment #567564 - Flags: feedback?(roc)
Here you can see background color (darker area around content) which you will see when you zoom out quickly (before there are was checkerboard)
Attachment #567648 - Flags: ui-review?(mbrubeck)
Attachment #567564 - Flags: ui-review?(mbrubeck)
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.
Attachment #567564 - Flags: review?(jones.chris.g) → review-
Added reftest
Attachment #567564 - Attachment is obsolete: true
Attachment #567564 - Flags: ui-review?(mbrubeck)
Attachment #567684 - Flags: ui-review?(mbrubeck)
Attachment #567684 - Flags: review?(jones.chris.g)
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.
Attachment #567648 - Flags: ui-review?(mbrubeck) → ui-review+
Attachment #567684 - Flags: ui-review?(mbrubeck) → ui-review+
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
Attachment #567763 - Flags: review?(roc)
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.
Attachment #567684 - Flags: review?(jones.chris.g) → review-
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..
Attachment #567684 - Attachment is obsolete: true
Attachment #568167 - Attachment is obsolete: true
Attachment #568168 - Flags: review?(jones.chris.g)
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.
Attachment #568168 - Flags: review?(jones.chris.g) → review+
Attachment #568467 - Attachment description: Remove checkerboard and replace with default page bgcolor layer → Remove checkerboard and replace with default page bgcolor layer. part1
Attachment #567763 - Attachment description: Remove checkerboard and replace with default page bgcolor layer → Disable fixed pos test for remote browser, bug 656167. part2
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
Attachment #567763 - Attachment is obsolete: true
Keywords: checkin-needed
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
Attachment #568467 - Attachment description: Remove checkerboard and replace with default page bgcolor layer. part1 → Remove checkerboard and replace with default page bgcolor layer. to Push
https://hg.mozilla.org/mozilla-central/rev/36d12bbf310a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
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)
Status: RESOLVED → VERIFIED
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?
Attachment #568467 - Flags: approval-mozilla-aurora?
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
Attachment #568467 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
This doesn't seem to apply cleanly to beta. Please land it yourself as soon as possible.
This already landed on beta but wasn't marked as fixed:
http://hg.mozilla.org/releases/mozilla-beta/rev/0ffb24f7c34a
You need to log in before you can comment on or make changes to this bug.