Closed Bug 805939 Opened 12 years ago Closed 12 years ago

White flash upon orientation change in gallery

Categories

(Firefox OS Graveyard :: General, defect, P4)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18+ fixed)

Status:
RESOLVED FIXED
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 + fixed

People

(Reporter: overholt, Assigned: chiajung)

References

Details

(Keywords: b2g-testdriver, unagi, Whiteboard: UX-P?)

Attachments

(2 files, 9 obsolete files)

Patch V9
25.34 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Patch V9-Fix Windows Compilation
26.20 KB, patch
Details | Diff | Splinter Review 
STR:

1. Ensure you have a few photo thumbnails visible in your gallery
2. Rotate the phone from a vertical orientation to a horizontal orientation

Expected:

smooth rotation of the gallery app

Actual:

~half the screen flashes white when the orientation changes

Potentially related:  bug #796762

Video of behaviour:  http://youtu.be/JgpvqpOUOMA
Jet, can someone from Layout help determine what's causing this?
blocking-basecamp: ? → +
Priority: -- → P3
Whiteboard: [soft]
I found the problem is not restrict to Gallery application. I can reproduce similar issue in browser application, too.
Assignee: nobody → chung
After some debugging and code tracing, I think the problem is some sync problem between chrome process and content process:

When screen rotates, chrome process triggers dimension update, and then make content process reflow its content. But chrome process DO not wait content process complete its work and trigger a force repaint in nsWindow::Resize. So chrome process may use old data to do a redraw and cause the problem.
Attached patch Proposed patch (obsolete) — — Splinter Review 
This patch fix the out-of-sync composition problem which cause the flash screen occurred in Gallary and Browser apps. 

Bt the way, this patch would make the SPS problem become more noticeable and I think it should be another problem.
Attachment #680515 - Flags: review?(jones.chris.g)
I would like to describe more detail about the problem I mentioned in previous post for clarity.

Originally, there are 2 strange frame when screen rotated:
  1. A broken frame with the original frame duplicated 3 times.
  2. A scaled(Browser case) or a white(Gallery case) frame

The first problem is not always shown and this patch fixes the second one. As a result, the first problem would become easier to be observed.

* NOTE: SPS means Spike-Polygon-Syndrome. I use this word here since I can not find a more clear and compact word to describe it.
Forgot to mention that I am investigating the SPS problem mentioned previously now.
By the way, Android has the similar issue. It can be eaiser to observe by turn the Animation off.
Attached patch Patch Version 2 (obsolete) — — Splinter Review 
After some investigation the SPS is introduced by race condition between Compositor thread and Painting(main) thread of chrome process. (The white flash was introduced by race condition between content process and chrome process)

It can be fixed by simply set the sentinel value early then reflow occurred.
Attachment #680531 - Flags: review?(jones.chris.g)
There are a few things we need to do here

 - fix the race condition here http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#952, which will cause the compositor to read the transformed widget bounds in a racy way.  This will lead to artifacts regardless of what we do here.

 - extend TargetConfig [1] with "orientation" member.  This is different than the "rotation" member.

 - set the orientation value for all forwarded layers transactions

 - in CompositorParent, remember the last TargetConfig for each subprocess transaction [2]

 - when we resolve the ref layers [3], check that the orientation value for each remote subtree matches the orientation for the top-level tree

 - if there's a mismatched orientation, bail out of the composition

 - ... but set a timer for X seconds (maybe start with X=1) in the future to recomposite anyway.  We can't block the compositor indefinitely on subprocesses repainting after orientation changes.

I think this is where your version 2 patch here is heading.

Let me know if this makes sense.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/PLayers.ipdl#39
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1303
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#549
(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> There are a few things we need to do here
> 
>  - fix the race condition here
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/
> LayerManagerOGL.cpp#952, which will cause the compositor to read the
> transformed widget bounds in a racy way.  This will lead to artifacts
> regardless of what we do here.
I tried to log the rect size after WorldTransform here, and find the size is always (320, 480) no matter the screen orientation. I think the size should be framebuffer dependent rather than viewport dependent. Correct me if I am wrong :)

> 
>  - extend TargetConfig [1] with "orientation" member.  This is different
> than the "rotation" member.
> 
>  - set the orientation value for all forwarded layers transactions
> 
>  - in CompositorParent, remember the last TargetConfig for each subprocess
> transaction [2]
> 
>  - when we resolve the ref layers [3], check that the orientation value for
> each remote subtree matches the orientation for the top-level tree
> 
>  - if there's a mismatched orientation, bail out of the composition
I will try this solution later. In fact, I think the idea is the same with mine, but implement it this way should be better. It avoid static values and handle the orientation in a more clear way.
> 
>  - ... but set a timer for X seconds (maybe start with X=1) in the future to
> recomposite anyway.  We can't block the compositor indefinitely on
> subprocesses repainting after orientation changes.
I don't know how long it should be, but I have to agree with you. Yesterday, I noticed that if we start an application, rotate the device and then if the application died during rotation, we can never composite a new frame later...

> 
> I think this is where your version 2 patch here is heading.
> 
> Let me know if this makes sense.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/PLayers.ipdl#39
> [2]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp#1303
> [3]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> CompositorParent.cpp#549
Hmm...I think the child process does not know anything about screen rotation. Child process just update its window dimensions and paint its buffer, then  CompositorParent calculates the rotation matrix for its children somehow.

It may be infeasible to send the orientation from child to parent. I am trying to figure out how to match the rotation from Parent side...
Also, please note again that screen *rotation* is different than screen *orientation*.  We can only attempt to match up orientation.

The screen *rotation* is a physical property of the screen itself.  The value is meaningless outside of the compositor in the master process.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> You want http://mxr.mozilla.org/mozilla-central/source/hal/Hal.h#389 .

This function should triggers an IPC to get the parent orientation which means it should always the same between parent/child process. 

NOTE: Since a feature or a bug of the Hal.cpp implementaion, the child side ScreenConfiguration::orientation() always return 1, no matter what orientation  value the parent is. And if you remove the cache check of http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#243 then the value should always the same thus can not used to compare the orientation)
(In reply to Chiajung Hung [:chiajung] from comment #15)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > You want http://mxr.mozilla.org/mozilla-central/source/hal/Hal.h#389 .
> 
> This function should triggers an IPC to get the parent orientation which
> means it should always the same between parent/child process. 
> 

There's only a message sent on the first request.  After that the value is cached and only updated when changed.

> NOTE: Since a feature or a bug of the Hal.cpp implementaion, the child side
> ScreenConfiguration::orientation() always return 1, no matter what
> orientation  value the parent is. And if you remove the cache check of
> http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#243 then the value
> should always the same thus can not used to compare the orientation)

That's a very bad bug!  It affects DOM APIs.  Are you sure about that?  If so, please file a separate bug.

But you're correct that this implementation will still be vulnerable to a race condition if the child process repaints after the cached orientation is updated, but before its dimensions are updated.  A better fix is to send the current parent orientation down with

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#294

and stamp layers transactions with that orientation.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> (In reply to Chiajung Hung [:chiajung] from comment #15)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > > You want http://mxr.mozilla.org/mozilla-central/source/hal/Hal.h#389 .
> > 
> > This function should triggers an IPC to get the parent orientation which
> > means it should always the same between parent/child process. 
> > 
> 
> There's only a message sent on the first request.  After that the value is
> cached and only updated when changed.
> 
> > NOTE: Since a feature or a bug of the Hal.cpp implementaion, the child side
> > ScreenConfiguration::orientation() always return 1, no matter what
> > orientation  value the parent is. And if you remove the cache check of
> > http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#243 then the value
> > should always the same thus can not used to compare the orientation)
> 
> That's a very bad bug!  It affects DOM APIs.  Are you sure about that?  If
> so, please file a separate bug.
I found it strange from log. After discuss with Steven Lee, we found that if we do not register an observer from child process, the cached value is never updated. This make the global hal::GetCurrentScreenConfiguration confusing. Since we can not get the newest value from here if we do not register observer. 

