708414 - invite users to set up sync on about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Madhava Enros [:madhava]]

The main about:home design/dev is going on over in bug 706667.
The sync setup overview bug is here: bug 698594.

The current mockups in 706667 don't include the invitation to set up sync that is stipulated in 698594, but the next rev will. Oh yes - IT WILL.

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: Mockup of first run start page

Button specs and assets to follow soon

Comment 2

[Ian Barlow (:ibarlow)]

Attachment: button specs

(also, changed the button to grey)

Comment 3

[Ian Barlow (:ibarlow)]

Attachment: Sync button assets (includes button and icon)

Comment 4

[Axel Hecht [:Pike]]

I expect this to come with strings?

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Axel Hecht [:Pike] from comment #4)
> I expect this to come with strings?

Yes, it will add a new string. I saw that Brian has fixed the hardcoded strings in about:home in his patch for bug 712970.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Add API to check if Fennec is running for the first time

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: (2/4) Improve layout of about:home for top sites

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: (3/4) Show message when there are no top sites to show in about:home

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (4/4) Invite users to set up sync on about:home

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 589592 [details] [diff] [review]
(1/4) Add API to check if Fennec is running for the first time

Let's just make sure we don't get a StrictMode violation on this. Being in a background thread should keep that from being a problem.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 589595 [details] [diff] [review]
(3/4) Show message when there are no top sites to show in about:home

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
>+<!ENTITY abouthome_no_top_sites "You do no have any top sites yet. Tap the Title bar to start browsing.">
>\ No newline at end of file

nit: add a newline at end of file

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 589597 [details] [diff] [review]
(4/4) Invite users to set up sync on about:home


>diff --git a/mobile/android/base/AboutHomeContent.java b/mobile/android/base/AboutHomeContent.java

>+            TextView syncTextView = (TextView) findViewById(R.id.sync_text);
>+            String syncText = syncTextView.getText().toString() + " \u00BB";
>+            int styleIndex = syncText.indexOf("Firefox Sync");

Don't hard code this. Make a second string "abouthome_sync_bold" and use it to find the style index

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY abouthome_no_top_sites "You do no have any top sites yet. Tap the Title bar to start browsing.">
>+<!ENTITY abouthome_sync "Set up Firefox Sync to access bookmarks, history and tabs from your other devices">

Normally I would suggest using brandShortName entity to make the string, but we use "Firefox Sync" like this in XUL too.

I do think we want to better control the "bold" string, so add:

  <!ENTITY abouthome_sync_bold "Firefox Sync">

>\ No newline at end of file

nit: add one

>diff --git a/mobile/android/base/resources/drawable/abouthome_sync_box.xml b/mobile/android/base/resources/drawable/abouthome_sync_box.xml

>\ No newline at end of file

nit: add one

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 589592 [details] [diff] [review]
(1/4) Add API to check if Fennec is running for the first time

pretty sure this will regress start up. SharedPreferences is pretty heavy for this particular purpose, according to DougT accessing it can take ~200ms because it needs to hit the disk. Since you're posting to the front of the queue in onCreate, you'll delay onNewIntent() by that time.

Instead I suggest you perform your own disk access on a background thread to do this. 

Its also not obvious to me that we need to do this so early in start up anyway. Unless there's a good reason not to, I'd suggest you delay initializing mStartupMode until it is accessed. And we shouldn't have to access it until about:home is being created or even after it is displayed.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

sorry, just realized that you're doing this on the background thread, my assumption that this was the main UI thread came from the fact that you're using SharedPreferences and as far as I know that needs to be accessed on the main UI thread. I'll look into that further in the morning.

Comment 15

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #14)
> sorry, just realized that you're doing this on the background thread, my
> assumption that this was the main UI thread came from the fact that you're
> using SharedPreferences and as far as I know that needs to be accessed on
> the main UI thread. I'll look into that further in the morning.

AFAIK, SharedPreferences is thread-safe. What you can't do is reading and writing prefs from different *processes*.

Comment 16

[Madhava Enros [:madhava]]

Regarding bug 717691 -- looks like sync setup isn't possible for certain older versions of Android? If it's not possible, we shouldn't show the UI on the start page (vs. showing it and then having setup fail).

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 589592 [details] [diff] [review]
(1/4) Add API to check if Fennec is running for the first time

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

