Closed Bug 551889 Opened 14 years ago Closed 14 years ago

Start Page redesign

Categories

(Firefox for Android Graveyard :: General, defect)

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

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file, 6 obsolete files)

patch 2
112.35 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
Attached patch WIP 1 (obsolete) — — Splinter Review 
If Fennec opens in a new profile, we launch the about:firstrun page. Otherwise, we open with about:blank and show the awesomescreen.

We should add support for a true start page. Basic requirements are:
* Create an about:home with specialized content
* Provide a way in preferences to set the home page. Options include: None (use awesomescreen), Default (use about:home) or Use the current page in the active browser.
Flags: in-testsuite?
Flags: in-litmus?
Attached patch WIP 2 (obsolete) — — Splinter Review 
This patch adds basic content to the homepage:
* list of recent URLs
* button to show bookmarks

http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-homepage-01.png
Assignee: nobody → mark.finkle
Attachment #432074 - Attachment is obsolete: true
Thinking of what's left to do here:
* Blue highlight when picking a URL
* Any other sections?
* Add-on section support?
* How to access the homepage after browsing away (button? bookmark?)
* Show current homepage title in preferences (as footnote text)
Oh and:
* Style it prettier
Attached patch WIP 3 (obsolete) — — Splinter Review 
Oops. Missed the aboutHome.xhtml file
Attachment #432635 - Attachment is obsolete: true
Attached patch WIP 4 (obsolete) — — Splinter Review 
Adds support for basic session store:
* Does not restore anything
* Does not store history or form data
* Only stores windows and tabs (url & title)
* No real access to the service from the outside

Adds a simple "Recent Tabs" to the home page:
* Reads the sessionstore.bak file to see what tabs were open when the browser was closed

Adds some notifications to support session store like on desktop:
* browser-lastwindow-close-requested
* browser-lastwindow-close-granted
* This code can be used to add the close warning in bug 545395

Requesting feedback from vingtetun on the new code. This is not completely final yet and has some debug code in it, but it's close to ready.
Attachment #432648 - Attachment is obsolete: true
Attachment #433416 - Flags: feedback?(21)
I really like this new homepage, it looks "so more web" contrary to the functional style of the awesome panel.
But, I'm a little scarred that this page share so much features with the awesome panel that it will have a difficulty in a near future to decide where to put such new feature - but maybe it is just me that has not followed the project till the start!

Otherwise, without having put a look at the source code:
  * when we open a new tab, i assume we're supposed to see this new page
  * It feels strange that the "See All Bookmarks" shortcuts is under the history while it looks like it is above in the awesome panel
  * Blue glow highlight on links/rows
  * Avoid scroll
    - well, I know this is very hard because of the screens sizes, orientations, etc., but I think it will be really appreciate if you don't need to pan to use the main fonctionnalities: history and bookmarks at least
  * Support an addon section
    - grummph, this is supposed to be bug 496654 which in my mind will finish to add some UI somewhere near the awesome panel, but I already start to change my mind with such a homepage!
    - by the way, if we add such a feature we *really* need a button (sigh) somewhere to go to this page
  * Show a subset of bookmarks, maybe those tagged with the "homepage" keyword?


I think this page is still a big improvment compare to what we are doing actually.

Now, I'll look at the code :)
screenshot of the "recent tabs" section:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-homepage-03.png

We need to add favicons. We can pull those from the nsIFavIconService.
Comment on attachment 433416 [details] [diff] [review]
WIP 4