We do not confirm whether it is wanted behaviour. We will discuss with others before file it :)
> 
> But you're correct that this implementation will still be vulnerable to a
> race condition if the child process repaints after the cached orientation is
> updated, but before its dimensions are updated.  A better fix is to send the
> current parent orientation down with
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#294
> 
> and stamp layers transactions with that orientation.
I will try it later, and I think this would be a longer path to be checked. By the way, I think UpdateDimension would be called when window shrink. I think it should be better to add UpdateOrientation but this would affect the API...
(In reply to Chiajung Hung [:chiajung] from comment #17)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > (In reply to Chiajung Hung [:chiajung] from comment #15)
> > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > > > You want http://mxr.mozilla.org/mozilla-central/source/hal/Hal.h#389 .
> > > 
> > > This function should triggers an IPC to get the parent orientation which
> > > means it should always the same between parent/child process. 
> > > 
> > 
> > There's only a message sent on the first request.  After that the value is
> > cached and only updated when changed.
> > 
> > > NOTE: Since a feature or a bug of the Hal.cpp implementaion, the child side
> > > ScreenConfiguration::orientation() always return 1, no matter what
> > > orientation  value the parent is. And if you remove the cache check of
> > > http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#243 then the value
> > > should always the same thus can not used to compare the orientation)
> > 
> > That's a very bad bug!  It affects DOM APIs.  Are you sure about that?  If
> > so, please file a separate bug.
> I found it strange from log. After discuss with Steven Lee, we found that if
> we do not register an observer from child process, the cached value is never
> updated. This make the global hal::GetCurrentScreenConfiguration confusing.
> Since we can not get the newest value from here if we do not register
> observer. 
> 
> We do not confirm whether it is wanted behaviour. We will discuss with
> others before file it :)

Oh, I thought the DOM always installed an orientation listener.  If that only happens on demand, then the behavior you're seeing makes sense.

> > 
> > But you're correct that this implementation will still be vulnerable to a
> > race condition if the child process repaints after the cached orientation is
> > updated, but before its dimensions are updated.  A better fix is to send the
> > current parent orientation down with
> > 
> > http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#294
> > 
> > and stamp layers transactions with that orientation.
> I will try it later, and I think this would be a longer path to be checked.
> By the way, I think UpdateDimension would be called when window shrink. I
> think it should be better to add UpdateOrientation but this would affect the
> API...

I'm not sure I understand this comment.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> (In reply to Chiajung Hung [:chiajung] from comment #17)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > > (In reply to Chiajung Hung [:chiajung] from comment #15)
> > > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > > > > You want http://mxr.mozilla.org/mozilla-central/source/hal/Hal.h#389 .
> > > > 
> > > > This function should triggers an IPC to get the parent orientation which
> > > > means it should always the same between parent/child process. 
> > > > 
> > > 
> > > There's only a message sent on the first request.  After that the value is
> > > cached and only updated when changed.
> > > 
> > > > NOTE: Since a feature or a bug of the Hal.cpp implementaion, the child side
> > > > ScreenConfiguration::orientation() always return 1, no matter what
> > > > orientation  value the parent is. And if you remove the cache check of
> > > > http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#243 then the value
> > > > should always the same thus can not used to compare the orientation)
> > > 
> > > That's a very bad bug!  It affects DOM APIs.  Are you sure about that?  If
> > > so, please file a separate bug.
> > I found it strange from log. After discuss with Steven Lee, we found that if
> > we do not register an observer from child process, the cached value is never
> > updated. This make the global hal::GetCurrentScreenConfiguration confusing.
> > Since we can not get the newest value from here if we do not register
> > observer. 
> > 
> > We do not confirm whether it is wanted behaviour. We will discuss with
> > others before file it :)
> 
> Oh, I thought the DOM always installed an orientation listener.  If that
> only happens on demand, then the behavior you're seeing makes sense.
> 
> > > 
> > > But you're correct that this implementation will still be vulnerable to a
> > > race condition if the child process repaints after the cached orientation is
> > > updated, but before its dimensions are updated.  A better fix is to send the
> > > current parent orientation down with
> > > 
> > > http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#294
> > > 
> > > and stamp layers transactions with that orientation.
> > I will try it later, and I think this would be a longer path to be checked.
> > By the way, I think UpdateDimension would be called when window shrink. I
> > think it should be better to add UpdateOrientation but this would affect the
> > API...
> 
> I'm not sure I understand this comment.

I mean I don't know whether embed the screen orientation related code in UpdateDimension affect window shrink condition. And I am not sure whether it is worthy to add a new API for screen orientation. Anyway, I am just talk to myself :p
(In reply to Chiajung Hung [:chiajung] from comment #19)
> I mean I don't know whether embed the screen orientation related code in
> UpdateDimension affect window shrink condition. And I am not sure whether it
> is worthy to add a new API for screen orientation. Anyway, I am just talk to
> myself :p

Well, the problem here is that compositor needs a way to (i) atomically request a repaint for a given <dimensions, orientation> configuration and (ii) know when that repaint has finished.  I think maybe you're pointing out the problem that |orientation| can change, but |dimensions| might remain the same.  Hm, that's an annoying one.  (For example, with a 180 degree orientation change.)

I /think/ that any orientation change should trigger a reflow.  If that's not the case, we can certainly guarantee that.  I also /think/ that all reflows should end up calling UpdateDimensions(), but I'm less sure about that.  If that's not the case already, it would be hard to guarantee.  And we probably shouldn't rely on that.

If we can't do this 100% correctly, I think a reasonable compromise is to have TabChild listen for ScreenConfiguration changes and then set a timer waiting for the UpdateDimensions() message.  If the timer expires, TabChild would force a repaint of its content.  We'd want this timer to be something like 1/2 the amount of time CompositorParent waits before recompositing with a mismatched layer tree.

Btw, all this complexity is why I haven't fixed this bug already :/.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> (In reply to Chiajung Hung [:chiajung] from comment #17)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> > > (In reply to Chiajung Hung [:chiajung] from comment #15)
> > > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> > > > > You want http://mxr.mozilla.org/mozilla-central/source/hal/Hal.h#389 .
> > > > 
> > > > This function should triggers an IPC to get the parent orientation which
> > > > means it should always the same between parent/child process. 
> > > > 
> > > 
> > > There's only a message sent on the first request.  After that the value is
> > > cached and only updated when changed.
> > > 
> > > > NOTE: Since a feature or a bug of the Hal.cpp implementaion, the child side
> > > > ScreenConfiguration::orientation() always return 1, no matter what
> > > > orientation  value the parent is. And if you remove the cache check of
> > > > http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#243 then the value
> > > > should always the same thus can not used to compare the orientation)
> > > 
> > > That's a very bad bug!  It affects DOM APIs.  Are you sure about that?  If
> > > so, please file a separate bug.
> > I found it strange from log. After discuss with Steven Lee, we found that if
> > we do not register an observer from child process, the cached value is never
> > updated. This make the global hal::GetCurrentScreenConfiguration confusing.
> > Since we can not get the newest value from here if we do not register
> > observer. 
> > 
> > We do not confirm whether it is wanted behaviour. We will discuss with
> > others before file it :)
> 
> Oh, I thought the DOM always installed an orientation listener.  If that
> only happens on demand, then the behavior you're seeing makes sense.
> 
I don't think so :p
If the DOM always install a listener for orientation, the cache there should be updated as the listener notified with new orientation, isn't it?

The screen orientation changed DOM event must be propagated other way but I don't know how. 
> > > 
> > > But you're correct that this implementation will still be vulnerable to a
> > > race condition if the child process repaints after the cached orientation is
> > > updated, but before its dimensions are updated.  A better fix is to send the
> > > current parent orientation down with
> > > 
> > > http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#294
> > > 
> > > and stamp layers transactions with that orientation.
> > I will try it later, and I think this would be a longer path to be checked.
> > By the way, I think UpdateDimension would be called when window shrink. I
> > think it should be better to add UpdateOrientation but this would affect the
> > API...
> 
> I'm not sure I understand this comment.
The DOM orientation API probably only sets up a hal observer when content registers to listen for the orientation-change DOM events.  That would explain what you see.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> The DOM orientation API probably only sets up a hal observer when content
> registers to listen for the orientation-change DOM events.  That would
> explain what you see.

If it is true, then I think http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#243 should check whether an observer registered. Otherwise the value returned is always wrong...
Attached patch [WIP] Version 3 (obsolete) — — Splinter Review 
Here is a new patch based on Chris's suggestion. Basically, it has 2 parts:
1. LayerManagerOGL: Avoid composition if the layer size mismatches widget size (This is the true cause of SPS)
2. CompositorParent: Avoid composition if content layer trees orientation mismatch chrome layer tree.

This patch needs a major clean up, but I want to upload it here and discuss furthermore base on this.

After discusses with Thinker, I think if the widget dimension of content process matches the expected dimension, then composition should be safe. No matter what orientation it is. 

There is a naturalBounds member which seems encode some related information. I would like to write a new patch to see if it feasible :)
Attachment #680515 - Attachment is obsolete: true
Attachment #680531 - Attachment is obsolete: true
Attachment #680515 - Flags: review?(jones.chris.g)
Attachment #680531 - Flags: review?(jones.chris.g)
Attachment #684280 - Flags: review?(jones.chris.g)
(In reply to Chiajung Hung [:chiajung] from comment #24)
> Created attachment 684280 [details] [diff] [review]
> [WIP] Version 3
> 
> Here is a new patch based on Chris's suggestion. Basically, it has 2 parts:
> 1. LayerManagerOGL: Avoid composition if the layer size mismatches widget
> size (This is the true cause of SPS)
> 2. CompositorParent: Avoid composition if content layer trees orientation
> mismatch chrome layer tree.
> 
> This patch needs a major clean up, but I want to upload it here and discuss
> furthermore base on this.
> 
> After discusses with Thinker, I think if the widget dimension of content
> process matches the expected dimension, then composition should be safe. No
> matter what orientation it is. 
> 
> There is a naturalBounds member which seems encode some related information.
> I would like to write a new patch to see if it feasible :)

