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

VERIFIED FIXED in Firefox 4.0

Status

defect
P2
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: aza, Assigned: mitcho)

Tracking

Trunk
Firefox 4.0
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments, 8 obsolete attachments)

176.66 KB, image/png
Details
149.56 KB, image/png
Details
19.06 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → 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
Status: NEW → ASSIGNED
Depends on: 596201
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?
Priority: -- → P2
(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
Blocks: 591831
Duplicate of this bug: 597549
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!
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! :)
Agreed.
Blocks: 597043
blocking2.0: --- → ?
Target Milestone: --- → Firefox 4.0
Posted patch Patch v1 (obsolete) — Splinter Review
This patch *might* apply cleanly, but it'll definitely apply cleanly if you apply the patch for bug 593157 first.
Attachment #476701 - Flags: ui-review?(aza)
Attachment #476701 - Flags: feedback?(ian)
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?
Depends on: 597931
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.
The patch isn't applying cleanly :(
Updated so as to no longer break other tests, by setting a user_pref.
Attachment #476701 - Attachment is obsolete: true
Attachment #477020 - Flags: ui-review?(aza)
Attachment #477020 - Flags: feedback?(ian)
Attachment #476701 - Flags: ui-review?(aza)
Attachment #476701 - Flags: feedback?(ian)
For Aza: in case you're comfortable ui-reviewing that way.
Attachment #477021 - Flags: ui-review?(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.
Attachment #477020 - Flags: ui-review?(aza) → ui-review+
Attachment #477021 - Flags: ui-review?(aza) → ui-review+
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.
Attachment #477020 - Flags: feedback?(ian) → feedback+
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)}.
Attachment #477210 - Flags: ui-review?(aza)
> * 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.
Attachment #477020 - Attachment is obsolete: true
Attachment #477218 - Flags: review?(dietrich)
blocking2.0: ? → betaN+
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.
Attachment #477218 - Flags: review?(dietrich) → review?(dolske)
Looking good.
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).
Attachment #477218 - Flags: review?(dolske) → review-
(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.
(In reply to comment #22)

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

Ah, ok. Fine with me.
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.
Attachment #477218 - Attachment is obsolete: true
Attachment #479762 - Flags: review?(dolske)
Attachment #479762 - Attachment is obsolete: true
Attachment #479769 - Flags: review?(dolske)
Attachment #479762 - Flags: review?(dolske)
Attachment #477210 - Flags: ui-review?(aza) → ui-review+
Attachment #479769 - Flags: review?(dolske) → review+
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.
There's no reason to prefer %APP% over "firefox", the patch is fine as is.
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.
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.
Posted patch Patch for checkin (obsolete) — Splinter Review
Attachment #479769 - Attachment is obsolete: true
Realized we haven't done a try run recently (post-recent-p-c merge) so I've sent it to try: 1c29981378ac.
Attachment #482133 - Attachment is obsolete: true
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.
Per that bug, we should not hard-code the locale code in the URL.
Last patch (which was updated for rot) passed try. Updated with correct URL, based on Gavin's comment above, and passed locally.
Attachment #485597 - Attachment is obsolete: true
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?
(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
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.
Pushed to try: 12b5143f74ad
Attachment #485628 - Attachment is obsolete: true
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.
(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.
Landed on panorama-central:

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/db8a93e30aa8
Keywords: checkin-needed
Whiteboard: [on-panorama-central]
Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/e08e1f42bf11
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [on-panorama-central]
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?
(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?
Yes, thanks!
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.