596075 - Move First-Run Video To Be A Tab Pointing To A Mozilla-Hosted Web Page

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Aza Raskin [:aza]]

Instead of having a new type of object in the Panorama space for the video we should have:

* A tab (scaled large so that the contents are visible) pointing to a Mozilla-hosted web page with the video tutorial set to auto-start.

This enables us to do translations and updates post-hoc.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Just to spell this out... we will follow our current code and create a single large group with all the tabs, and a small group on the top right.

The small group on the top right will:
1. be the active group
2. have one tab, which points to the welcome video/page
3. its one tab will be active
4. be sufficiently large

Comment 2

[Aza Raskin [:aza]]

Actually,

* The single large group will be the active group (else, using command-t will open a new tab in the intro-group, which is unwanted).

Also, I was imagining it would be a tab that wasn't grouped. Why did you want it to be a group?

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #2)
> Actually,
> 
> * The single large group will be the active group (else, using command-t will
> open a new tab in the intro-group, which is unwanted).
> 
> Also, I was imagining it would be a tab that wasn't grouped. Why did you want
> it to be a group?

Okay. That works too. Glad I checked. :D

Comment 5

[Laxminarayan G Kamath A]

Or why dont you just make it an html5 presentation and ship it with FF? A couple of JPEGs and a bit of html and js scripts shouldnt be much data. If the accompanying audio is small, that would be great too.  Even if you dont ship it with FF, It will result in a lot lesser bandwidth usage. And it will be a great html5 demo too!

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Laxminarayan, we unfortunately can't do that as a big reason why we're doing this is to make it a web property which is localizable using the regular localization workflow. We can still use cool HTML5, though! :)

Comment 7

[Laxminarayan G Kamath A]

Agreed.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1

This patch *might* apply cleanly, but it'll definitely apply cleanly if you apply the patch for bug 593157 first.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Dietrich and/or Dolske: the test I wrote in bug 593157 basically creates a "first run" startup and a "non-first run" startup, and checks for those proper states. I'd like to suggest that there be a single test which covers both bug 593157 and this bug. Would that be okay?

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

In working on this patch, I discovered bug 597931, which I now have blocking this bug. It's hard to get the positioning of the welcome tab right if setBounds doesn't work as expected... particularly if there are discrepancies between groups and orphan tabs.

Comment 11

[Aza Raskin [:aza]]

The patch isn't applying cleanly :(

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2: requires patch v4 for bug 593157

Updated so as to no longer break other tests, by setting a user_pref.

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Screenshot of first run

For Aza: in case you're comfortable ui-reviewing that way.

Comment 14

[Aza Raskin [:aza]]

Mitcho this is looking great. A couple thoughts:

* Align the tab to have its top aligned with the main group?
* When the window is at a smaller size (say only 900px in width) how does it look?

ui+ with these addressed.

Comment 15

[Ian Gilman [:iangilman]]

Comment on attachment 477020 [details] [diff] [review]
Patch v2: requires patch v4 for bug 593157

+      let onChildRemoved = function(data) {
+        parent.removeSubscriber(newTabItem,'childRemoved',onChildRemoved);
+        let aspect = TabItems.tabHeight / TabItems.tabWidth;
+        let welcomeBounds = new Rect(box.right + padding, box.top,
+                                     welcomeWidth, welcomeWidth * aspect);
+        newTabItem.setBounds(welcomeBounds, true);
+        GroupItems.setActiveGroupItem(groupItem);
+      };
+      parent.addSubscriber(newTabItem,'childRemoved',onChildRemoved);
+      newTabItem.parent.remove(newTabItem);

Do we need to use the subscriber mechanism here? I think you can count on the removal happening completely during the parent.remove, so you can just stick the remaining code directly below that instead of inside a handler. 

+  let tabItems = contentWindow.TabItems.getItems();
+  let orphanTabs = 0;
+  tabItems.forEach(function(tabItem) {
+    if (!tabItem.parent)
+      orphanTabs++;
+  });

Use: 

  let orphanTabCount = contentWindow.GroupItems.getOrphanedTabs().length; 

