Closed Bug 1293713 Opened 8 years ago Closed 7 years ago

Rethink how homepages are done on Firefox Android

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 affected, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: mkaply, Assigned: cnevinchen, NeedInfo)

References

Details

Attachments

(2 files)

Screen Shot 2017-02-09 at 1.35.07 PM.png
76.27 KB, image/png
Details
Bug 1293713 - Make new tab action respect HomePage preference.
59 bytes, text/x-review-board-request
mkaply
: review+
sebastian
: review+
Details 
With Firefox 48, Firefox Android was switched to restoring the user's session by default no matter how Firefox is closed.

This is a great behavior for users, but it means that our homepage support now becomes pretty useless.

The only way to see the homepage is:

1. First run of Firefox.
2. Close all tabs of Firefox (which actually leaves a tab open) and the close Firefox and then restart it.

We should try to make the display of the homepage somewhat consistent if the user (or distribution) has set it.

For reference, the way Chrome works is that if you have a homepage set, there are a few unique behaviors.

1. Closing the last tab completely closes the browser so that on restart the homepage is shown.
2. If you close explicitly via the app list, instead of restoring the session, it shows the homepage on restart.
3. The homepage is only shown for the first tab. Subsequent new tabs get their internal about:home page similar to ours.

I do not know what happens if Chrome is closed automatically due to memory constraints.

As a side note, it appears that Chrome's ability to set a homepage is determined by the OEM, so if you find you can't set a homepage in Chrome at all, it's probably because your phone/tablet manufacturer didn't allow it.

I have three devices, and only one has the homepage setting on Chrome (Galaxy S7).
Flags: needinfo?(bbermes)
Could we just revert this to not be the default and let users decide via settings? And after first run of Firefox, we could show a contextual hint to have users decide what they prefer.
Flags: needinfo?(bbermes) → needinfo?(alam)
Reverting the setting's default is not really a good option: "Do not restore tabs" effectively means: "Sometimes you open your browser and the tabs are still there and sometimes they are gone. We just can't tell you upfront". Apart from that we just shipped that to release - do we really want to flip the setting on the next release again? Especially considering how much we discussed making this change in the first place. :)

What chrome does is a bit weird but I think your proposal from bug 1291704 is a good idea:
> Is there any way to change the Firefox behavior so that closing the last tab
> goes to the website homepage instead of top sites?
I hesitate to look for a "quick fix" here. We've gone back and forth on this issue a lot before so we know that it can quickly get out of hand. Perhaps to help scope out this work, we should take a step back and look at what problem we're trying to solve here?

I understand that our experience is different to Chrome's. And that in Chrome, the user see's this "home page" a lot more often. But, there are pros and cons to each.

Is our goal to increase the frequency of the Homepage visit? Is our goal to offer more flexibility around home page settings? Who would determine this flexibility? 

This could get complicated. We should think about this thoroughly to minimize edge cases and fallout down the road.
Flags: needinfo?(alam) → needinfo?(bbermes)
> 2. If you close explicitly via the app list, instead of restoring the session, it shows the homepage on restart.

This statement is incorrect. Chrome always restores your session. I'm not sure what I was doing to see the homepage behavior.

Even if your only open tab is a Chrome new tab, that's what shows.

> Is our goal to increase the frequency of the Homepage visit? Is our goal to offer more flexibility around home page settings? Who would determine this flexibility? 

Our primary goal is to allow partners to show a homepage that works.
Here's some info about other browsers:


1: UC and QQ Browser
Can redirect to homepage that operator require, when create new tab.

2: Chrome
When create new tab, redirect to Google, but provide a shortcut button to access homepage.

3: Opera
Can redirect to homepage that operator require, when create new tab.
They keep website navigation that function with firefox similar.
User can access website navigation by slide(left to right) on screen.
I personally like what Opera does, and it seems like the most straightforward to implement.

Add an additional item on the panel for the homepage?
Sebastian/Anthony, can we do the following:

- Closing the last tab completely closes the browser so that on restart the homepage is shown." for distribution versions only?
- In addition to that, could we add 1-2 tiles for their homepage on our top sites default
Flags: needinfo?(s.kaspari)
Flags: needinfo?(bbermes)
Flags: needinfo?(alam)
Chrome also puts an icon on the toolbar for quick access to the homepage.

Have we ever had a homepage icon?