looks like SharedPreferences is safe to use on the background thread. But the rest of my reviewis still valid, we don't need to be doing this so early on startup.

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: Add API to check if Fennec is running for the first time (

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 590717 [details] [diff] [review]
Add API to check if Fennec is running for the first time (

Sorry for the spam. Ignore this one.

Comment 20

[Lucas Rocha (:lucasr)]

Attachment: (1/4) Add API to check if Fennec is running for the first time

Removed early initialization of startup mode. Moved it to when we init about:home's content (see patch 4/4).

Comment 21

[Lucas Rocha (:lucasr)]

Attachment: (4/4) Invite users to set up sync on about:home

Just added a getStartupMode() call in background thread while loading about:home's content. Keeping r+.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 590718 [details] [diff] [review]
(1/4) Add API to check if Fennec is running for the first time

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

::: mobile/android/base/GeckoApp.java
@@ +774,5 @@
>              }
>          });
>      }
>  
> +    public StartupMode getStartupMode() {

add a comment above this that says "this function may touch the disk, don't call it on the main thread"

Also, this should probably be synchronized unless we can guarantee its always run on the same thread. If you're going with the "always run on the same thread" approach, add a check for that.

@@ +775,5 @@
>          });
>      }
>  
> +    public StartupMode getStartupMode() {
> +        if (mStartupMode == null) {

I'm a fan of early returns, so:

if (mStartupMode != null)
    return mStartupMode;

// initialize it
return mStartupMode;

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #22)
> Comment on attachment 590718 [details] [diff] [review]
> (1/4) Add API to check if Fennec is running for the first time
> 
> Review of attachment 590718 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +774,5 @@
> >              }
> >          });
> >      }
> >  
> > +    public StartupMode getStartupMode() {
> 
> add a comment above this that says "this function may touch the disk, don't
> call it on the main thread"

Done.

> Also, this should probably be synchronized unless we can guarantee its
> always run on the same thread. If you're going with the "always run on the
> same thread" approach, add a check for that.

Good point. Enclosed everything in a synchronized block.

> @@ +775,5 @@
> >          });
> >      }
> >  
> > +    public StartupMode getStartupMode() {
> > +        if (mStartupMode == null) {
> 
> I'm a fan of early returns, so:

Me too :-) Don't know why I did it differently now. Fixed.

Comment 24

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 589595 [details] [diff] [review]
> (3/4) Show message when there are no top sites to show in about:home
> 
> >diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
> >+<!ENTITY abouthome_no_top_sites "You do no have any top sites yet. Tap the Title bar to start browsing.">
> >\ No newline at end of file
> 
> nit: add a newline at end of file

Fixed.

Comment 25

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 589597 [details] [diff] [review]
> (4/4) Invite users to set up sync on about:home
> 
> 
> >diff --git a/mobile/android/base/AboutHomeContent.java b/mobile/android/base/AboutHomeContent.java
> 
> >+            TextView syncTextView = (TextView) findViewById(R.id.sync_text);
> >+            String syncText = syncTextView.getText().toString() + " \u00BB";
> >+            int styleIndex = syncText.indexOf("Firefox Sync");
> 
> Don't hard code this. Make a second string "abouthome_sync_bold" and use it
> to find the style index
> 
> >diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
> 
> >+<!ENTITY abouthome_no_top_sites "You do no have any top sites yet. Tap the Title bar to start browsing.">
> >+<!ENTITY abouthome_sync "Set up Firefox Sync to access bookmarks, history and tabs from your other devices">
> 
> Normally I would suggest using brandShortName entity to make the string, but
> we use "Firefox Sync" like this in XUL too.
> 
> I do think we want to better control the "bold" string, so add:
> 
>   <!ENTITY abouthome_sync_bold "Firefox Sync">

Done.

> >\ No newline at end of file
> 
> nit: add one

Done.

> >diff --git a/mobile/android/base/resources/drawable/abouthome_sync_box.xml b/mobile/android/base/resources/drawable/abouthome_sync_box.xml
> 
> >\ No newline at end of file
> 
> nit: add one

Done.

Comment 26

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/16cc48013e6a
http://hg.mozilla.org/integration/mozilla-inbound/rev/977686f756fe
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a59fe0a2dbe
http://hg.mozilla.org/integration/mozilla-inbound/rev/9e1503b399a7

Comment 27

[Lucas Rocha (:lucasr)]

Comment on attachment 589593 [details] [diff] [review]
(2/4) Improve layout of about:home for top sites

Missing feature in aurora. Mobile only.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 589595 [details] [diff] [review]
(3/4) Show message when there are no top sites to show in about:home

Missing feature in aurora. Mobile only.

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 590718 [details] [diff] [review]
(1/4) Add API to check if Fennec is running for the first time

Missing feature in aurora. Mobile only.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 590720 [details] [diff] [review]
(4/4) Invite users to set up sync on about:home

Missing feature in aurora. Mobile only.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/16cc48013e6a
https://hg.mozilla.org/mozilla-central/rev/977686f756fe
https://hg.mozilla.org/mozilla-central/rev/3a59fe0a2dbe
https://hg.mozilla.org/mozilla-central/rev/9e1503b399a7

Comment 32

[Alex Keybl [:akeybl]]

Comment on attachment 589593 [details] [diff] [review]
(2/4) Improve layout of about:home for top sites

[Triage Comment]
Mobile only - approved for Aurora.

Comment 33

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68105795f01d
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d59e1b01229
https://hg.mozilla.org/releases/mozilla-aurora/rev/1fae65115913
https://hg.mozilla.org/releases/mozilla-aurora/rev/c77e8f61673d

Comment 34

[Camelia Urian]

Nightly 13.0a1 (2012-02-03)
Aurora 12.0a2 (2012-02-02)
Beta 11.0 (2012-02-01)
Device: Samsung Google Nexus S - Android 2.3.6

Invite to sync is displayed on about:home page. After sync is performed, invite is not displayed anymore on start page.
Marking bug as verified fixed.