f+ with those changes.

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Screenshot of first run on small screen

The layout algorithm is such that the group will be the smaller of {2/3 of the screen width, the screen's safe bounds - the width of the welcome tab (now set to 300)}.

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

> * Align the tab to have its top aligned with the main group?

This is what bug 597931 is for.

> * When the window is at a smaller size (say only 900px in width) how does it
> look?

See the new attachment.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3: fixed Ian's suggestions

Comment 19

[Ian Gilman [:iangilman]]

Comment on attachment 477218 [details] [diff] [review]
Patch v3: fixed Ian's suggestions

Reassigning review to dolske while dietrich is jammed up with b7 stuff.

Comment 20

[Aza Raskin [:aza]]

Looking good.

Comment 21

[Justin Dolske [:Dolske]]

Comment on attachment 477218 [details] [diff] [review]
Patch v3: fixed Ian's suggestions

>+++ b/browser/base/content/tabview/ui.js
...
>+    let $actions = iQ("#actions");
>+    if ($actions)
>+      pageBounds.width -= $actions.width();

This seems unrelated, spin off to a separate bug?


>-      let infoItem = new InfoItem(infoBox);
>-      infoItem.html(html);

This seems to have been the only thing using InfoItem, so hg remove infoitems.js now?

>+      let url = "http://azarask.in/projects/tabcandy/tutorial/";

We can't ship this. :)

Fantastic that you've already got bug 596201 spinning up a Mozilla-hosted page.

This code will need to pull the page URL from a pref and format it (for locales). Take a look at how the firstrun page (startup.homepage_welcome_url) is handled, as an example. (APP/VERSION not needed for your usage).

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #21)
> Comment on attachment 477218 [details] [diff] [review]
> Patch v3: fixed Ian's suggestions
> 
> >+++ b/browser/base/content/tabview/ui.js
> ...
> >+    let $actions = iQ("#actions");
> >+    if ($actions)
> >+      pageBounds.width -= $actions.width();
> 
> This seems unrelated, spin off to a separate bug?

This is related. This is in order to position the new welcome tab in a place that does not overlap with the #actions. When the original first run code was written, #actions did not exist. I would think it's appropriate to put this within the scope of this bug, no?

> >-      let infoItem = new InfoItem(infoBox);
> >-      infoItem.html(html);
> 
> This seems to have been the only thing using InfoItem, so hg remove
> infoitems.js now?

Yes! Good call.

> >+      let url = "http://azarask.in/projects/tabcandy/tutorial/";
> 
> We can't ship this. :)
> 
> Fantastic that you've already got bug 596201 spinning up a Mozilla-hosted page.
> 
> This code will need to pull the page URL from a pref and format it (for
> locales). Take a look at how the firstrun page (startup.homepage_welcome_url)
> is handled, as an example. (APP/VERSION not needed for your usage).

Ah, okay. Will do.

Comment 23

[Justin Dolske [:Dolske]]

(In reply to comment #22)

> > This seems unrelated, spin off to a separate bug?
> 
> This is related.

Ah, ok. Fine with me.

Comment 24

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v4: made changes suggested by Dolske

Requesting review for the removal of infoitem and the implementation of browser.panorama.welcome_url. We can't land this until we have a fixed URL.

For this patch, I assume http://www.mozilla.com/%LOCALE%/firefox/panorama/ as the welcome url. %LOCALE% actually doesn't look like it gets replaced in browser/branding/nightly and /unofficial... so maybe we need a different static one or should assume en-US for the nightlies.

Comment 25

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v5: URL's %LOCALE% is now replaced correctly

Comment 26

[Justin Dolske [:Dolske]]

The only thing I'm slightly unsure of is if the firefox-branding.js changes should use %APP% instead of an explicit "firefox". I don't think it's _useful_ -- I don't think you want different pages for nightly/alpha builds -- but maybe there's some nitpicky reason for it.

CC'd gavin for his opinion, but I think it's fine to land as-is. We can fix that in a followup if there's some reason to do so.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

There's no reason to prefer %APP% over "firefox", the patch is fine as is.

Comment 28

[Michael Yoshitaka Erlewine [:mitcho]]

Should we wait to land this patch until the http://www.mozilla.com/en-US/firefox/panorama/ target actually goes somewhere, even with a simple dummy page? I'll also inquire on bug 596201.

Comment 29

[Justin Dolske [:Dolske]]

It's fine to land this as-is, even if the mozilla.com page is currently a 404. Delaying a bit for a placeholder page would also be acceptable.

Comment 30

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 31

[Michael Yoshitaka Erlewine [:mitcho]]

Realized we haven't done a try run recently (post-recent-p-c merge) so I've sent it to try: 1c29981378ac.

Comment 32

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin, updated for rot

Comment 33

[Michael Yoshitaka Erlewine [:mitcho]]

Sent to try: 007b4e8ca6cf

As there's been no discussion on bug 596201 in the past two weeks, and I'm not thrilled about continuously fixing this patch for rot, if the try comes back clean I will request checkin, even though the URL currently 404's.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Per that bug, we should not hard-code the locale code in the URL.

Comment 35

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin, updated with correct URL

Last patch (which was updated for rot) passed try. Updated with correct URL, based on Gavin's comment above, and passed locally.

Comment 36

[Ian Gilman [:iangilman]]

I just pushed this to try along with a bunch of other patches (0f1d9b3bd42d),  and am getting this failure consistently:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1288050364.1288051249.11044.gz

Possibly related to this change?

Comment 37

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #36)
> I just pushed this to try along with a bunch of other patches (0f1d9b3bd42d), 
> and am getting this failure consistently:
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1288050364.1288051249.11044.gz
> 
> Possibly related to this change?

I think that would be odd. Since that last change I made, I did actually push to try again as well, just to be safe. It came back clean. Revision 0b76d958a650

Comment 38

[Ian Gilman [:iangilman]]

Well, this patch adds the test that's failing, so I suppose maybe it's something else that's breaking it. At any rate, I can repro consistently on my local machine, and removing this patch makes the difference. 

Do you have the latest mozilla-central code? The only other thing I had applied was bug 598383.

Comment 39

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin, updated test for bug 598600

Pushed to try: 12b5143f74ad

Comment 40

[Michael Yoshitaka Erlewine [:mitcho]]

The try run mostly passed, except that one windows build's browser-chrome run simply timed out. I'm not sure what to make of that.

Comment 41

[Ian Gilman [:iangilman]]

(In reply to comment #40)
> The try run mostly passed, except that one windows build's browser-chrome run
> simply timed out. I'm not sure what to make of that.

Probably just random weirdness.

Comment 42

[Ian Gilman [:iangilman]]

Landed on panorama-central:

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/db8a93e30aa8

Comment 43

[Ian Gilman [:iangilman]]

Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/e08e1f42bf11

Comment 44

[(no longer active)]

Reading the patch here to see if I need to do anything special for RTL support, I'm kind of puzzled.  Given the new value of the pref, and the fact that the video URL seems to be removed from the code base in this patch, how can one trigger this UI for testing?

Comment 45

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #44)
> Reading the patch here to see if I need to do anything special for RTL support,
> I'm kind of puzzled.  Given the new value of the pref, and the fact that the
> video URL seems to be removed from the code base in this patch, how can one
> trigger this UI for testing?

Mmm, I'm not sure where your confusion lies.

The way the test works: mochitest runs with browser.panorama.experienced_first_run = true so it doesn't get triggered on other tests. We temporarily set experienced_first_run = false, then open a new Panorama in a new window, so this triggers the firstrun experience... then we close it and reset experienced_first_run = true.

The video URL: we use the value of browser.panorama.welcome_url, which is given a default in the firefox-branding.js files.

Does that explain everything?

Comment 46

[(no longer active)]

Yes, thanks!

Comment 47

[Tracy Walker [:tracy]]

First run tab is pointing to http://www.mozilla.com/en-US/firefox/panorama/ which is 404ing right.  

Verifying, but the content needs to be posted.