If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

invite users to set up sync on about:home

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: madhava, Assigned: lucasr)

Tracking

({uiwanted})

unspecified
Firefox 12
ARM
Android
uiwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 verified, firefox12 verified, firefox13 verified, fennec11+)

Details

Attachments

(7 attachments, 3 obsolete attachments)

523.52 KB, image/png
Details
196.27 KB, image/png
Details
47.05 KB, application/zip
Details
2.41 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
6.36 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
4.08 KB, patch
blassey
: review+
Details | Diff | Splinter Review
74.71 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Keywords: uiwanted
Assignee: nobody → lucasr.at.mozilla
Priority: -- → P2
Created attachment 581301 [details]
Mockup of first run start page

Button specs and assets to follow soon
Created attachment 581658 [details]
button specs

(also, changed the button to grey)
Created attachment 581779 [details]
Sync button assets (includes button and icon)
OS: Mac OS X → Android
Hardware: x86 → ARM
tracking-fennec: --- → 11+
(Reporter)

Updated

6 years ago
Blocks: 698594

Comment 4

6 years ago
I expect this to come with strings?
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
Created attachment 589592 [details] [diff] [review]
(1/4) Add API to check if Fennec is running for the first time
Attachment #589592 - Flags: review?(blassey.bugs)
(Assignee)

Comment 7

6 years ago
Created attachment 589593 [details] [diff] [review]
(2/4) Improve layout of about:home for top sites
Attachment #589593 - Flags: review?(mark.finkle)
(Assignee)

Comment 8

6 years ago
Created attachment 589595 [details] [diff] [review]
(3/4) Show message when there are no top sites to show in about:home
Attachment #589595 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

6 years ago
Created attachment 589597 [details] [diff] [review]
(4/4) Invite users to set up sync on about:home
Attachment #589597 - Flags: review?(mark.finkle)
Attachment #589593 - Flags: review?(mark.finkle) → review+
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.
Attachment #589592 - Flags: feedback+
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
Attachment #589595 - Flags: review?(mark.finkle) → review+
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
Attachment #589597 - Flags: review?(mark.finkle) → review+
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.
Attachment #589592 - Flags: review?(blassey.bugs) → review-
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.
(Assignee)

Comment 15

6 years ago
(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*.
(Reporter)

Comment 16

6 years ago
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).

Updated

6 years ago
Blocks: 719714
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.
(Assignee)

Comment 18

6 years ago
Created attachment 590717 [details] [diff] [review]
Add API to check if Fennec is running for the first time (
(Assignee)

Comment 19

6 years ago
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.
Attachment #590717 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Created attachment 590718 [details] [diff] [review]
(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).
Attachment #590718 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Attachment #589592 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 590720 [details] [diff] [review]
(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+.
Attachment #589597 - Attachment is obsolete: true
Attachment #590720 - Flags: review+
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;
Attachment #590718 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 23

6 years ago
(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.
(Assignee)

Comment 24

6 years ago
(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.
(Assignee)

Comment 25

6 years ago
(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.
(Assignee)

Comment 26

6 years ago
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
(Assignee)

Comment 27

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

Missing feature in aurora. Mobile only.
Attachment #589593 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 28

6 years ago
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.
Attachment #589595 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 29

6 years ago
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.
Attachment #590718 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 30

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

Missing feature in aurora. Mobile only.
Attachment #590720 - Flags: approval-mozilla-aurora?
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12

Updated

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

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #589593 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #589595 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #590718 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #590720 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox11: --- → fixed

Comment 34

6 years ago
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.
Status: RESOLVED → VERIFIED
status-firefox11: fixed → verified
status-firefox12: --- → verified
status-firefox13: --- → verified
You need to log in before you can comment on or make changes to this bug.