Closed Bug 902821 Opened 7 years ago Closed 7 years ago

Screen rotation will be lagged for a while.

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: vlin, Assigned: vlin)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix.patch (obsolete) — Splinter Review
Bug 796722 will not fix screen rotation issue completely. Open a new bug to fix it. The modification is referenced to the *PaintWindow calls flow in Bug 826817.
Attachment #787354 - Flags: review?(mwu)
Attachment #787354 - Flags: review?(mchen)
Comment on attachment 787354 [details] [diff] [review]
fix.patch

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

Michael is the correct one to do the review and is the module owner.

::: widget/gonk/nsWindow.cpp
@@ +255,4 @@
>              AutoLayerManagerSetup setupLayerManager(
>                  gWindowToRedraw, ctx, mozilla::layers::BUFFER_NONE,
>                  ScreenRotation(EffectiveScreenRotation()));
> +	

Remove spaces.

@@ +255,5 @@
>              AutoLayerManagerSetup setupLayerManager(
>                  gWindowToRedraw, ctx, mozilla::layers::BUFFER_NONE,
>                  ScreenRotation(EffectiveScreenRotation()));
> +	
> +	    listener = gWindowToRedraw->GetWidgetListener();

Does it need to get listener again and again or just get it once is enough?
Attachment #787354 - Flags: review?(mchen)
(In reply to Marco Chen [:mchen] from comment #1)
> Comment on attachment 787354 [details] [diff] [review]
> fix.patch
> 
> Review of attachment 787354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Michael is the correct one to do the review and is the module owner.
> 
> ::: widget/gonk/nsWindow.cpp
> @@ +255,4 @@
> >              AutoLayerManagerSetup setupLayerManager(
> >                  gWindowToRedraw, ctx, mozilla::layers::BUFFER_NONE,
> >                  ScreenRotation(EffectiveScreenRotation()));
> > +	
> 
> Remove spaces.
> 
> @@ +255,5 @@
> >              AutoLayerManagerSetup setupLayerManager(
> >                  gWindowToRedraw, ctx, mozilla::layers::BUFFER_NONE,
> >                  ScreenRotation(EffectiveScreenRotation()));
> > +	
> > +	    listener = gWindowToRedraw->GetWidgetListener();
> 
> Does it need to get listener again and again or just get it once is enough?

Yes, it does.

Comment from here.
https://bugzilla.mozilla.org/show_bug.cgi?id=796722#c20
blocking-b2g: --- → hd?
blocking-b2g: hd? → leo?
Assignee: nobody → vlin
Spoken to vlin, 

bug 796722 will work on its own for master, but this bug is required for 796722 to have effect on v1.1.

leo+ing this as bug 796722 is leo+ and has been included on v1.1
Blocks: 796722
blocking-b2g: leo? → leo+
No longer depends on: 796722
Comment on attachment 787354 [details] [diff] [review]
fix.patch

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

Is this a 1.1 only patch?

There's a bunch of tabs in this patch. Please replace them with spaces and make sure the style is consistent.
Comment on attachment 787354 [details] [diff] [review]
fix.patch

Clearing review while waiting for a new patch and an answer about whether this is 1.1 only.
Attachment #787354 - Flags: review?(mwu)
(In reply to Wayne Chang [:wchang] from comment #3)
> leo+ing this as bug 796722 is leo+ and has been included on v1.1

This isn't automatically leo+, it all depends on current user experience. Are we sure it's not good enough already?
blocking-b2g: leo+ → leo?
Flagging Patrick to comment on this rotation issue since this is HD. Please let me know if this should be a blocker for 1.1.
Flags: needinfo?(padamczyk)
Can I get more info? I am running 2013-08-22's build and the rotation on apps like the browser is OKAY, but could be faster. Its about 2 second delay before rotation, I'd expect it to be under 1 second. Is this what you mean?
(In reply to Patryk Adamczyk [:patryk] UX from comment #8)
> Can I get more info? I am running 2013-08-22's build and the rotation on
> apps like the browser is OKAY, but could be faster. Its about 2 second delay
> before rotation, I'd expect it to be under 1 second. Is this what you mean?

Yap~
You can flip screen between 0 and 180 orientation. This issue will be more visible and reproducible. And after orientation changes, sometimes user even have to touch the screen to trigger screen update.
Attached patch Remove tabs.Splinter Review
This patch is for 1.1 only.
Attachment #795234 - Flags: review?(mwu)
Leo has stopped accepting patches for their 1.1 build, so not going to block on this unless they explicitly ask for this to block the release.
blocking-b2g: leo? → -
Attachment #787354 - Attachment is obsolete: true
Comment on attachment 795234 [details] [diff] [review]
Remove tabs.

r=me for a straightforward backport of a fix.

This is probably worth landing on 1.1 if we can. If not for leo, then for other 1.1 updates.
Attachment #795234 - Flags: review?(mwu) → review+
Dietrich, as part of our mega triage of 1.1 leo+ blockers, this bug had a vote to retain in 1.1

I'll wait for Patryk's testing and if worth while will move to a +
I am going to need more information. 
+ how do I test this?
+ where is the repo?
+ how much faster is it (before / after)

From my experience on the leo device, I don't think the rotation performance is a blocker, but if it affects all 1.1 devices, we should push this into 1.1 HD and 1.2, 100%
blocking-b2g: - → leo?
Flags: needinfo?(padamczyk)
(In reply to Patryk Adamczyk [:patryk] UX from comment #14)
> I am going to need more information. 
> + how do I test this?
> + where is the repo?
> + how much faster is it (before / after)

needinfo'ing vin to help with this info here.
> 
> From my experience on the leo device, I don't think the rotation performance
> is a blocker,

Agreed !

> but if it affects all 1.1 devices, we should push this into
> 1.1 HD and 1.2, 100%

This is close to resolution on 1.2 anyway, nevertheless flipping it back to koi?
blocking-b2g: leo? → koi?
This is a backport of a fix that's already on 1.2, so no need to block on koi.
blocking-b2g: koi? → ---
Keywords: checkin-needed
blocking-b2g: --- → hd?
Can't land without appropriate blocking status. Please re-request checkin once it does.
Keywords: checkin-needed
Flags: needinfo?(mwu)
Not sure what the ni? here is for.
Flags: needinfo?(mwu)
Please make sure the blocking status(Project Flags).
Status: NEW → ASSIGNED
Flags: needinfo?(wchang)
Let's hold this for now as we're all moving towards v1.2, unless a partner has a strong request on leo for this.

Setting state according to comment 16.
blocking-b2g: hd? → ---
Flags: needinfo?(wchang)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.