Hmm...If I check the TargetConfig.natureBounds() size with aLayer->GetEffectiveVisibleRegion().GetBounds() when resolve the RefLayers, the performance becomes very bad :S

It seems the GetBounds() call is costly.
I find that hard to believe since it's implemented like this:
const nsRect& GetBounds () const { return mBoundRect; }
Attached patch Test patch (obsolete) — — Splinter Review 
This patch is what I tested. 

The diff from previous patch is:

diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp
index 64b492b..237ecfb 100644
--- a/gfx/layers/ipc/CompositorParent.cpp
+++ b/gfx/layers/ipc/CompositorParent.cpp
@@ -492,11 +492,16 @@ private:
         /*__android_log_print(ANDROID_LOG_INFO, "CompositorParent", "target bound(%d, %d), layer(%d, %d)", 
                state->mTargetConfig.naturalBounds().width, state->mTargetConfig.naturalBounds().height, aLayer->GetClipRect()->width, aLayer->GetClipRect()->height);*/
         if (!ref->GetEffectiveVisibleRegion().IsEmpty()) {
-          hal::ScreenConfiguration currentConfig;
+          /*hal::ScreenConfiguration currentConfig;
           hal::GetCurrentScreenConfiguration(&currentConfig);
           if (state->mTargetConfig.orientation() != currentConfig.orientation()) {
             //__android_log_print(ANDROID_LOG_INFO, "CompositorParent", "main tree orientation: %d, tree %lld orientation: %d", currentConfig.orientation(), ref->GetReferentId(), state->mTargetConfig.orie
             mChecked = false;
+          }*/
+          nsIntRect layerRect  = aLayer->GetEffectiveVisibleRegion().GetBounds();
+          const nsIntRect& widgetRect = state->mTargetConfig.naturalBounds();
+          if (layerRect.width != widgetRect.width || layerRect.height != widgetRect.height) {
+            mChecked = false;
           }
         }

And the homescreen scroll response time is really bad. Maybe I just skip too much composition this way.
If I change the 

nsIntRect layerRect  = aLayer->GetEffectiveVisibleRegion().GetBounds();
to 
const nsIntRect* layerRect  = aLayer->GetClipRect();

the slowdown gone. However, I suspect this kill window transition animation but it not. :P
(In reply to Chiajung Hung [:chiajung] from comment #24)
> Created attachment 684280 [details] [diff] [review]
> [WIP] Version 3
> 
> Here is a new patch based on Chris's suggestion. Basically, it has 2 parts:
> 1. LayerManagerOGL: Avoid composition if the layer size mismatches widget
> size (This is the true cause of SPS)

I don't understand what this means.  Maybe it'll be clearer in the patch :).

> After discusses with Thinker, I think if the widget dimension of content
> process matches the expected dimension, then composition should be safe. No
> matter what orientation it is. 

Hm, I don't understand this either.  What are the "expected dimensions"?  What does "safe" mean?

> There is a naturalBounds member which seems encode some related information.
> I would like to write a new patch to see if it feasible :)

The natural bounds are only meaningful for the top-level nsIWidget that's drawing to the hardware framebuffer.  Those bounds never change.  They don't have any meaning for any other widget.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> (In reply to Chiajung Hung [:chiajung] from comment #24)
> > Created attachment 684280 [details] [diff] [review]
> > [WIP] Version 3
> > 
> > Here is a new patch based on Chris's suggestion. Basically, it has 2 parts:
> > 1. LayerManagerOGL: Avoid composition if the layer size mismatches widget
> > size (This is the true cause of SPS)
> 
> I don't understand what this means.  Maybe it'll be clearer in the patch :).
I mean this one: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#952

This line will cause LayerManagerOGL render to a widget size respects to current orientation, but the reflow may not complete.

I am searching for a better way to handle this, and workaround it here in the patch.

> 
> > After discusses with Thinker, I think if the widget dimension of content
> > process matches the expected dimension, then composition should be safe. No
> > matter what orientation it is. 
> 
> Hm, I don't understand this either.  What are the "expected dimensions"? 
> What does "safe" mean?
Hmm, let me try my best to explain it :p

When orientation changed, chrome process will do reflow and send new dimensions for each content processes. These sizes are so called "expected dimensions" in my previous comment.

When TabChild::RecvUpdateDimensions called, content process /*may*/ doing reflow base on new dimensions. If no reflow occured since the new dimensions are same as the old dimensions, then the layer would not updated (e.g 180 degree case). However, we /*think*/ the layer would not be painted differently when the orientation is different. That means we think the layer content is the same when orientation is 0/180 degree. Then the orientation /*should be*/ applied when doing composition and controlled by Compositor. 

If we are right, then composite a layer with dimension same as the "expected" one should not introduce any artifact. Since the buffer do not need reflow/repaint, using an old buffer is /*safe*/.

Since I am not good at explain my thought...if there is still any confusion please just point out. I will try my best to let you understand.

> 
> > There is a naturalBounds member which seems encode some related information.
> > I would like to write a new patch to see if it feasible :)
> 
> The natural bounds are only meaningful for the top-level nsIWidget that's
> drawing to the hardware framebuffer.  Those bounds never change.  They don't
> have any meaning for any other widget.

The natural bounds are updated when orientation changed in my experiment, so I expected it should be the "widget" size updated in TabChild::UpdateDimensions. Maybe I was wrong again...

I think this may hard to be implemented after some test. Because CompositorParent do not know the "expected dimensions" of each layer trees. To infer the "expected dimensions" from the layer tree is infeasible since the layer maybe larger than widget. And I think it may kill the layer scaling animation. Since it do not update the widget size but the layer size...

As a result, I will clean up the patch(version 3) to be reviewed.
Attached patch Patch V4 (obsolete) — — Splinter Review 
Patch version 4. 

This is cleaned up version of v3.

NOTE: I do not sure whether there are any better way to check widget dimension against layer tree. Any idea?
Attachment #684280 - Attachment is obsolete: true
Attachment #684318 - Attachment is obsolete: true
Attachment #684280 - Flags: review?(jones.chris.g)
Attachment #684560 - Flags: review?(jones.chris.g)
(In reply to Chiajung Hung [:chiajung] from comment #30)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> > (In reply to Chiajung Hung [:chiajung] from comment #24)
> > > Created attachment 684280 [details] [diff] [review]
> > > [WIP] Version 3
> > > 
> > > Here is a new patch based on Chris's suggestion. Basically, it has 2 parts:
> > > 1. LayerManagerOGL: Avoid composition if the layer size mismatches widget
> > > size (This is the true cause of SPS)
> > 
> > I don't understand what this means.  Maybe it'll be clearer in the patch :).
> I mean this one:
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/
> LayerManagerOGL.cpp#952
> 
> This line will cause LayerManagerOGL render to a widget size respects to
> current orientation, but the reflow may not complete.
> 
> I am searching for a better way to handle this, and workaround it here in
> the patch.

I'm not quite sure how your patch is handling this yet, but this doesn't sound right to me.  I suspect you're running into the race condition I mentioned in comment 10.  The right way to fix that is to fix the race condition ;). 

> > > After discusses with Thinker, I think if the widget dimension of content
> > > process matches the expected dimension, then composition should be safe. No
> > > matter what orientation it is. 
> > 
> > Hm, I don't understand this either.  What are the "expected dimensions"? 
> > What does "safe" mean?
> Hmm, let me try my best to explain it :p
> 
> When orientation changed, chrome process will do reflow and send new
> dimensions for each content processes. These sizes are so called "expected
> dimensions" in my previous comment.
> 
> When TabChild::RecvUpdateDimensions called, content process /*may*/ doing
> reflow base on new dimensions. If no reflow occured since the new dimensions
> are same as the old dimensions, then the layer would not updated (e.g 180
> degree case). However, we /*think*/ the layer would not be painted
> differently when the orientation is different. That means we think the layer
> content is the same when orientation is 0/180 degree. Then the orientation
> /*should be*/ applied when doing composition and controlled by Compositor. 
> 
> If we are right, then composite a layer with dimension same as the
> "expected" one should not introduce any artifact. Since the buffer do not
> need reflow/repaint, using an old buffer is /*safe*/.
> 
> Since I am not good at explain my thought...if there is still any confusion
> please just point out. I will try my best to let you understand.
> 

This makes sense, but doesn't sound like a solution to the problem.  I think we're discussing the same issue as in comment 20.