I can't figure out how to set homepage in Opera or UC browser. I'm contacting our partner to see if they have any information.
(In reply to Barbara Bermes [:barbara] - NI please! from comment #7)
> - Closing the last tab completely closes the browser so that on restart the
> homepage is shown." for distribution versions only?

Technically this might be possible. I wouldn't change that for all distributions but instead introduce a pref for it. The goal here is to get to the homepage more often/easily, right? I'd be interested to hear antlam's take on this because this doesn't sound like it is the most intuitive way (even though Chrome does this too).


(In reply to Barbara Bermes [:barbara] - NI please! from comment #7)
> - In addition to that, could we add 1-2 tiles for their homepage on our top
> sites default

For the current top sites implementation this is already possible: A distribution bookmark can be "pinned" - This means it won't show up in the bookmarks panel but instead be listed as top site:
https://wiki.mozilla.org/Mobile/Distribution_Files#Bookmarks

For the new "new tab" experience that we are developing for Activity Stream we have been thinking about this a little bit. Eventually we would like to allow partners, but also add-on developers, to inject their content into the experience.
Flags: needinfo?(s.kaspari)
We brought up the tiles, but AT&T wasn't interested.

I think if we could put their homepage in a panel widget, that would be an interesting idea.

We're trying to figure out something short term here to meet the need. That's why we are looking to chrome parity.

Any thoughts on having an easy way to get to home? Home button?
Blocks: 1314038
(In reply to Barbara Bermes [:barbara] - NI please! from comment #7)
> Sebastian/Anthony, can we do the following:
> 
> - Closing the last tab completely closes the browser so that on restart the
> homepage is shown." for distribution versions only?

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> (In reply to Barbara Bermes [:barbara] - NI please! from comment #7)
> > - Closing the last tab completely closes the browser so that on restart the
> > homepage is shown." for distribution versions only?
> 
> Technically this might be possible. I wouldn't change that for all
> distributions but instead introduce a pref for it. The goal here is to get
> to the homepage more often/easily, right? I'd be interested to hear antlam's
> take on this because this doesn't sound like it is the most intuitive way
> (even though Chrome does this too).

Is this the solution we landed on via Vidyo? I can't remember what we decided here.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(bbermes)
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #11)
> Is this the solution we landed on via Vidyo? I can't remember what we
> decided here.

Mh, I don't think we ever made a decision, did we? Either I wasn't part of that or I don't remember. :)

Introducing the (hidden) pref for distributions is probably the simplest way and won't affect existing users; however it's also something that can break and nobody will notice. Although I wonder if this is something that would support use cases of regular users too and whether we should show this inside settings.
Flags: needinfo?(s.kaspari)
See Also: → 1332441
There's more context in bug 1210290.

But I'll post the final design here since this is being tracked by product.
Discussed the following with Anthony:

SCENARIO 1: Homepage is set by user or partner (I use example.com as an example)
 
- First time —> example.com is shown
- Last tab closed -> closes app
- New tab -> about:home unless user/partner checked off “set for new tabs” (Anthony to post UI), then it would be example.com
- Tap in URL bar: about:home
- Default (session restored on start): Existing tabs show up
- User overwrites default: session not restored on start: example.com

SCENARIO 2: No homepage set, default about:home

- First time —> about:home is shown
- Last tab closed -> closes app
- New tab -> about:home 
- Tap in URL bar: about:home
- Default (session restored on start): Existing tabs show up
- User overwrites default: session not restored on start: about:home
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mozilla)
Flags: needinfo?(bbermes)
Flags: needinfo?(alam)
This looks like it would satisfy the partners (and get rid of the wizard in scenario 1).

Only other remaining item would be putting a home button somewhere in the UI (similar to Firefox iOS)
Flags: needinfo?(mozilla)
Attaching spec. 

LTR:

1) User presses "Set a Homepage" pref
2) User presses "Homepage - Firefox Home" item
3) User changes to a custom URL for the Homepage (medium.com/@antlam in this example).

Michelle, any quick thoughts on the copy here? I've taken an initial stab at most of the strings here but if you have better suggestions :)
Flags: needinfo?(alam) → needinfo?(mheubusch)
(In reply to Mike Kaply [:mkaply] from comment #16)
> This looks like it would satisfy the partners (and get rid of the wizard in
> scenario 1).
> 
> Only other remaining item would be putting a home button somewhere in the UI
> (similar to Firefox iOS)

As for the home button UI, do you consider this a very important ask by partners?

Anthony, thoughts on that idea in general?
Flags: needinfo?(alam)
> As for the home button UI, do you consider this a very important ask by partners?

They have asked for it, but it's also important for MozillaOnline. I think we should solve for both.
The scenarios sound good to me. Although it feels weird that we give up so easily on features that we heavily invest in to improve retention (new tab + first run).

> - Last tab closed -> closes app

This is something I'd like to move behind an (invisible?) setting that partners can enable.
Flags: needinfo?(s.kaspari)
> The scenarios sound good to me. Although it feels weird that we give up so easily on features that we heavily invest in to improve retention (new tab + first run).

Honestly, if we could come up with a solution that simply put the homepage in the first page of the new tab widget, I'd be find with that as a solution. I'm not completely sold on replacing our new tab with a page.

As far as the firstrun wizard goes, Is there data that shows it improves retention?

I think for most partners, it just feels like a barrier. Especially when you're integrating the browser as the default on the device. If you launch a URL in the default browser and the user has never started Firefox, won't they get the wizard instead of seeing the URL?

Have we thought about showing the "first run panel" later. Like as a "Hey, want to know how to do more with Firefox"? It seems like a lot of work to go through just to start using the browser. Have we done any tests to see if people read the panels instead of just skipping through them?

>> - Last tab closed -> closes app
> This is something I'd like to move behind an (invisible?) setting that partners can enable.

FYI, Chrome uses the fact that a homepage exists to determine this. The idea is that if a user has chosen a homepage, they probably want to see it when the browser starts up. This mechanism makes them more likely to see it.
(In reply to Barbara Bermes [:barbara] - NI please! from comment #18)
> (In reply to Mike Kaply [:mkaply] from comment #16)
> > This looks like it would satisfy the partners (and get rid of the wizard in
> > scenario 1).
> > 
> > Only other remaining item would be putting a home button somewhere in the UI
> > (similar to Firefox iOS)
> 
> As for the home button UI, do you consider this a very important ask by
> partners?
> 
> Anthony, thoughts on that idea in general?


Home button UI will be more problematic and probably out of scope of this bug. Especially since we're looking to touch a lot of the visual design of our Toolbar/Action bar as a part of the Photon work.

If there's not a heavy prescription for the design aspect here, I'd suggest having a menu item in the mean time. 

Thoughts here? Does that help?
Flags: needinfo?(alam)
(In reply to Mike Kaply [:mkaply] from comment #21)
> I think for most partners, it just feels like a barrier. Especially when
> you're integrating the browser as the default on the device. If you launch a
> URL in the default browser and the user has never started Firefox, won't
> they get the wizard instead of seeing the URL?

If so then this would be a regression. We have code in place to only show the tour if this is a normal launch. If an external URL is opened then the tour should not show up.


(In reply to Mike Kaply [:mkaply] from comment #21)
> Have we thought about showing the "first run panel" later. Like as a "Hey,
> want to know how to do more with Firefox"? It seems like a lot of work to go
> through just to start using the browser.

I agree and personally I'm not a fan of "explaining an app" before getting a chance to actually use the app. I guess I derailed this bug a bit. Sorry. :)
> If so then this would be a regression. We have code in place to only show the tour if this is a normal launch. If an external URL is opened then the tour should not show up.

That's good to know.

We've been testing the firstrun stuff for a while. Do we have data? Should the tests be changed?
Has development been scheduled for this?
Assignee: nobody → cnevinchen
Action items
1. See if comment 15 is still valid. If not, fix it.
2. See if comment 23 is still valid. If not, fix it.
3. Add "Go to Home Page" in menu item.
4. See if there's any data related to homepage experience.

Please feel free to comment. If that works for you I'll bring this up in our scrum/backlog meeting.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mozilla)
Flags: needinfo?(alam)
Comment 15 is the primary issue here. I wouldn't worry about 23, I trust sebastian.

The main thing partners want is to see the homepage on new tab. I wish there was some better way, but I can't think of any.

As far as homepage in the menu, that one was primarily for China and they want it elsewhere. See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1332441

So that should be handled in that bug.

This bug is focused on homepage access.
Flags: needinfo?(mozilla)
Thanks Nevin! 

I agree with Mike, let's leave the menu item out of this bug's scope.
Flags: needinfo?(alam)
Looks like you already got the answers you wanted? Otherwise please re-flag. :)
Flags: needinfo?(s.kaspari)
IIRC, this bug was considered too big for one sprint so let's split it into two separate items if possible.
Would it be possible to put some general time parameters around this?

We're getting asked from a partner to commit to a release that will have this.
(In reply to Mike Kaply [:mkaply] from comment #31)
> Would it be possible to put some general time parameters around this?
> 
> We're getting asked from a partner to commit to a release that will have
> this.

Here is the plan:
1. Scope of this one (1293713) is to provide a way for partner configurable pref to "also load for any new tab".
2. A follow up bug will be created for the user facing interface as denoted in attachment 8835713 [details] 
3. There will be telemetry relevant efforts as well 

So #1 is committed in the current sprint (~Mar/16). 
The rest efforts incl. #2, #3, testing, and stabilization will take a few more weeks. 
That means the reasonable target to be v56.
> That means the reasonable target to be v56.

That's assuming this rides the release trains. If we were to uplift it, what release do you think we could get this in?
If there are any string changes then it would need to ride the trains.
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review121444

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:408
(Diff revision 1)
>  
>          int tabId = tab.getId();
>          removeTab(tabId);
> -
>          if (nextTab == null) {
> -            nextTab = loadUrl(AboutPages.HOME, LOADURL_NEW_TAB);
> +            nextTab = loadUrl(HomeConfig.getHomepage(mAppContext), LOADURL_NEW_TAB);

Don't we have to do [something similar as here](https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1575), that is check whether the homepage pref is empty and then substitute `AboutPages.HOME` if it is?

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:990
(Diff revision 1)
>  
>          return tabToSelect;
>      }
>  
>      public Tab addTab() {
> -        return loadUrl(AboutPages.HOME, Tabs.LOADURL_NEW_TAB);
> +        return loadUrl(HomeConfig.getHomepage(mAppContext), Tabs.LOADURL_NEW_TAB);

Ditto.
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

wip. Will check localization and setup from distribution file later.
This patch isn't working for me. Preference has no effect (I still see the about:home tabs).
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

wip. Added UI. I haven't tested with Distribution files yet.
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review121444

> Don't we have to do [something similar as here](https://dxr.mozilla.org/mozilla-central/rev/f9362554866b327700c7f9b18050d7b7eb3d2b23/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1575), that is check whether the homepage pref is empty and then substitute `AboutPages.HOME` if it is?

Thanks.. I'll fix it in the private static method HomeConfig.getHomeForEveryNewTab
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

This is my test preferences.json
{
  "Global": {
    "id": "NevinSample3",
    "version": 3.0,
    "about": "Nevin Distribution"
  },
  "Preferences": {
    "privacy.donottrackheader.enabled": true
  },
  "AndroidPreferences": {
    "distribution.read_partner_customizations_provider": true,
    "homepage.partner": "apple.com",
    "homepage": "mozilla.org",
    "load.for.every.new.tab": true
  },
  "LocalizablePreferences": {
    "browser.search.defaultenginename": "Bugzilla@MozillaSearch"
  }
}
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review123478

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2267
(Diff revision 4)
>          return (mTabsPanel != null && mTabsPanel.isShown());
>      }
>  
>      @Override
>      public String getHomepage() {
> -        final SharedPreferences preferences = GeckoSharedPrefs.forProfile(this);
> +        return HomeConfig.getHomepage(this);

Are we able to remove any of the imports from this file since code went to HomeConfig.java?

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfig.java:1707
(Diff revision 4)
>  
>      public static HomeConfig getDefault(Context context) {
>          return new HomeConfig(new HomeConfigPrefsBackend(context));
>      }
> +
> +    public static String getHomepage(Context context) {

I think this is named improperly. It's job is to get the homepage, but it also has an effect with the new tab.

I think it would make more sense to create a generic getHomepage function and then do the forEverynewTab check separately.

So opening a new tab would not call "getHomepage", it would call "GetHomepageForNewTab" which would call getHomepage.

getHomepage should always retur the homepage.

::: mobile/android/base/java/org/mozilla/gecko/preferences/DistroSharedPrefsImport.java:48
(Diff revision 4)
>                  Log.e(LOGTAG, "Unable to completely process Android Preferences JSON.", e);
>                  continue;
>              }
>  
>              // We currently don't support Float preferences.
> -            if (value instanceof String) {
> +            // PREFS_LOAD_FOR_EVERY_NEW_TAB should be for App

Was there a reason you made it an app preference instead of a shared preference?

I'd rather not make this change if not necessary.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:889
(Diff revision 4)
>                      }
>                  } else if (PREFS_HOMEPAGE.equals(key)) {
>                          String setUrl = GeckoSharedPrefs.forProfile(getBaseContext()).getString(PREFS_HOMEPAGE, AboutPages.HOME);
>                          setHomePageSummary(pref, setUrl);
>                          pref.setOnPreferenceChangeListener(this);
> +                } else if (PREFS_LOAD_FOR_EVERY_NEW_TAB.equals(key)) {

Remove debug code.

::: mobile/android/base/resources/xml/preferences_home.xml:20
(Diff revision 4)
>                  android:title="@string/home_homepage_title"
>                  android:persistent="false"
>                  android:dialogLayout="@layout/preference_set_homepage"/>
>  
> +        <SwitchPreference
> +            android:key="android.not_a_preference.load.for.every.new.tab"

We should come up with a better name for this preference.

newtab.load_homepage or something like that.
We need to finalize the text of this settings item so I can attempt to get it translated.

Current text is:

Also load for every new tab

Which I think is a little wordy.

It's already in a section with a title "Homepage"

Under a section called "Set a homepage"

It will be a checkbox essentially

Some ideas:

Open in new tab
Use in new tab
Use as new tab
Set as new tab
Use for new tab

Also, should it be disabled if homepage is set to about:home?
Hi Anthony
Please help give us some input about comment 46.
Thank you!
Flags: needinfo?(alam)
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review121942

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:408
(Diff revision 1)
>  
>          int tabId = tab.getId();
>          removeTab(tabId);
> -
>          if (nextTab == null) {
> -            nextTab = loadUrl(AboutPages.HOME, LOADURL_NEW_TAB);
> +            nextTab = loadUrl(HomeConfig.getHomepage(mAppContext), LOADURL_NEW_TAB);

Thanks a lot ! I'll add more check later
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review123478

> Are we able to remove any of the imports from this file since code went to HomeConfig.java?

Oops.. sorry ... I guess you mean org.mozilla.gecko.distribution.PartnerBrowserCustomizationsClient;.
Yes. I can remove that.

> Was there a reason you made it an app preference instead of a shared preference?
> 
> I'd rather not make this change if not necessary.

Local variable appReferences and sharedPreferences are an instance of SharedPreferences class. But they have different scope. appReferences is an app-scoped SharedPreferences instance. 
sharedPreferences is an profile-scoped SharedPreferences instance.
My original idea is that this partner distribution setting should be for entire app(forApp). Not profile specific.

> We should come up with a better name for this preference.
> 
> newtab.load_homepage or something like that.

How about "homepage.for.new.tab.enabled" ?
(In reply to Mike Kaply [:mkaply] from comment #46) 
> Open in new tab
> Use in new tab
> Use as new tab
> Set as new tab
> Use for new tab

Looking at https://material.io/guidelines/patterns/settings.html#settings-usage, it seems like starting with "Also" is a pattern in their design guidelines and that's where I got it from.

How about "Also use for new tabs" ? Michelle might also have thoughts here

> Also, should it be disabled if homepage is set to about:home?

I think it's fine to have it enabled if homepage is set to about:home. Since every new tab _will_ be "about:home" right?
Flags: needinfo?(alam)
(In reply to Nevin Chen [:nechen] from comment #49)
> Comment on attachment 8846506 [details]
> Bug 1293713 - Make new tab action respect HomePage preference.
> 
> https://reviewboard.mozilla.org/r/119566/#review123478

> > Was there a reason you made it an app preference instead of a shared preference?
> > 
> > I'd rather not make this change if not necessary.
> 
> Local variable appReferences and sharedPreferences are an instance of
> SharedPreferences class. But they have different scope. appReferences is an
> app-scoped SharedPreferences instance. 
> sharedPreferences is an profile-scoped SharedPreferences instance.
> My original idea is that this partner distribution setting should be for
> entire app(forApp). Not profile specific.

I understand what you were thinking, but considering how poorly profile/app is handled on Android anyway, I don't think it matters.

I think we should use the existing distribution architecture (SharedPreferences) instead of adding a special case for this one preference.
(In reply to Mike Kaply [:mkaply] from comment #52)
> I understand what you were thinking, but considering how poorly profile/app
> is handled on Android anyway, I don't think it matters.
> 
> I think we should use the existing distribution architecture
> (SharedPreferences) instead of adding a special case for this one preference.

Another concern is about our GeckoPreferenceFragment[1] is using forApp sharedPreference.
If I want to set homepage.for.new.tab.enabled as profile sharedPreference then I can't reuse GeckoPreferenceFragment.

I can make GeckoPreferenceFragment to handle profile sharedPreference, but I think it's better to let DistroSharedPrefsImport to accept app preference as well.

Do you know if there's any partner had customized app preference before?
Cause in current design, I think it's not possible for distribution file to update app preference....


[1]http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java#74
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mozilla)
Depends on: 1295675
Flags: needinfo?(s.kaspari)
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review124360

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2264
(Diff revision 5)
>          return (mTabsPanel != null && mTabsPanel.isShown());
>      }
>  
>      @Override
>      public String getHomepage() {
> -        final SharedPreferences preferences = GeckoSharedPrefs.forProfile(this);
> +        return HomeConfig.GetHomepageForNewTab(this);

It's nice to move this logic in a new place. However HomeConfig is exclusively about the panel configuration - adding this code here doesn't make sense.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfig.java:1711
(Diff revision 5)
> +
> +    public static String GetHomepageForNewTab(Context context) {
> +        final SharedPreferences preferences = GeckoSharedPrefs.forApp(context);
> +        final boolean forEveryNewTab = preferences.getBoolean(GeckoPreferences.PREFS_LOAD_FOR_EVERY_NEW_TAB, false);
> +        if (forEveryNewTab) {
> +            return getHomepage(context);

This method can return null. I guess in this case we should fallback to AboutPages.HOME?

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfig.java:1717
(Diff revision 5)
> +        } else {
> +            return AboutPages.HOME;
> +        }
> +    }
> +
> +    private static String getHomepage(Context context) {

nit: @Nullable annotation

::: mobile/android/base/java/org/mozilla/gecko/preferences/DistroSharedPrefsImport.java:50
(Diff revision 5)
> +            if (fullKey.equals(GeckoPreferences.PREFS_LOAD_FOR_EVERY_NEW_TAB) && value instanceof Boolean) {
> +                appPreferences.putBoolean(fullKey, (boolean) value);

This is too hacky. We should either fix bug 1295675 first to allow setting application preferences from a distribution. Or just use a profile preference for this one too.
Attachment #8846506 - Flags: review?(s.kaspari)
After bug 1295675 lands, this patch should work.
Attached the related sample distribution for this bug

in preferences.json
{
  "Global": {
    "id": "NevinSample4",
    "version": 4.0,
    "about": "Nevin Distribution"
  },
  "Preferences": {
    "privacy.donottrackheader.enabled": true
  },
  "ApplicationPreferences": {
    "homepage.for.new.tab.enabled": true
  },
  "AndroidPreferences": {
    "distribution.read_partner_customizations_provider": true,
    "homepage": "mozilla.org"
  },
  "LocalizablePreferences": {
    "browser.search.defaultenginename": "Bugzilla@MozillaSearch"
  }
}
(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #32)

> 1. Scope of this one (1293713) is to provide a way for partner configurable
> pref to "also load for any new tab".
> 2. A follow up bug will be created for the user facing interface as denoted
> in attachment 8835713 [details] 
> 3. There will be telemetry relevant efforts as well 

Hey Nevin,
I'm not sure if I had confirmed with you somewhere else, but let's make sure here.
Is there relevant telemetry effort need to be added for this?
Yes.. 
Probes that I can think of is... 
1. How partners customize our distribution files
2. How users interact with them
3. How does this feature help our user retension / DAU / MAU...
4. See if there's any data related to homepage experience.

For 1-3, we need to think what we want to analysis and add new probes (and file a new bug for that)
For 4, I can only find bug 1021751. And a ui event about user change their home page setting :(please find android.not_a_preference.homepage in https://sql.telemetry.mozilla.org/queries/59#633
Flags: needinfo?(whuang)
Clearing needinfo do to bug 1295675
Flags: needinfo?(mozilla)
(In reply to Nevin Chen [:nechen] from comment #58)
> Yes.. 
> Probes that I can think of is... 
> 1. How partners customize our distribution files
> 2. How users interact with them
> 3. How does this feature help our user retension / DAU / MAU...
> 4. See if there's any data related to homepage experience.
> 
> For 1-3, we need to think what we want to analysis and add new probes (and
> file a new bug for that)
> For 4, I can only find bug 1021751. And a ui event about user change their
> home page setting :(please find android.not_a_preference.homepage in
> https://sql.telemetry.mozilla.org/queries/59#633

Redirecting the NI to Joe. 
We'll probably need a meeting to conclude these new telemetry probes (incl. standalone mode)
Flags: needinfo?(whuang) → needinfo?(jcheng)
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review126712
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review125336

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2264
(Diff revision 6)
>          return (mTabsPanel != null && mTabsPanel.isShown());
>      }
>  
>      @Override
>      public String getHomepage() {
> -        final SharedPreferences preferences = GeckoSharedPrefs.forProfile(this);
> +        return Tabs.getHomepageForNewTab(this);

This is called from loadStartupTab(). However getHomepageForNewTab() only returns the homepage if PREFS_HOMEPAGE_FOR_EVERY_NEW_TAB is set. Doesn't this break the homepage for regular users that set one in settings?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1606
(Diff revision 6)
>  
>          Tabs.getInstance().loadUrlWithIntentExtras(url, intent, flags);
>      }
>  
>      public String getHomepage() {
> -        return null;
> +        return Tabs.getHomepageForNewTab(this);

I don't think we want to set this here? This method is overriden in BrowserApp.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:1193
(Diff revision 6)
>      }
> +
> +    public static String getHomepageForNewTab(Context context) {
> +        final SharedPreferences preferences = GeckoSharedPrefs.forApp(context);
> +        final boolean forEveryNewTab = preferences.getBoolean(GeckoPreferences.PREFS_HOMEPAGE_FOR_EVERY_NEW_TAB, false);
> +        String homePage = getHomepage(context);

Let's move this inside "if (forEveryNewTab) { ... }" - No need to read all those content providers / preferences if we are not going to need it.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:1193
(Diff revision 6)
>      }
> +
> +    public static String getHomepageForNewTab(Context context) {
> +        final SharedPreferences preferences = GeckoSharedPrefs.forApp(context);
> +        final boolean forEveryNewTab = preferences.getBoolean(GeckoPreferences.PREFS_HOMEPAGE_FOR_EVERY_NEW_TAB, false);
> +        String homePage = getHomepage(context);

nit: final

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:1194
(Diff revision 6)
> +
> +    public static String getHomepageForNewTab(Context context) {
> +        final SharedPreferences preferences = GeckoSharedPrefs.forApp(context);
> +        final boolean forEveryNewTab = preferences.getBoolean(GeckoPreferences.PREFS_HOMEPAGE_FOR_EVERY_NEW_TAB, false);
> +        String homePage = getHomepage(context);
> +        if (forEveryNewTab && homePage != null) {

Let's use !TextUtils.isEmpty(homepage)

::: mobile/android/base/locales/en-US/android_strings.dtd:218
(Diff revision 6)
>  <!ENTITY pref_home_updates2 "Content updates">
>  <!ENTITY pref_home_updates_enabled "Enabled">
>  <!ENTITY pref_home_updates_wifi "Only over Wi-Fi">
>  <!ENTITY pref_category_home_homepage "Homepage">
> +<!ENTITY home_homepage_every_new_tab "Also use for new tabs">
> +<!-- Localization note (home_homepage_every_new_tab): The user will see a switch to determine if the

nit: the comment should be above the string

::: mobile/android/base/resources/xml/preferences_home.xml:20
(Diff revision 6)
>                  android:title="@string/home_homepage_title"
>                  android:persistent="false"
>                  android:dialogLayout="@layout/preference_set_homepage"/>
>  
> +        <SwitchPreference
> +            android:key="android.not_a_preference.homepage.for.new.tab.enabled"

the name of the preference itself usually uses underscores, like android.not_a_preference.new_tab_load_homepage
Attachment #8846506 - Flags: review?(s.kaspari)
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review127980

::: mobile/android/base/resources/xml/preferences_home.xml:19
(Diff revision 8)
> +        <SwitchPreference
> +            android:key="android.not_a_preference.homepage.for_new_tab_enabled"
> +            android:title="@string/home_homepage_every_new_tab"
> +            android:defaultValue="false"
> +            android:persistent="true"/>

Was UX involved in this? I won't block landing this - but I think we should look at this again in a follow-up bug. This switch only makes sense when a custom homepage was set. Otherwise it just does nothing.

For partners we won't need this, so maybe we can land this without the switch and then let's look at the switch in the next bug? Maybe we don't want to expose this at all.
Attachment #8846506 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #65)
> Comment on attachment 8846506 [details]

> Was UX involved in this? I won't block landing this - but I think we should
> look at this again in a follow-up bug. This switch only makes sense when a
> custom homepage was set. Otherwise it just does nothing.

Yes, please see comment 17


> For partners we won't need this, so maybe we can land this without the
> switch and then let's look at the switch in the next bug? 

I agree. I think the problem here was we want to use about:home as much as possible and didn't respect the user's custom home page setting enough. This bug is to fix this issue using a switch. Which I think is a compromise between the two and is also a solution.
> For partners we won't need this, so maybe we can land this without the switch and then let's look at the switch in the next bug? Maybe we don't want to expose this at all.

If we allow partners to do this, don't we want to provide users a way to turn it off? Or do you think we're OK allowing partners to override new tab for good?

On another note, I think we need a better pref name. homepage.for_new_tab_enabled seems a little odd. I think the focus should be on the new tab. Maybe

newtab.show_homepage
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review127980

> Was UX involved in this? I won't block landing this - but I think we should look at this again in a follow-up bug. This switch only makes sense when a custom homepage was set. Otherwise it just does nothing.
> 
> For partners we won't need this, so maybe we can land this without the switch and then let's look at the switch in the next bug? Maybe we don't want to expose this at all.

1. Yes. Please see comment 17.
2. We need the swtich to let users override partner's setting.
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

The sample distribution file now is 
{
  "Global": {
    "id": "NevinSample4",
    "version": 4.0,
    "about": "Nevin Distribution"
  },
  "Preferences": {
    "privacy.donottrackheader.enabled": true
  },
  "AndroidPreferences": {
    "homepage": "mozilla.org",
    "newtab.load_homepage": true
  },
  "LocalizablePreferences": {
    "browser.search.defaultenginename": "Bugzilla@MozillaSearch"
  }
}
(In reply to Nevin Chen [:nechen] from comment #71)
> Comment on attachment 8846506 [details]
> Bug 1293713 - Make new tab action respect HomePage preference.
> 
> The sample distribution file now is 

Sorry, the correct sample is this one 
{
  "Global": {
    "id": "NevinSample4",
    "version": 4.0,
    "about": "Nevin Distribution"
  },
  "Preferences": {
    "privacy.donottrackheader.enabled": true
  },
  "AndroidPreferences": {
    "homepage": "mozilla.org",
    "newtab.load_homepage": true
  },
  "ApplicationPreferences": {
    "newtab.load_homepage": "true"
  },
  "LocalizablePreferences": {
    "browser.search.defaultenginename": "Bugzilla@MozillaSearch"
  }
}
This document also edited: https://wiki.mozilla.org/Mobile/Distribution_Files
Comment on attachment 8846506 [details]
Bug 1293713 - Make new tab action respect HomePage preference.

https://reviewboard.mozilla.org/r/119566/#review130070
Attachment #8846506 - Flags: review?(mozilla) → review+
If we could get this checked in tonight, that would be great. Trying to get strings translate.

Should the string be 

Also use for new tabs
or
Also use for new tab

?
(In reply to Mike Kaply [:mkaply] from comment #74)
> If we could get this checked in tonight, that would be great. Trying to get
> strings translate.
> 
> Should the string be 
> 
> Also use for new tabs
> or
> Also use for new tab
> 
> ?

I think "Also use for new tabs" from antlam is better. I'll do that now.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 835cacbfdd43 -d 5c6da13f6288: rebasing 387479:835cacbfdd43 "Bug 1293713 - Make new tab action respect HomePage preference. r=mkaply,sebastian" (tip)
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
merging mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
merging mobile/android/base/locales/en-US/android_strings.dtd
merging mobile/android/base/strings.xml.in
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 835cacbfdd43 -d 0a3044dc01d2: rebasing 387484:835cacbfdd43 "Bug 1293713 - Make new tab action respect HomePage preference. r=mkaply,sebastian" (tip)
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
merging mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
merging mobile/android/base/locales/en-US/android_strings.dtd
merging mobile/android/base/strings.xml.in
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
just landed it mannually
Keywords: checkin-needed
(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #60)
> (In reply to Nevin Chen [:nechen] from comment #58)
> > Yes.. 
> > Probes that I can think of is... 
> > 1. How partners customize our distribution files
> > 2. How users interact with them
> > 3. How does this feature help our user retension / DAU / MAU...
> > 4. See if there's any data related to homepage experience.
> > 
> > For 1-3, we need to think what we want to analysis and add new probes (and
> > file a new bug for that)
> > For 4, I can only find bug 1021751. And a ui event about user change their
> > home page setting :(please find android.not_a_preference.homepage in
> > https://sql.telemetry.mozilla.org/queries/59#633
> 
> Redirecting the NI to Joe. 
> We'll probably need a meeting to conclude these new telemetry probes (incl.
> standalone mode)

had a chat with Nevin
It looks like thru the core ping we can know the distribution and in addition, if the user opt-in, we can know if the user turn the feature off manually in settings. I think this may be enough initially for us to understand if users are turning off this feature
Flags: needinfo?(jcheng)
> just landed it mannually

I can't find the entry in treeherder. Do you have a link?

I'm surprised this hasn't moved from inbound to central at this point. We need this to go in for translation.
Flags: needinfo?(cnevinchen)
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0612106ae03e
Make new tab action respect HomePage preference. r=mkaply,sebastian
Sorry I didn't land it correctly. Is there anything I can do to make it up?
Flags: needinfo?(mozilla)
Flags: needinfo?(kbrosnan)
Flags: needinfo?(cnevinchen)
Hi Francesco, Mike
May I know the reason why yesterday should be the last day to submit a patch for translation? 
Is there anything I can do to make it up?
Thank you!
Flags: needinfo?(francesco.lodolo)
It wasn't the last day. But, by missing a landing this Friday, we'll likely lose 4/5 days for translation, out of 10 remaining in Aurora.

Aurora is string frozen, i.e. you're not supposed to land strings. Here you're landing strings at the very end of the cycle, and once moved to beta several locales won't be able to translate them.
Flags: needinfo?(francesco.lodolo)
https://hg.mozilla.org/mozilla-central/rev/0612106ae03e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(In reply to Francesco Lodolo [:flod] from comment #85)
> It wasn't the last day. But, by missing a landing this Friday, we'll likely
> lose 4/5 days for translation, out of 10 remaining in Aurora.
> 

Thanks a lot !
But why last Friday so critical? Is it related to release schedule?
I want to be more careful in the future.. so it'll be great if you advise me where I can find the information for this day.
I can't find it on [1] nor [2]
Much appreciated!

[1] https://wiki.mozilla.org/RapidRelease/Calendar
[2] https://calendar.google.com/calendar/embed?src=mozilla.com_2d37383433353432352d3939@resource.calendar.google.com
Flags: needinfo?(francesco.lodolo)
Let's start with the obvious: you should NOT land strings in mozilla-aurora. Features should land in mozilla-central and ride the trains.

If you need to land strings in mozilla-aurora, you should do it at the beginning of the cycle to get as much time as possible for localization. After that, there's no schedule saying that it should be one specific day of the cycle, just common sense and case-by-case evaluation. If you land at the end of the cycle, you need to be aware that you'll make a) localizers angry b) your feature will likely remain untranslated for a cycle in several languages.

Also, these notions are soon going to become obsolete with project Dawn.
Flags: needinfo?(francesco.lodolo)
Yes, to be even more clear, I was trying to go against the process here. This is something that should be done as rarely as possible.
Flags: needinfo?(mozilla)
(In reply to Francesco Lodolo [:flod] from comment #88)
> Also, these notions are soon going to become obsolete with project Dawn.

As I know 55 won't branch to aurora (according to project Dawn) so I'm wondering what the new l10n process will be like.
(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #90)
> As I know 55 won't branch to aurora (according to project Dawn) so I'm
> wondering what the new l10n process will be like.

Currently figuring it out. Patches will land in mozilla-central, and at least for 54 we should leave beta alone as much as possible.
Flags: needinfo?(kbrosnan)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: