Last Comment Bug 745077 - Touch event is not correct when the orientation changes
: Touch event is not correct when the orientation changes
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Widget (show other bugs)
: unspecified
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
:
Mentors:
Depends on: 737199
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-12 21:36 PDT by Kan-Ru Chen [:kanru] (UTC+8)
Modified: 2012-04-26 03:59 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 745077 - Reconfigure InputReader on screen rotation. (2.04 KB, patch)
2012-04-19 21:31 PDT, Kan-Ru Chen [:kanru] (UTC+8)
mwu.code: review+
Details | Diff | Splinter Review
Reconfigure InputReader on screen rotation. v1.1 (1.86 KB, patch)
2012-04-24 20:45 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Splinter Review

Description Kan-Ru Chen [:kanru] (UTC+8) 2012-04-12 21:36:27 PDT
STR:

 1. rotate the device (sgs2)
 2. the homescreen is in landscape mode
 3. but the touch events are still in portrait coordinate.
Comment 1 Cervantes Yu [:cyu] [:cervantes] 2012-04-19 05:02:47 PDT
It looks like methods handling input coordinates need to consider current screen rotation and make adjustments accordingly. There is already a quick access to current rotation via nsScreenGonk, but width and height are problems. Querying screen manager seems not a good idea in such performance sensitive code.
Comment 2 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-19 21:31:24 PDT
Created attachment 616871 [details] [diff] [review]
Bug 745077 - Reconfigure InputReader on screen rotation.
Comment 3 Michael Wu [:mwu] 2012-04-22 02:14:39 PDT
Regression from bug 737199
Comment 4 Michael Wu [:mwu] 2012-04-23 13:32:37 PDT
Comment on attachment 616871 [details] [diff] [review]
Bug 745077 - Reconfigure InputReader on screen rotation.

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

Looks good. r=me with the requested changes.

::: widget/gonk/nsAppShell.cpp
@@ +704,5 @@
>      write(signalfds[1], "w", 1);
>  }
>  
> +void
> +nsAppShell::NotifyScreenChangesInternal()

Inline this into NotifyScreenChanges. Also rename NotifyScreenChanges to NotifyScreenRotation.
Comment 5 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-23 19:29:03 PDT
(In reply to Michael Wu [:mwu] from comment #4)
> > +void
> > +nsAppShell::NotifyScreenChangesInternal()
> 
> Inline this into NotifyScreenChanges. Also rename NotifyScreenChanges to
> NotifyScreenRotation.

Hmm.. I need to access the private mReader. Do you want to expose gAppShell to nsWindow? 

NotifyScreenChanges does not only notify the orientation change but also the screen size change. But if you think NotifyScreenRotation fits here then I'm okay as well.
Comment 6 Michael Wu [:mwu] 2012-04-23 19:38:42 PDT
(In reply to Kan-Ru Chen [:kanru] from comment #5)
> (In reply to Michael Wu [:mwu] from comment #4)
> > > +void
> > > +nsAppShell::NotifyScreenChangesInternal()
> > 
> > Inline this into NotifyScreenChanges. Also rename NotifyScreenChanges to
> > NotifyScreenRotation.
> 
> Hmm.. I need to access the private mReader. Do you want to expose gAppShell
> to nsWindow? 
> 

NotifyScreenChanges is part of nsAppShell, so it should be able to access mReader, no?

> NotifyScreenChanges does not only notify the orientation change but also the
> screen size change. But if you think NotifyScreenRotation fits here then I'm
> okay as well.

Yeah, it's really the only reason why the screen size would change, so I think notifyscreenrotation is fine.
Comment 7 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-23 19:54:56 PDT
(In reply to Michael Wu [:mwu] from comment #6)
> > Hmm.. I need to access the private mReader. Do you want to expose gAppShell
> > to nsWindow? 
> > 
> 
> NotifyScreenChanges is part of nsAppShell, so it should be able to access
> mReader, no?

Oh, yes. So I will do gAppShell->mReader->... in it.
 
> > NotifyScreenChanges does not only notify the orientation change but also the
> > screen size change. But if you think NotifyScreenRotation fits here then I'm
> > okay as well.
> 
> Yeah, it's really the only reason why the screen size would change, so I
> think notifyscreenrotation is fine.

Got it.
Comment 8 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-24 20:45:49 PDT
Created attachment 618160 [details] [diff] [review]
Reconfigure InputReader on screen rotation. v1.1
Comment 9 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-24 21:02:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/26dbb2e9d91d

Note You need to log in before you can comment on or make changes to this bug.