> > 
> > > There is a naturalBounds member which seems encode some related information.
> > > I would like to write a new patch to see if it feasible :)
> > 
> > The natural bounds are only meaningful for the top-level nsIWidget that's
> > drawing to the hardware framebuffer.  Those bounds never change.  They don't
> > have any meaning for any other widget.
> 
> The natural bounds are updated when orientation changed in my experiment, so
> I expected it should be the "widget" size updated in
> TabChild::UpdateDimensions. Maybe I was wrong again...
> 
> I think this may hard to be implemented after some test. Because
> CompositorParent do not know the "expected dimensions" of each layer trees.
> To infer the "expected dimensions" from the layer tree is infeasible since
> the layer maybe larger than widget. And I think it may kill the layer
> scaling animation. Since it do not update the widget size but the layer
> size...
> 
> As a result, I will clean up the patch(version 3) to be reviewed.

Yes, these are all reasons why we need to check the expected *orientation* ;).

Sorry for the review lag (US holidays), looking as soon as I can today.
Since this is fairly complex and there's not a summary comment describing the approach here, let me write down my mental model for how this code should work.  This is what I'll be reviewing the patch against.

 - in CompositorParent, the top-level TargetConfig's orientation value is always "right".  This is because it's only ever updated atomically in a PLayers transaction from the main thread to the compositor thread.  So the top-level layer tree will never "veto" a composition because of orientation mismatch.

 - when the orientation of the top-level nsIWidget on the master-process main thread changes, then the following things happen

  1. For all remote subtrees that might be visible, TabParent::UpdateDimensions() is called
  2. TabParent sends the new dimensions and orientation atomically to TabChild
  3. TabChild::RecvUpdateDimensions() always forces a repaint if the orientation has changed
  4. Content processes always send the most recent orientation value received in TabChild::RecvUpdateDimensions() atomically in TargetConfig along with layers transactions

 - in CompositorParent, if a sub-tree's TargetConfig's orientation value doesn't match the top-level TargetConfig's orientation value, then the subtree "vetos" the current composition.  Remember, the top-level TargetConfig's value is always "right".

 - ... except, if compositions have been vetoed longer than X seconds in CompositorParent, then we go ahead and composite anyway
Comment on attachment 684560 [details] [diff] [review]
Patch V4

>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
>index e1ae01b..644904c 100644
>--- a/dom/ipc/PBrowser.ipdl
>+++ b/dom/ipc/PBrowser.ipdl
>@@ -291,7 +291,7 @@ child:
> 
>     LoadURL(nsCString uri);
> 
>-    UpdateDimensions(nsRect rect, nsIntSize size) compress;
>+    UpdateDimensions(nsRect rect, nsIntSize size, uint32_t orientation) compress;

Please use the |ScreenOrientation| type instead of |uint32_t| here.

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

>+#include "android/log.h"

This code won't compile on non-ANDROID platforms.  Please remove this
#include and the logging statements.

(For testing patches, it's easier to use printf_stderr(), which goes
to logcat on ANDROID platforms.)

> #define BROWSER_ELEMENT_CHILD_SCRIPT \
>     NS_LITERAL_STRING("chrome://global/content/BrowserElementChild.js")
>@@ -1095,7 +1096,7 @@ TabChild::RecvShow(const nsIntSize& size)
> }
> 
> bool
>-TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size)
>+TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size, const uint32_t& orientation)
> {
>     if (!mRemoteFrame) {
>         return true;
>@@ -1106,6 +1107,7 @@ TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size)
>     mOuterRect.width = rect.width;
>     mOuterRect.height = rect.height;
> 
>+    mOrientation = orientation;
>     mInnerSize = size;
>     mWidget->Resize(0, 0, size.width, size.height,
>                     true);
>@@ -1114,6 +1116,8 @@ TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size)
>     baseWin->SetPositionAndSize(0, 0, size.width, size.height,
>                                 true);
> 
>+    __android_log_print(ANDROID_LOG_INFO, "TabChild", "Update widget size to: (%d, %d)", size.width, size.height);
>+
>     HandlePossibleViewportChange();
> 
>     return true;
>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h
>index 589978e..ece9fa8 100644
>--- a/dom/ipc/TabChild.h
>+++ b/dom/ipc/TabChild.h
>@@ -196,7 +196,7 @@ public:
> 
>     virtual bool RecvLoadURL(const nsCString& uri);
>     virtual bool RecvShow(const nsIntSize& size);
>-    virtual bool RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size);
>+    virtual bool RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size, const uint32_t& orientation);
>     virtual bool RecvUpdateFrame(const mozilla::layers::FrameMetrics& aFrameMetrics);
>     virtual bool RecvHandleDoubleTap(const nsIntPoint& aPoint);
>     virtual bool RecvHandleSingleTap(const nsIntPoint& aPoint);
>@@ -290,6 +290,8 @@ public:
>     /** Return the DPI of the widget this TabChild draws to. */
>     void GetDPI(float* aDPI);
> 
>+    uint32_t GetOrientation() { return mOrientation; }
>+
>     void SetBackgroundColor(const nscolor& aColor);
> 
>     void NotifyPainted();
>@@ -405,6 +407,7 @@ private:
>     bool mNotified;
>     bool mContentDocumentIsDisplayed;
>     bool mTriedBrowserInit;
>+    uint32_t mOrientation;
> 
>     DISALLOW_EVIL_CONSTRUCTORS(TabChild);
> };

>diff --git a/gfx/layers/Makefile.in b/gfx/layers/Makefile.in
>index 3ea24a4..9457e24 100644
>--- a/gfx/layers/Makefile.in
>+++ b/gfx/layers/Makefile.in
>@@ -194,6 +194,9 @@ include $(topsrcdir)/ipc/chromium/chromium-config.mk
> 
> LOCAL_INCLUDES += \
>         -I$(topsrcdir)/content/events/src \
>+        -I$(topsrcdir)/hal \
>+        -I$(topsrcdir)/widget/xpwidgets \
>+        -I$(topsrcdir)/widget/gonk \
>         -I$(ANDROID_SOURCE)/frameworks/base/include/media/stagefright \
>         -I$(ANDROID_SOURCE)/frameworks/base/include/media/stagefright/openmax \
>         $(NULL)
>diff --git a/gfx/layers/basic/BasicLayerManager.cpp b/gfx/layers/basic/BasicLayerManager.cpp
>index 947638b..68672ef 100644
>--- a/gfx/layers/basic/BasicLayerManager.cpp
>+++ b/gfx/layers/basic/BasicLayerManager.cpp
>@@ -15,6 +15,7 @@
> #include "nsXULAppAPI.h"
> #include "RenderTrace.h"
> #include "sampler.h"
>+#include "Hal.h"
> 
> #define PIXMAN_DONT_DEFINE_STDINT
> #include "pixman.h"
>@@ -31,6 +32,8 @@
> #include "AndroidBridge.h"
> #endif
> 
>+#include "android/log.h"
>+

Remember to remove these too.

