Last Comment Bug 695836 - sync last screen and last uri preference
: sync last screen and last uri preference
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: P1 normal (vote)
: ---
Assigned To: Doug Turner (:dougt)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-19 13:28 PDT by Doug Turner (:dougt)
Modified: 2012-01-09 11:24 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
work in progress. (21.39 KB, patch)
2011-10-23 18:52 PDT, Doug Turner (:dougt)
mark.finkle: review+
Details | Diff | Review

Description Doug Turner (:dougt) 2011-10-19 13:28:33 PDT

    
Comment 1 Doug Turner (:dougt) 2011-10-23 18:52:55 PDT
Created attachment 568994 [details] [diff] [review]
work in progress.

pretty close to what we want.  I want to see if we can also store the offset
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-23 19:45:52 PDT
(In reply to Doug Turner (:dougt) from comment #1)
> Created attachment 568994 [details] [diff] [review] [diff] [details] [review]
> work in progress.
> 
> pretty close to what we want.  I want to see if we can also store the offset

Sounds like something we want in session restore anyway
Comment 3 Doug Turner (:dougt) 2011-10-23 20:23:30 PDT
if so, maybe we go with this as-is.  it is a ton better now.

the basic idea is that we can't keep the last uri in prefs because we need it before gecko starts up.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-23 20:53:29 PDT
Comment on attachment 568994 [details] [diff] [review]
work in progress.


>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java

>+                    // this is already true
>+                    // surfaceView.showStartupBitmap();

Remove these

>+                    final String awesomeURI = uri; 
>+                    mMainHandler.post(new Runnable() {
>+                      public void run() {
>+                        mBrowserToolbar.setTitle(awesomeURI);
>+                      }
>+                    });

Does this pass the blassey-test for mMainHandler vs. mHandler ?

Also, can't we try to use "last-title" pref here?

>+    private void rememberLastScreen(boolean sync) {

>+        if (!tab.getHistory().empty()) {
>+
>+            SharedPreferences prefs = getSharedPreferences ("GeckoApp", 0);

nits: no blank line needed at top of block and drop the space from | getSharedPreferences ( | (you do it every time you use getSharedPreferences)

>+            surfaceView.saveLast(sync);
>+        }
>+
>+    }

no blank line needed here

>+    private boolean restoreLastScreen() {

>+        return true;
>+        //        surfaceView.saveLast(sync);

remove

>     private void quit() {

>+        System.exit(0);
>         GeckoAppShell.nativeQuit();
>         finish();

Brad removed System.exit(0) and added the 2 new lines in this patch:
http://hg.mozilla.org/projects/birch/rev/653aa43e9643

Do we need it?

>diff --git a/embedding/android/GeckoSurfaceView.java b/embedding/android/GeckoSurfaceView.java

>     public void surfaceCreated(SurfaceHolder holder) {

>+        Log.i(LOG_FILE_NAME, "!! surface created");
>+
>+        // Delay sending this event if we are painting the
>+        // load screen.  The native access paint path will
>+        // paint directly to the screen and we will see a
>+        // black screen while content is initally being
>+        // drawn.
>+        if (!mShowingLoadScreen) {
>+            GeckoEvent e = new GeckoEvent(GeckoEvent.SURFACE_CREATED);
>+            GeckoAppShell.sendEventToGecko(e);
>+        }

You might want to move the Log into the if block, so we know if this is _really_ called

>diff --git a/mobile/components/BrowserCLH.js b/mobile/components/BrowserCLH.js

> BrowserCLH.prototype = {
>   handle: function fs_handle(aCmdLine) {
>-    dump("fs_handle");
> 
>-    let urlParam = aCmdLine.handleFlagWithParam("remote", false);

Don't leave the leading blank line

>+    let urlParam = "about:";

about:home? or do you want to wait?

>+    try {
>+        urlParam = aCmdLine.handleFlagWithParam("remote", false);

2 spaces

>+    } catch (e) { dump(""+e); }

nit: dump("" + e);

r+ with nits and some thought for the System.exit(0)

the mMainHandler things is up to you :)
Comment 5 Doug Turner (:dougt) 2011-10-23 21:06:08 PDT
> Does this pass the blassey-test for mMainHandler vs. mHandler ?

There are like 2 dozen similar patterns here for setting the ui widgets.  we can have a follow up.

> Also, can't we try to use "last-title" pref here?

Not sure what you mean.  We can set the title instead, either way.
Comment 6 Doug Turner (:dougt) 2011-10-23 21:27:31 PDT
http://hg.mozilla.org/projects/birch/rev/45c91268cfee

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