>+  <link rel="stylesheet" href="chrome://browser/skin/aboutHome.css" type="text/css"/>
>+  <script type="application/javascript;version=1.8"><![CDATA[
>+    var Ci = Components.interfaces, Cc = Components.classes;
>+    var gChromeWin = null;

use "let"

>+    
>+    function getChromeWin() {
>+      if (!gChromeWin) {
>+        gChromeWin = window
>+                    .QueryInterface(Ci.nsIInterfaceRequestor)
>+                    .getInterface(Ci.nsIWebNavigation)
>+                    .QueryInterface(Ci.nsIDocShellTreeItem)
>+                    .rootTreeItem
>+                    .QueryInterface(Ci.nsIInterfaceRequestor)
>+                    .getInterface(Ci.nsIDOMWindow)
>+                    .QueryInterface(Ci.nsIDOMChromeWindow);

hmm, I'm not really sure of the right practice here doing "window.QueryInterface(Ci.nsIInterfaceRequestor)" and aligning from it sounds better for me

>+
>+    function initLinks() {
>+      let ac = Cc["@mozilla.org/autocomplete/search;1?name=history"].getService(Ci.nsIAutoCompleteSearch);
>+      ac.startSearch("", "", null, {
>+        onSearchResult: function(search, result) {
>+          let count = Math.min(5, result.matchCount);
>+          let list = document.getElementById("recentLinks");
>+          for (let i=0; i<count; i++) {
>+
>+            outer.appendChild(inner);
>+            list.appendChild(outer);
>+          }
>+        }
>+      });
>+    }

maybe use a documentFragment even if I'm not really sure that will change anything for so few links

>+    function initTabs() {
>+      let dirService = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);
>+      let session = dirService.get("ProfD", Ci.nsILocalFile);
>+      session.append("sessionstore.bak");
>+dump("1\n")
>+      let data = JSON.parse(_readFile(session));

Did that slow down startup? Or could we use something async into _readFile?

>+      let list = document.getElementById("recentTabs");
>+      for (let i=0; i<count; i++) {
>+        let url = tabs[i].url;
>+        let favicon = "";//result.getImageAt(i);

this explain why I was not seeing icons for tabs!

>+        outer.appendChild(inner);
>+        list.appendChild(outer);
>+      }
>+    }

Use documentFragment here too

>+<body dir="&locale.dir;" onload="init();">

Maybe we could use https://developer.mozilla.org/En/Making_Sure_Your_Theme_Works_with_RTL_Locales#Gecko_1.9.2_and_later for something like this?
I've to admit that I'm not really sure this apply to "webpages".


>+XPCOMUtils.defineLazyServiceGetter(this, "gObs",
>+                                   "@mozilla.org/observer-service;1",
>+                                   "nsIObserverService");
>+
>+XPCOMUtils.defineLazyServiceGetter(this, "gWM",
>+                                   "@mozilla.org/appshell/window-mediator;1",
>+                                   "nsIWindowMediator");
>+

Could we use some nice names here?

>+function SessionStore() { }

Add ";" at the end of the line


>+
>+  onTabClose: function ss_onTabClose(aWindow, aTab) {
>+    // notify the tabbrowser that the tab state will be retrieved for the last time
>+    // (so that extension authors can easily set data on soon-to-be-closed tabs)
>+    var event = aWindow.document.createEvent("Events");
>+    event.initEvent("SSTabClosing", true, false);
>+    aTab.dispatchEvent(event);

Since we don't have a tabbrowser, the comments is probably out-of-date and maybe we don't need this source code?

>+
>+  onTabSelect: function ss_onTabSelect(aWindow) {
>+    //if (this._loadState == STATE_RUNNING)
>+      //this._windows[aWindow.__SSID].selected = aWindow.gBrowser.tabContainer.selectedIndex;

I guess this is for debug purpose

>+
>+#recentLinks {
>+  background-color: white;
>+  padding: 5px;
>+  border: 2px solid #e6e5e3;
>+}

...

We're using .links in the hildon theme for the css rules

---

That's a great feature you add with that!
Attachment #433416 - Flags: feedback?(21) → feedback+
For strings/options in the related preference, here's what I'd suggest:

http://www.flickr.com/photos/madhava_work/4461157146/
Attached patch patch (obsolete) — — Splinter Review 
This patch:
* Adds proper styling to the about:home HTML
* Removes all debugging code
* Makes some string changes to the startpage preferences
* Currently displays "recent tabs, "remote tabs (weave)" and "recommended add-ons" on the startpage
* Hides any section if there are no items to display or weave is not installed
* Adds a nightly and official branding logo

No changes to the Session Store code in this patch. Same as it was in the last patch.
Attachment #433416 - Attachment is obsolete: true
Attachment #435087 - Flags: review?(21)
Comment on attachment 435087 [details] [diff] [review]
patch

>+    function _readFile(aFile) {
>+      try {
>+        let stream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
>+        stream.init(aFile, 0x01, 0, 0);
>+        let cvstream = Cc["@mozilla.org/intl/converter-input-stream;1"].createInstance(Ci.nsIConverterInputStream);
>+  
>+        let fileSize = stream.available();  
>+        cvstream.init(stream, "UTF-8", fileSize, Ci.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);
>+        let data = {};
>+        cvstream.readString(fileSize, data);
>+        let content = data.value;
>+        cvstream.close();
>+        return content.replace(/\r\n?/g, "\n");
>+      }
>+      catch (ex) { Cu.reportError(ex); }

Cu is undefined here

>+    function openTabs(aURLs) {
>+      let BrowserUI = getChromeWin().BrowserUI;
>+      urls = aURLs.split("|");

let urls = aURLs.split("|");

>+        let uri = chromeWin.makeURI(url);
>+        let favicon = chromeWin.gFaviconService.getFaviconImageForPage(uri).spec;
>+        let title = tabs[i].title;
>+
>+        let outer = document.createElement("div");
>+

I think we miss the blue highlight here. We could probably use the xhtml role attribute (http://www.w3.org/TR/xhtml-role/), to indicate that all the div of this page could be highlight during a mousedown, and add in our code a handler for that here http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1721

>+    function initWeave() {
>+      var em = Cc["@mozilla.org/extensions/manager;1"].getService(Ci.nsIExtensionManager);  
>+      var weave = em.getItemForID("{340c2bbc-ce74-4362-90b5-7c26312808ef}");

Use let

>+    function goToAddons(aSearchString) {
>+      let chromeWin = getChromeWin();
>+      let BrowserUI = chromeWin.BrowserUI;
>+      BrowserUI.showPanel("addons-container");
>+      if (aSearchString) {

You could just return early if there is no string

>+    function initAddons() {
>+      let repository = "@mozilla.org/extensions/addon-repository;1";
>+      let repo = Cc[repository].createInstance(Ci.nsIAddonRepository);
>+      if (repo.isSearching)
>+        repo.cancelSearch();
>+      repo.retrieveRecommendedAddons(2, RecommendedSearchResults);

2 can be a const, or even better a preference!

+    <div id="remoteTabs" class="section-row" onclick="openRemoteTabs();">
+      <img class="favicon" src="chrome://browser/skin/images/remotetabs-32.png"/>
+      <span>&aboutHome.remoteTabs;</span>
+    </div>

This doesn't crop is the window is too small for the text to fit which is not what I'm expecting about a button.

>diff --git a/themes/wince/aboutHome.css b/themes/wince/aboutHome.css
>new file mode 100644
>--- /dev/null
>+++ b/themes/wince/aboutHome.css

The wince css is really, really different from the hildon's one. (It still mention bookmarks)


The content didn't resize itself when the window size change, due to the meta viewport. This is another bug i think.

Also, if one of the last webpage didn't have a title the tabs section layout is broken:
http://vingtetun.org/tmp/no-title.png.


r- for the wince css and the little layout bug
(In reply to comment #11)
> (From update of attachment 435087 [details] [diff] [review])
> >+      catch (ex) { Cu.reportError(ex); }
> 
> Cu is undefined here

Fixed

> >+    function openTabs(aURLs) {
> >+      let BrowserUI = getChromeWin().BrowserUI;
> >+      urls = aURLs.split("|");
> 
> let urls = aURLs.split("|");

Fixed

> I think we miss the blue highlight here. We could probably use the xhtml role
> attribute (http://www.w3.org/TR/xhtml-role/), to indicate that all the div of
> this page could be highlight during a mousedown, and add in our code a handler
> for that here
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1721

Yeah. I'll file a followup bug

> >+      var em = Cc["@mozilla.org/extensions/manager;1"].getService(Ci.nsIExtensionManager);  
> >+      var weave = em.getItemForID("{340c2bbc-ce74-4362-90b5-7c26312808ef}");
> 
> Use let

Fixed

> >+    function goToAddons(aSearchString) {
> >+      let chromeWin = getChromeWin();
> >+      let BrowserUI = chromeWin.BrowserUI;
> >+      BrowserUI.showPanel("addons-container");
> >+      if (aSearchString) {
> 
> You could just return early if there is no string

If you pass no search string, we should take you to the add-ons manager

> >+      repo.retrieveRecommendedAddons(2, RecommendedSearchResults);
> 
> 2 can be a const, or even better a preference!

Made a const for now

> +    <div id="remoteTabs" class="section-row" onclick="openRemoteTabs();">

> +    </div>
> 
> This doesn't crop is the window is too small for the text to fit which is not
> what I'm expecting about a button.

Fixed

> The wince css is really, really different from the hildon's one. (It still
> mention bookmarks)

Fixed. But I didn't try to tweak the CSS for winmo

> The content didn't resize itself when the window size change, due to the meta
> viewport. This is another bug i think.

Yeah, I'll file a bug (if we don't already have one)

> Also, if one of the last webpage didn't have a title the tabs section layout is
> broken:
> http://vingtetun.org/tmp/no-title.png.

Fixed by skipping title-less pages
(In reply to comment #8)

> >+    var Ci = Components.interfaces, Cc = Components.classes;
> >+    var gChromeWin = null;
> 
> use "let"

Fixed

> >+        gChromeWin = window
> >+                    .QueryInterface(Ci.nsIInterfaceRequestor)
> >+                    .getInterface(Ci.nsIWebNavigation)
> >+                    .QueryInterface(Ci.nsIDocShellTreeItem)
> >+                    .rootTreeItem
> >+                    .QueryInterface(Ci.nsIInterfaceRequestor)
> >+                    .getInterface(Ci.nsIDOMWindow)
> >+                    .QueryInterface(Ci.nsIDOMChromeWindow);
> 
> hmm, I'm not really sure of the right practice here doing
> "window.QueryInterface(Ci.nsIInterfaceRequestor)" and aligning from it sounds
> better for me

It's ugly no matter what. I just left it as Gavin had it in firstrun.xhtml

> maybe use a documentFragment even if I'm not really sure that will change
> anything for so few links

I haven't noticed any slowness, but we can use a document fragment later if we need to speed things up.

I'll file an "optimize startup page" bug

> 
> Did that slow down startup? Or could we use something async into _readFile?

Same

> >+        let favicon = "";//result.getImageAt(i);
> 
> this explain why I was not seeing icons for tabs!

Fixed

> >+<body dir="&locale.dir;" onload="init();">
> 
> Maybe we could use
> https://developer.mozilla.org/En/Making_Sure_Your_Theme_Works_with_RTL_Locales#Gecko_1.9.2_and_later
> for something like this?
> I've to admit that I'm not really sure this apply to "webpages".

Me either. This is what we were doing in the firstrun.xhml, so I just copied for now.

> >+XPCOMUtils.defineLazyServiceGetter(this, "gObs",
> >+                                   "@mozilla.org/observer-service;1",
> >+                                   "nsIObserverService");
> >+
> >+XPCOMUtils.defineLazyServiceGetter(this, "gWM",
> >+                                   "@mozilla.org/appshell/window-mediator;1",
> >+                                   "nsIWindowMediator");
> >+
> 
> Could we use some nice names here?

Fixed

> >+  onTabClose: function ss_onTabClose(aWindow, aTab) {
> >+    var event = aWindow.document.createEvent("Events");
> >+    event.initEvent("SSTabClosing", true, false);
> >+    aTab.dispatchEvent(event);
> 
> Since we don't have a tabbrowser, the comments is probably out-of-date and
> maybe we don't need this source code?

I removed this for now, but we will add it back when we support a true SessionStore service.
Attached patch patch 2 — — Splinter Review 
Addressed review comments
Attachment #435087 - Attachment is obsolete: true
Attachment #435156 - Flags: review?(21)
Comment on attachment 435156 [details] [diff] [review]
patch 2

This is looking very nice, congrats for the look and feel!
Attachment #435156 - Flags: review?(21) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/7f60ba499cbe
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Some l10n comments.

Having aboutHome.start="Start" (aboutHome.dtd) and homepage.default="&brandShortName; Start" (preferences.dtd) is not a great idea, since in the first case the &brandShortName; is hard coded into the xhtml page and they both refer to the same thing.

homepage.custom=Custom in browser.properties. No idea at all of what this string does (no l10n comments, no clue from the key name).
(In reply to comment #17)
> Some l10n comments.
> 
> Having aboutHome.start="Start" (aboutHome.dtd) and
> homepage.default="&brandShortName; Start" (preferences.dtd) is not a great
> idea, since in the first case the &brandShortName; is hard coded into the xhtml
> page and they both refer to the same thing.

True the result in both cases is "Firefox Start", but the string in the XHTML each part, "Firefox" and "Start" are in two different <span>s and are styled differently.

> homepage.custom=Custom in browser.properties. No idea at all of what this
> string does (no l10n comments, no clue from the key name).

I'll add a comment to the file. Basically, if the user has chosen a web page to be the startpage, we show the "Custom" label on the menulist widget.

(is that comment ok for the properties file?)
(In reply to comment #18)
> True the result in both cases is "Firefox Start", but the string in the XHTML
> each part, "Firefox" and "Start" are in two different <span>s and are styled
> differently.

Ok, but if I want to translate "Firefox Start" as something like "Start point for Firefox" what can I do? Firefox+noun is not a common structure for non English languages

> (is that comment ok for the properties file?)

Yes, it sounds fine
(In reply to comment #19)
> (In reply to comment #18)
> > True the result in both cases is "Firefox Start", but the string in the XHTML
> > each part, "Firefox" and "Start" are in two different <span>s and are styled
> > differently.
> 
> Ok, but if I want to translate "Firefox Start" as something like "Start point
> for Firefox" what can I do? Firefox+noun is not a common structure for non
> English languages

I guess "Firefox Start" is more of a title and not a sentence. It is short for "Firefox Startpage"
(In reply to comment #20)
> I guess "Firefox Start" is more of a title and not a sentence. It is short for
> "Firefox Startpage"

I know, and I can work around this for Italian, but I'm not sure that's so easy for other languages.

Adding community@localization.bugs in cc to catch other localizers' opinion.
Comment on attachment 435156 [details] [diff] [review]
patch 2

>+++ b/locales/en-US/chrome/aboutHome.dtd
>@@ -0,0 +1,6 @@
>+<!ENTITY aboutHome.recommendedAddons            "Add-ons for your Firefox">

s/Firefox/&brandShortName; ?
Attached patch Patch (obsolete) — — Splinter Review 
(In reply to comment #22)
> (From update of attachment 435156 [details] [diff] [review])
> >+++ b/locales/en-US/chrome/aboutHome.dtd
> >@@ -0,0 +1,6 @@
> >+<!ENTITY aboutHome.recommendedAddons            "Add-ons for your Firefox">
> 
> s/Firefox/&brandShortName; ?

Oups. I've missed it while reviewing.
Thanks for catching it!
Attachment #435382 - Flags: review?(mark.finkle)
Comment on attachment 435382 [details] [diff] [review]
Patch

Given that some teams have that string already, should probably rev the entity key.
(In reply to comment #24)
> (From update of attachment 435382 [details] [diff] [review])
> Given that some teams have that string already, should probably rev the entity
> key.

What is the right practive here?

Is duplicating <!ENTITY aboutHome.recommendedAddons "Add-ons for your Firefox"> to <!ENTITY aboutHome.recommendedAddons2 "Add-ons for your &brandShortName;"> and use &aboutHome.recommendedAddons2; for this release and remove/rename the wrong one for the next release a good solution?
(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 435382 [details] [diff] [review] [details])
> > Given that some teams have that string already, should probably rev the entity
> > key.
> 
> What is the right practive here?

Changing to:
<!ENTITY aboutHome.recommendedAddons2 "Add-ons for your &brandShortName;">

is the current "best" practice. Changing now...
Attachment #435382 - Attachment is obsolete: true
Attachment #435382 - Flags: review?(mark.finkle)
The rather undocumented string change in http://hg.mozilla.org/mobile-browser/rev/15b55dc65e87 hasn't been caught by http://hg.mozilla.org/releases/l10n-mozilla-1.9.2/pt-PT/annotate/2589c6ebd632/mobile/chrome/browser.properties. Mind checking the rest of the locales?

Note that the comment for the en-US change didn't get updated, too.

I'm not sure what folks expect to be the outcome of this in terms of the quality of the localized product.
bumped the entity:
http://hg.mozilla.org/mobile-browser/rev/b045a79434ef

I've said it before, but I'm in the mood to say it again: entity bumping for string changes is a horrible practice! we need to fix our tools!
(In reply to comment #29)
> I've said it before, but I'm in the mood to say it again: entity bumping for
> string changes is a horrible practice! we need to fix our tools!

Why don't we just not localize Fennec until you're happy?
verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100329 Namoroka/3.6.3pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.3a4pre) Gecko/20100329 Namoroka/3.7a4pre Fennec/1.1a2pre
Status: RESOLVED → VERIFIED
litmus testcases:

https://litmus.mozilla.org/show_test.cgi?id=11657
https://litmus.mozilla.org/show_test.cgi?id=11658

have been created to regression test this page.
Flags: in-litmus? → in-litmus+
Depends on: 556313
Blocks: 557623
You need to log in before you can comment on or make changes to this bug.