> using namespace mozilla::dom;
> using namespace mozilla::gfx;
> 
>@@ -1097,7 +1100,18 @@ BasicShadowLayerManager::BeginTransactionWithTarget(gfxContext* aTarget)
>   // don't signal a new transaction to ShadowLayerForwarder. Carry on adding
>   // to the previous transaction.
>   if (HasShadowManager()) {
>-    ShadowLayerForwarder::BeginTransaction(mTargetBounds, mTargetRotation);
>+    uint32_t orientation;
>+    if (GeckoProcessType_Content == XRE_GetProcessType()) {
>+      TabChild* window = mWidget->GetOwningTabChild();
>+      orientation = window->GetOrientation();

Just use,

 if (TabChild* tab = mWidget->GetOwningTabChild()) {
   orientation = tab->GetOrientation();
 } else {
   //...

This is cleaner and makes fewer assumptions about the process type.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>+#include "Hal.h"
>+

We shouldn't need to include Hal.h here.

>+#define ORIENTATION_CHANGE_SYNC_MILLIS 1000

Let's make this a pref and read it on startup in gfxPlatform.  Ping me
for details on doing that if you need to.

>@@ -527,6 +541,8 @@ private:
>   }
> 
>   Layer* mRoot;
>+  TargetConfig mTargetConfig;
>+  bool mChecked;

Let's use a more descriptive name for this like |mReadyForCompose|.
And similarly for the |GetReadyForCompose()| helper above.

>@@ -546,7 +562,19 @@ CompositorParent::Composite()
>   }
> 
>   Layer* layer = mLayerManager->GetRoot();
>-  AutoResolveRefLayers resolve(layer);
>+  AutoResolveRefLayers resolve(layer, mTargetConfig);
>+  if (!resolve.GetTargetConfigChecked()) {
>+    if (mLastComposeCheckFail.IsNull()) {
>+      mLastComposeCheckFail = TimeStamp::Now();
>+    }
>+
>+    TimeDuration duration = TimeStamp::Now() - mLastComposeCheckFail;
>+    if (duration.ToMilliseconds() < ORIENTATION_CHANGE_SYNC_MILLIS) {
>+      ScheduleComposition();
>+      return;
>+    }
>+  }
>+  mLastComposeCheckFail = TimeStamp();
> 

There are two issues with the logic here.

First, if it takes a content process a really long time to update,
here's what might happen
 - try to compose
 - skip, set the variable here
 - (continue to fail until the timeout has elapsed)
 - force a composition anyway
 - reset the variable to "null"
 - on next composite, skip again
 - set the variable here
 - (continue to fail until the timeout has elapsed)

So we can get into a cycle where we only compose once every |timeout|
seconds.  That's not really what we want.

The second problem here is that the compositor thread will "spin" here
waiting on updates from subprocesses.  We'll recheck the subtrees at
60Hz.  This isn't a huge deal because this is relatively cheap, but
we're already notified when the subtree state changes so it's
unnecessary.

We can solve both problems by
 - in CompositorParent::ShadowLayersUpdate(), when the orientation has
   changed, create a timer task to expire after |timeout| ms.
   (Cancelling any old timer task.)
 - if the check here fails and the timer task is still pending, then
   bail
 - if the check fails and the timer task is *not* pending, then force
   a composite anyway

Let me know if this makes sense.

>diff --git a/gfx/layers/ipc/PLayers.ipdl b/gfx/layers/ipc/PLayers.ipdl
>index 6ebfde9..43f05ed 100644
>--- a/gfx/layers/ipc/PLayers.ipdl
>+++ b/gfx/layers/ipc/PLayers.ipdl
>@@ -39,6 +39,7 @@ namespace layers {
> struct TargetConfig {
>   nsIntRect naturalBounds;
>   ScreenRotation rotation;
>+  uint32_t orientation;

Use ScreenOrientation here too.

>diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp

>+  void Begin(const nsIntRect& aTargetBounds, ScreenRotation aRotation, uint32_t aOrientation)

ScreenOrientation

>diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp

>+  if (mRoot) {
>+    nsIntRect layerSize = mRoot->GetClipRect() ? *(mRoot->GetClipRect()) : mRoot->GetVisibleRegion().GetBounds();
>+    if (layerSize.width != rect.width || layerSize.height != rect.height)
>+      // Do not render if the layer tree is not ready with the current widget dimensions.
>+	  return;

This change doesn't make sense to me.  Maybe it's attempting to work
around the race condition I mentioned in comment 10?  Let's discuss
this some more.

This patch is looking very good, I'm quite impressed :).  This isn't
easy code to get started with ;).
Attachment #684560 - Flags: review?(jones.chris.g)
One more issue here: if the CompositorParent is compositing to a temporary surface, for example if we're taking a screenshot, then we should force the composition regardless of subtree state.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #34)
> Comment on attachment 684560 [details] [diff] [review]
> Patch V4
> 
> >diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
> >index e1ae01b..644904c 100644
> >--- a/dom/ipc/PBrowser.ipdl
> >+++ b/dom/ipc/PBrowser.ipdl
> >@@ -291,7 +291,7 @@ child:
> > 
> >     LoadURL(nsCString uri);
> > 
> >-    UpdateDimensions(nsRect rect, nsIntSize size) compress;
> >+    UpdateDimensions(nsRect rect, nsIntSize size, uint32_t orientation) compress;
> 
> Please use the |ScreenOrientation| type instead of |uint32_t| here.
I had some trouble to compile when use it before, I will try to figure it out. :)
> 
> >diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
> 
> >+#include "android/log.h"
> 
> This code won't compile on non-ANDROID platforms.  Please remove this
> #include and the logging statements.
> 
> (For testing patches, it's easier to use printf_stderr(), which goes
> to logcat on ANDROID platforms.)
> 
> > #define BROWSER_ELEMENT_CHILD_SCRIPT \
> >     NS_LITERAL_STRING("chrome://global/content/BrowserElementChild.js")
> >@@ -1095,7 +1096,7 @@ TabChild::RecvShow(const nsIntSize& size)
> > }
> > 
> > bool
> >-TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size)
> >+TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size, const uint32_t& orientation)
> > {
> >     if (!mRemoteFrame) {
> >         return true;
> >@@ -1106,6 +1107,7 @@ TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size)
> >     mOuterRect.width = rect.width;
> >     mOuterRect.height = rect.height;
> > 
> >+    mOrientation = orientation;
> >     mInnerSize = size;
> >     mWidget->Resize(0, 0, size.width, size.height,
> >                     true);
> >@@ -1114,6 +1116,8 @@ TabChild::RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size)
> >     baseWin->SetPositionAndSize(0, 0, size.width, size.height,
> >                                 true);
> > 
> >+    __android_log_print(ANDROID_LOG_INFO, "TabChild", "Update widget size to: (%d, %d)", size.width, size.height);
> >+
> >     HandlePossibleViewportChange();
> > 
> >     return true;
> >diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h
> >index 589978e..ece9fa8 100644
> >--- a/dom/ipc/TabChild.h
> >+++ b/dom/ipc/TabChild.h
> >@@ -196,7 +196,7 @@ public:
> > 
> >     virtual bool RecvLoadURL(const nsCString& uri);
> >     virtual bool RecvShow(const nsIntSize& size);
> >-    virtual bool RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size);
> >+    virtual bool RecvUpdateDimensions(const nsRect& rect, const nsIntSize& size, const uint32_t& orientation);
> >     virtual bool RecvUpdateFrame(const mozilla::layers::FrameMetrics& aFrameMetrics);
> >     virtual bool RecvHandleDoubleTap(const nsIntPoint& aPoint);
> >     virtual bool RecvHandleSingleTap(const nsIntPoint& aPoint);
> >@@ -290,6 +290,8 @@ public:
> >     /** Return the DPI of the widget this TabChild draws to. */
> >     void GetDPI(float* aDPI);
> > 
> >+    uint32_t GetOrientation() { return mOrientation; }
> >+
> >     void SetBackgroundColor(const nscolor& aColor);
> > 
> >     void NotifyPainted();
> >@@ -405,6 +407,7 @@ private:
> >     bool mNotified;
> >     bool mContentDocumentIsDisplayed;
> >     bool mTriedBrowserInit;
> >+    uint32_t mOrientation;
> > 
> >     DISALLOW_EVIL_CONSTRUCTORS(TabChild);
> > };
> 
> >diff --git a/gfx/layers/Makefile.in b/gfx/layers/Makefile.in
> >index 3ea24a4..9457e24 100644
> >--- a/gfx/layers/Makefile.in
> >+++ b/gfx/layers/Makefile.in
> >@@ -194,6 +194,9 @@ include $(topsrcdir)/ipc/chromium/chromium-config.mk
> > 
> > LOCAL_INCLUDES += \
> >         -I$(topsrcdir)/content/events/src \
> >+        -I$(topsrcdir)/hal \
> >+        -I$(topsrcdir)/widget/xpwidgets \
> >+        -I$(topsrcdir)/widget/gonk \
> >         -I$(ANDROID_SOURCE)/frameworks/base/include/media/stagefright \
> >         -I$(ANDROID_SOURCE)/frameworks/base/include/media/stagefright/openmax \
> >         $(NULL)
> >diff --git a/gfx/layers/basic/BasicLayerManager.cpp b/gfx/layers/basic/BasicLayerManager.cpp
> >index 947638b..68672ef 100644
> >--- a/gfx/layers/basic/BasicLayerManager.cpp
> >+++ b/gfx/layers/basic/BasicLayerManager.cpp
> >@@ -15,6 +15,7 @@
> > #include "nsXULAppAPI.h"
> > #include "RenderTrace.h"
> > #include "sampler.h"
> >+#include "Hal.h"
> > 
> > #define PIXMAN_DONT_DEFINE_STDINT
> > #include "pixman.h"
> >@@ -31,6 +32,8 @@
> > #include "AndroidBridge.h"
> > #endif
> > 
> >+#include "android/log.h"
> >+
> 
> Remember to remove these too.
> 
> > using namespace mozilla::dom;
> > using namespace mozilla::gfx;
> > 
> >@@ -1097,7 +1100,18 @@ BasicShadowLayerManager::BeginTransactionWithTarget(gfxContext* aTarget)
> >   // don't signal a new transaction to ShadowLayerForwarder. Carry on adding
> >   // to the previous transaction.
> >   if (HasShadowManager()) {
> >-    ShadowLayerForwarder::BeginTransaction(mTargetBounds, mTargetRotation);
> >+    uint32_t orientation;
> >+    if (GeckoProcessType_Content == XRE_GetProcessType()) {
> >+      TabChild* window = mWidget->GetOwningTabChild();
> >+      orientation = window->GetOrientation();
> 
> Just use,
> 
>  if (TabChild* tab = mWidget->GetOwningTabChild()) {
>    orientation = tab->GetOrientation();
>  } else {
>    //...
> 
> This is cleaner and makes fewer assumptions about the process type.
I wrote it this way originally, but I found the mWidget->GetOwningTabChild /*does*/ return a non-null value in parent process, and it's UpdateDimensions never called.
> 
> >diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp
> 
> >+#include "Hal.h"
> >+
> 
> We shouldn't need to include Hal.h here.
> 
> >+#define ORIENTATION_CHANGE_SYNC_MILLIS 1000
> 
> Let's make this a pref and read it on startup in gfxPlatform.  Ping me
> for details on doing that if you need to.
> 
> >@@ -527,6 +541,8 @@ private:
> >   }
> > 
> >   Layer* mRoot;
> >+  TargetConfig mTargetConfig;
> >+  bool mChecked;
> 
> Let's use a more descriptive name for this like |mReadyForCompose|.
> And similarly for the |GetReadyForCompose()| helper above.
> 
> >@@ -546,7 +562,19 @@ CompositorParent::Composite()
> >   }
> > 
> >   Layer* layer = mLayerManager->GetRoot();
> >-  AutoResolveRefLayers resolve(layer);
> >+  AutoResolveRefLayers resolve(layer, mTargetConfig);
> >+  if (!resolve.GetTargetConfigChecked()) {
> >+    if (mLastComposeCheckFail.IsNull()) {
> >+      mLastComposeCheckFail = TimeStamp::Now();
> >+    }
> >+
> >+    TimeDuration duration = TimeStamp::Now() - mLastComposeCheckFail;
> >+    if (duration.ToMilliseconds() < ORIENTATION_CHANGE_SYNC_MILLIS) {
> >+      ScheduleComposition();
> >+      return;
> >+    }
> >+  }
> >+  mLastComposeCheckFail = TimeStamp();
> > 
> 
> There are two issues with the logic here.
> 
> First, if it takes a content process a really long time to update,
> here's what might happen
>  - try to compose
>  - skip, set the variable here
>  - (continue to fail until the timeout has elapsed)
>  - force a composition anyway
>  - reset the variable to "null"
>  - on next composite, skip again
>  - set the variable here
>  - (continue to fail until the timeout has elapsed)
> 
> So we can get into a cycle where we only compose once every |timeout|
> seconds.  That's not really what we want.
> 
> The second problem here is that the compositor thread will "spin" here
> waiting on updates from subprocesses.  We'll recheck the subtrees at
> 60Hz.  This isn't a huge deal because this is relatively cheap, but
> we're already notified when the subtree state changes so it's
> unnecessary.
> 
> We can solve both problems by
>  - in CompositorParent::ShadowLayersUpdate(), when the orientation has
>    changed, create a timer task to expire after |timeout| ms.
>    (Cancelling any old timer task.)
>  - if the check here fails and the timer task is still pending, then
>    bail
>  - if the check fails and the timer task is *not* pending, then force
>    a composite anyway
> 
> Let me know if this makes sense.
> 
> >diff --git a/gfx/layers/ipc/PLayers.ipdl b/gfx/layers/ipc/PLayers.ipdl
> >index 6ebfde9..43f05ed 100644
> >--- a/gfx/layers/ipc/PLayers.ipdl
> >+++ b/gfx/layers/ipc/PLayers.ipdl
> >@@ -39,6 +39,7 @@ namespace layers {
> > struct TargetConfig {
> >   nsIntRect naturalBounds;
> >   ScreenRotation rotation;
> >+  uint32_t orientation;
> 
> Use ScreenOrientation here too.
> 
> >diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp
> 
> >+  void Begin(const nsIntRect& aTargetBounds, ScreenRotation aRotation, uint32_t aOrientation)
> 
> ScreenOrientation
> 
> >diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp
> 
> >+  if (mRoot) {
> >+    nsIntRect layerSize = mRoot->GetClipRect() ? *(mRoot->GetClipRect()) : mRoot->GetVisibleRegion().GetBounds();
> >+    if (layerSize.width != rect.width || layerSize.height != rect.height)
> >+      // Do not render if the layer tree is not ready with the current widget dimensions.
> >+	  return;
> 
> This change doesn't make sense to me.  Maybe it's attempting to work
> around the race condition I mentioned in comment 10?  Let's discuss
> this some more.
Yes, I tried to workaround it here. A better solution here is to let CompositorParent wait for the main tree update before any attempt to composition. 

I will try to use a Mutex to protect the value. The basic idea is:
For main thread:
1. When orientation changed, acquire a lock 
2. Reflow main tree
3. Update width and height
4. Unlock the lock

For render thread:
1. Get widget dimension from mWidget->GetClientBounds()
2. Acquire a lock 
3. Return the up-to-date dimensions

I am not sure if this works. I will give it a try :)
> 
> This patch is looking very good, I'm quite impressed :).  This isn't
> easy code to get started with ;).
(In reply to Chiajung Hung [:chiajung] from comment #36)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #34)
> > >+  if (mRoot) {
> > >+    nsIntRect layerSize = mRoot->GetClipRect() ? *(mRoot->GetClipRect()) : mRoot->GetVisibleRegion().GetBounds();
> > >+    if (layerSize.width != rect.width || layerSize.height != rect.height)
> > >+      // Do not render if the layer tree is not ready with the current widget dimensions.
> > >+	  return;
> > 
> > This change doesn't make sense to me.  Maybe it's attempting to work
> > around the race condition I mentioned in comment 10?  Let's discuss
> > this some more.
> Yes, I tried to workaround it here. A better solution here is to let
> CompositorParent wait for the main tree update before any attempt to
> composition. 
> 
> I will try to use a Mutex to protect the value. The basic idea is:
> For main thread:
> 1. When orientation changed, acquire a lock 
> 2. Reflow main tree
> 3. Update width and height
> 4. Unlock the lock
> 

Oh.  Please don't do that :).  That code will be almost impossible to get right, and it's just making an existing problem worse (blocking the compositor on the content thread for an unbounded amount of time).

Probably the easiest thing to do is add an interface on LayerManager to explicitly update the render target bounds.  Then we can just skip over the racy code here if an external user (in our case CompositorParent) is updating those bounds for LayerManagerOGL.
> > Just use,
> > 
> >  if (TabChild* tab = mWidget->GetOwningTabChild()) {
> >    orientation = tab->GetOrientation();
> >  } else {
> >    //...
> > 
> > This is cleaner and makes fewer assumptions about the process type.
> I wrote it this way originally, but I found the mWidget->GetOwningTabChild
> /*does*/ return a non-null value in parent process, and it's
> UpdateDimensions never called.
I tried again and can not reproduce my problem.
I may do some silly mistake in my earlier try. :S

(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> Oh.  Please don't do that :).  That code will be almost impossible to get
> right, and it's just making an existing problem worse (blocking the
> compositor on the content thread for an unbounded amount of time).
> 
> Probably the easiest thing to do is add an interface on LayerManager to
> explicitly update the render target bounds.  Then we can just skip over the
> racy code here if an external user (in our case CompositorParent) is
> updating those bounds for LayerManagerOGL.
Hmm, I will try it this way :)
Attached patch Patch V5 (obsolete) — — Splinter Review 
I got many helps from many person and implemented this new version based on cjones's review. 

Major change:
- Remove android log
- Using ScreenOrientation instead of uint32_t
- Change the time out model
- Change the LayerManagerOGL race condition solution
- Add a preference for orientation sync time out
Attachment #684560 - Attachment is obsolete: true
Attachment #686028 - Flags: review?(jones.chris.g)
Comment on attachment 686028 [details] [diff] [review]
Patch V5

These changes look good except for these issues

 1. The UpdateRenderBounds() interface is still racy.  You're making a method call on the nsIWidget* from the compositor thread, but the orientation and bounds are updated on the main thread.  There's nothing to synchronize the two.

Instead, probably the simplest solution is to pass around the "effective" widget bounds, GetClientBounds(), in the TargetConfig as well.  That way the orientation and "effective" bounds are always changed atomically in a layers txn.  You can keep the UpdateRenderBounds() interface but have it take an explicit nsIntRect argument, passed in by CompositorParent on the compositor thread.

 2. I don't see where this patch is temporarily forcing composition when there's a temporary render target set.  This happens when CompositorParent::ComposeToTarget(gfxContext* aTarget) is called.  We need to implement this to allow testers to gather screenshots, and for our test harnesses to properly check orientation change correctness.  The override should apply for that composition only.

 3. It seems that CompositorParent.mOrientationChanged and CompositorParent.mForceCompositionTask are redundant.  Doesn't mOrientationChanged <=> (mForceCompositionTask != nullptr)?

 4. Please use nullptr instead of NULL.
Attachment #686028 - Flags: review?(jones.chris.g)
Attached patch Patch V6 (obsolete) — — Splinter Review 
Major change:
- Remove redundant variable
- Update render bounds from main thread
- Handle for capture mode

I found the naturalBounds for chrome process always stay in native dimensions (a.k.a. the dimensions of frame buffer) so I update the render bounds respect to the ScreenOrientation. I am not sure whether I am correct, please correct me if I am wrong.
Attachment #686028 - Attachment is obsolete: true
Attachment #686474 - Flags: review?(jones.chris.g)
We're now marking soft blockers as P4/blocking-basecamp=- so I'm changing this bug to be consistent.
blocking-basecamp: + → -
Priority: P3 → P4
Comment on attachment 686474 [details] [diff] [review]
Patch V6

>diff --git a/dom/ipc/Makefile.in b/dom/ipc/Makefile.in
>index 5a65074..0d60356 100644
>--- a/dom/ipc/Makefile.in
>+++ b/dom/ipc/Makefile.in
>@@ -105,6 +105,7 @@ LOCAL_INCLUDES += \
> 	-I$(topsrcdir)/widget/xpwidgets \
> 	-I$(topsrcdir)/dom/bluetooth \
> 	-I$(topsrcdir)/dom/bluetooth/ipc \
>+        -I$(topsrcdir)/hal \

Nit: the indentation here is still messed up.

>diff --git a/gfx/layers/basic/BasicLayerManager.cpp b/gfx/layers/basic/BasicLayerManager.cpp

>@@ -1097,7 +1098,15 @@ BasicShadowLayerManager::BeginTransactionWithTarget(gfxContext* aTarget)

>+    uint32_t orientation;

ScreenOrientation

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>@@ -467,12 +480,15 @@ public:
>    * guaranteed by this helper only being used during the drawing
>    * phase.
>    */
>-  AutoResolveRefLayers(Layer* aRoot) : mRoot(aRoot)
>+  AutoResolveRefLayers(Layer* aRoot, const TargetConfig& aConfig) : mRoot(aRoot), mTargetConfig(aConfig), mReadyForCompose(true)
>   { WalkTheTree<Resolve>(mRoot, nullptr); }
> 
>   ~AutoResolveRefLayers()
>   { WalkTheTree<Detach>(mRoot, nullptr); }
> 
>+  bool GetReadyForCompose()

Let's rename this to |ReadyForCompose()|.  In English,
"GetReadyForCompose" is ambiguous; it can mean either "get the
ReadyForCompose value", or "prepare to compose".

>@@ -578,10 +611,23 @@ CompositorParent::ComposeToTarget(gfxContext* aTarget)
>   if (!CanComposite()) {
>     return;
>   }
>+
>+  bool orientationChanging = mForceCompositionTask != NULL;
>+  if (mForceCompositionTask) {
>+    mForceCompositionTask->Cancel();
>+    mForceCompositionTask = nullptr;
>+  }

I think it would be cleaner to add another field to CompositorParent
|bool mOverrideComposeReadiness| and then do

  AutoRestore override(mOverrideComposeReadiness);
  mOverrideComposeReadiness = true;

Then in Compose() we can check this variable to see if we should override ReadyForCompose().

That means we don't need the new |TimeStamp mOrientationChangeStamp;|, I don't think.

>diff --git a/gfx/layers/ipc/PLayers.ipdl b/gfx/layers/ipc/PLayers.ipdl

>@@ -39,6 +41,7 @@ namespace layers {
> struct TargetConfig {
>   nsIntRect naturalBounds;
>   ScreenRotation rotation;
>+  ScreenOrientation orientation;

What I was suggesting in (1) above is to add another member
|clientBounds| to this struct and set it at draw time.  So you would
have

 struct TargetConfig {
   nsIntRect naturalBounds;
   ScreenRotation rotation;
   nsIntRect clientBounds;
   ScreenOrientation orientation;
 };

And in CompositorParent, call
|layerManager->UpdateRenderBounds(targetConfig.clientBounds())|

This is looking very good :).  I would like to see another patch.
Attachment #686474 - Flags: review?(jones.chris.g)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #43)
> Comment on attachment 686474 [details] [diff] [review]
> Patch V6
> 
> >diff --git a/dom/ipc/Makefile.in b/dom/ipc/Makefile.in
> >index 5a65074..0d60356 100644
> >--- a/dom/ipc/Makefile.in
> >+++ b/dom/ipc/Makefile.in
> >@@ -105,6 +105,7 @@ LOCAL_INCLUDES += \
> > 	-I$(topsrcdir)/widget/xpwidgets \
> > 	-I$(topsrcdir)/dom/bluetooth \
> > 	-I$(topsrcdir)/dom/bluetooth/ipc \
> >+        -I$(topsrcdir)/hal \
> 
> Nit: the indentation here is still messed up.
This is weird. I rechecked this from my vim, and the indentation is OK...

> >diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp
> 
> >@@ -467,12 +480,15 @@ public:
> >    * guaranteed by this helper only being used during the drawing
> >    * phase.
> >    */
> >-  AutoResolveRefLayers(Layer* aRoot) : mRoot(aRoot)
> >+  AutoResolveRefLayers(Layer* aRoot, const TargetConfig& aConfig) : mRoot(aRoot), mTargetConfig(aConfig), mReadyForCompose(true)
> >   { WalkTheTree<Resolve>(mRoot, nullptr); }
> > 
> >   ~AutoResolveRefLayers()
> >   { WalkTheTree<Detach>(mRoot, nullptr); }
> > 
> >+  bool GetReadyForCompose()
> 
> Let's rename this to |ReadyForCompose()|.  In English,
> "GetReadyForCompose" is ambiguous; it can mean either "get the
> ReadyForCompose value", or "prepare to compose".
Do you think |IsReadyForCompose| better? Since "ready" can be used as a verb (from Yahoo! Dictionary), too. ReadyForCompose can mean "prepare to compose", isn't it?

> >diff --git a/gfx/layers/ipc/PLayers.ipdl b/gfx/layers/ipc/PLayers.ipdl
> 
> >@@ -39,6 +41,7 @@ namespace layers {
> > struct TargetConfig {
> >   nsIntRect naturalBounds;
> >   ScreenRotation rotation;
> >+  ScreenOrientation orientation;
> 
> What I was suggesting in (1) above is to add another member
> |clientBounds| to this struct and set it at draw time.  So you would
> have
> 
>  struct TargetConfig {
>    nsIntRect naturalBounds;
>    ScreenRotation rotation;
>    nsIntRect clientBounds;
>    ScreenOrientation orientation;
>  };
> 
> And in CompositorParent, call
> |layerManager->UpdateRenderBounds(targetConfig.clientBounds())|
Oh! I had misunderstood your comment, trying this way in next version :P
Thanks for your great review. I learned much more aspect of this topic after your review!

> 
> This is looking very good :).  I would like to see another patch.
(In reply to Chiajung Hung [:chiajung] from comment #44)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #43)
> > Comment on attachment 686474 [details] [diff] [review]
> > Patch V6
> > 
> > >diff --git a/dom/ipc/Makefile.in b/dom/ipc/Makefile.in
> > >index 5a65074..0d60356 100644
> > >--- a/dom/ipc/Makefile.in
> > >+++ b/dom/ipc/Makefile.in
> > >@@ -105,6 +105,7 @@ LOCAL_INCLUDES += \
> > > 	-I$(topsrcdir)/widget/xpwidgets \
> > > 	-I$(topsrcdir)/dom/bluetooth \
> > > 	-I$(topsrcdir)/dom/bluetooth/ipc \
> > >+        -I$(topsrcdir)/hal \
> > 
> > Nit: the indentation here is still messed up.
> This is weird. I rechecked this from my vim, and the indentation is OK...
> 

> > Let's rename this to |ReadyForCompose()|.  In English,
> > "GetReadyForCompose" is ambiguous; it can mean either "get the
> > ReadyForCompose value", or "prepare to compose".
> Do you think |IsReadyForCompose| better? Since "ready" can be used as a verb
> (from Yahoo! Dictionary), too. ReadyForCompose can mean "prepare to
> compose", isn't it?
> 

Sure, sounds good to me :).
Attached patch Patch V7 (obsolete) — — Splinter Review 
Change log
- change implementation for composition checking overwrite
- fix missed uint32->ScreenOrientation
- use proper function name
Attachment #686474 - Attachment is obsolete: true
Attachment #686898 - Flags: review?(jones.chris.g)
Comment on attachment 686898 [details] [diff] [review]
Patch V7

Looks great, nice work! :)

Before landing, let's test *very* carefully.  In particular, we need to check that the assumption of comment 33 is correct

  3. TabChild::RecvUpdateDimensions() always forces a repaint if the orientation has changed

If there are cases where that can be incorrect, we'll have some bad perceived-performance problems.
Attachment #686898 - Flags: review?(jones.chris.g) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> Comment on attachment 686898 [details] [diff] [review]
> Patch V7
> 
> Looks great, nice work! :)
> 
> Before landing, let's test *very* carefully.  In particular, we need to
> check that the assumption of comment 33 is correct
> 
>   3. TabChild::RecvUpdateDimensions() always forces a repaint if the
> orientation has changed
> 
> If there are cases where that can be incorrect, we'll have some bad
> perceived-performance problems.

Yes, this assumption is critical to this patch. But I don't know a good way to confirm this via test code. 

Besides, I can think out some case this assumption may fail:
1. 180 degree orientation change
2. Rectangular application
(Any other possibilities?)

The first one depends on #796722 and the second one is not easy to be done. I will try to apply the patch in #796722 and check:

1. Whether 180 degree orientation change causes problem?
2. Whether 180 degree orientation change causes repaint?

And if you have any suggestion to test it, please give me some idea :)
Thanks!
(In reply to Chiajung Hung [:chiajung] from comment #48)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> > Comment on attachment 686898 [details] [diff] [review]
> > Patch V7
> > 
> > Looks great, nice work! :)
> > 
> > Before landing, let's test *very* carefully.  In particular, we need to
> > check that the assumption of comment 33 is correct
> > 
> >   3. TabChild::RecvUpdateDimensions() always forces a repaint if the
> > orientation has changed
> > 
> > If there are cases where that can be incorrect, we'll have some bad
> > perceived-performance problems.
> 
> Yes, this assumption is critical to this patch. But I don't know a good way
> to confirm this via test code. 
> 
> Besides, I can think out some case this assumption may fail:
> 1. 180 degree orientation change

We can test this with an application that locks orientation to portrait-secondary when launched.

> 2. Rectangular application

Square?  We can also test this by modifying the cost control widget that's loaded into the system tray.  Give it equal dimensions and test what happens on orientation change.

> The first one depends on #796722 and the second one is not easy to be done.
> I will try to apply the patch in #796722 and check:

Locking orientation to portait-secondary (on otoro and unagi, anyway) should do the same.

Note that there are two specific things we need to check
 - TabParent::UpdateDimensions() is always called when there's an orientation change
 - TabChild always forces a repaint (or at least a layers txn that has the updated TargetConfig) when it receives the UpdateDimensions message
(In reply to Chris Jones [:cjones] [:warhammer] from comment #49)
> (In reply to Chiajung Hung [:chiajung] from comment #48)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #47)
> > > Comment on attachment 686898 [details] [diff] [review]
> > > Patch V7
> > > 
> > > Looks great, nice work! :)
> > > 
> > > Before landing, let's test *very* carefully.  In particular, we need to
> > > check that the assumption of comment 33 is correct
> > > 
> > >   3. TabChild::RecvUpdateDimensions() always forces a repaint if the
> > > orientation has changed
> > > 
> > > If there are cases where that can be incorrect, we'll have some bad
> > > perceived-performance problems.
> > 
> > Yes, this assumption is critical to this patch. But I don't know a good way
> > to confirm this via test code. 
> > 
> > Besides, I can think out some case this assumption may fail:
> > 1. 180 degree orientation change
> 
> We can test this with an application that locks orientation to
> portrait-secondary when launched.
I found a 180 degree do not always trigger a ResizeReflow by experiment. After check the code, I think it is because http://mxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp#222. And I think this line should suppress reflow for square case, too.

Though the expected ResizeReflow do not occurred, I /*can not*/ notice any artifact. I think I may think too much at Comment19/20...
> 
> > 2. Rectangular application
> 
> Square?  We can also test this by modifying the cost control widget that's
> loaded into the system tray.  Give it equal dimensions and test what happens
> on orientation change.
> 
> > The first one depends on #796722 and the second one is not easy to be done.
> > I will try to apply the patch in #796722 and check:
> 
> Locking orientation to portait-secondary (on otoro and unagi, anyway) should
> do the same.
> 
> Note that there are two specific things we need to check
>  - TabParent::UpdateDimensions() is always called when there's an
> orientation change
>  - TabChild always forces a repaint (or at least a layers txn that has the
> updated TargetConfig) when it receives the UpdateDimensions message
Chiajung, any chance we could implement bug 763430?
I am not sure I understand what you mean, but I would like to try to answer your question :p

1. If you think this patch would disable the patch in #763430. I think you are right. But the result would depend on the application shown and the dom event trigger sequence and this patch is a must. Android has similar screen SPS issue, since they do not sync up re-layout and SurfaceFlinger composition. I think we can re-confirm the code flow when orientation changed, then find what we need.


2. If you are asking if we can do screen rotation animation from gecko layer. I think it is possible, too. But the code flow and what effect we want need some more discussion. :)
I think Cervantes would like an iOS like effect since it is fancy, and I would prefer Android version since it is simple.

Or if I misunderstand you, please just reply this, or find me in face~
Chiajung, have you completed enough testing here that you feel comfortable requesting an uplift?

These orientation change artifacts are really bad :/.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #53)
> Chiajung, have you completed enough testing here that you feel comfortable
> requesting an uplift?
> 
> These orientation change artifacts are really bad :/.

Since I think it should OK to compose if these is no dimension change, I will try to modify it slightly to allow compose when no dimension update.

That means I will try to add 2 check and try it again:
1. orientation change is 180 degree (portrait to portrait or landscape to landscape)
2. visible region is square

Sorry for late update, but I think review+ means it will land later. Maybe I am wrong again.

By the way, I think the question of 180 degree/square widget is not that bad since we timeout in 1 second. Furthermore, it is hard to be observed as 180 degree change would be blocked by bug 796722 and no square widget case in current stage. I think it should be OK to fix it in this issue or fire a new one to follow-up.

Anyway, if there is anything I have to do to land it please tell me. And I will start to work on special cases.
Attached patch Patch V8 (obsolete) — — Splinter Review 
Here is a patch based on newer code base, so there is some merged content I think. 

The main different in this patch is mentioned in comment #54. Though I think earlier version is OK, but if you think it is better to handle those special cases here it is :)
Attachment #692122 - Flags: review?(jones.chris.g)
Comment on attachment 692122 [details] [diff] [review]
Patch V8

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>+        if (!ref->GetVisibleRegion().IsEmpty()) {
>+          ScreenOrientation chromeOrientation = mTargetConfig.orientation();
>+          ScreenOrientation contentOrientation = state->mTargetConfig.orientation();
>+          if (!IsSameDimension(chromeOrientation, contentOrientation) && WillReflow(mTargetConfig.clientBounds())) {

This line is much longer than 80 characters, please reflow it.

>+            mReadyForCompose = false;
>+          }
>+        }
>+
>+  bool WillReflow(nsIntRect& rect) {

Call this, |ContentMightReflowOnOrientationChange()|.

r=me with nits fixed
Attachment #692122 - Flags: review?(jones.chris.g) → review+
Attached patch Patch V9 — — Splinter Review 
Sorry for late update, since I have no chance to get access to my computer these days.

This patch fixed the function naming and reflowed the long description mentioned in earlier comment :)

Thanks!!
Attachment #686898 - Attachment is obsolete: true
Attachment #692122 - Attachment is obsolete: true
Attachment #694156 - Flags: review?(jones.chris.g)
Attachment #694156 - Flags: review?(jones.chris.g) → review+
Updating soft-blocker syntax.

Let's get this landed asap so we can evaluate an uplift.
tracking-b2g18: --- → +
Keywords: checkin-needed
Whiteboard: [soft] → UX-P?
This is a pretty epic perceived-performance win.  Nice work, Chiajung! :)

I'm going to send the patch off to try first, because I found issue that I think would have bounced it on inbound.
Keywords: checkin-needed
Fix compilation error under windows platform.
Attachment #694751 - Flags: review?(jones.chris.g)
Oh sorry, I already fixed this and sent it through tryserver :).

https://hg.mozilla.org/try/rev/533653d3c737
https://tbpl.mozilla.org/?tree=Try&rev=abb2e990a98f

Will push tonight when I finish testing something else.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #64)
> Oh sorry, I already fixed this and sent it through tryserver :).
> 
> https://hg.mozilla.org/try/rev/533653d3c737
> https://tbpl.mozilla.org/?tree=Try&rev=abb2e990a98f
> 
> Will push tonight when I finish testing something else.

Wow! Awesome! And I found some stupid error in my previous comment.
Thanks for your great help!!
Since it seems like bug 823802 may be this same issue, let's block on it.
blocking-basecamp: - → +
https://hg.mozilla.org/mozilla-central/rev/c55e10ab852d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 825848
Hello：
I found after this patch. 
When I use the API "mozRequestFullScreen" and then change the orientation, I will see some uncorrected screen such as half video or picture show at first and after 1 second(or more time which is defined in b2g.js -> " pref("layers.orientation.sync.timeout", 1000);") the screen will be corrected.

Could anyone help me ?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: