851130 - Work - Try moving about:start to a separate XUL document loaded in a <xul:browser>

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect)

Description

[Matt Brubeck (:mbrubeck)]

We'd like to make it possible to load the about:start content in a <browser>.  This would let us take advantage of scrolling optimizations like AsyncPanZoomController.

(It could also potentially allow us to load about:start as a regular tab instead of a chrome overlay.  We haven't decided whether to make that change, since it has both advantages and disadvantages, but once we've done the basic work here it should be easy to try it out.)

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This is a totally non-working snapshot of my current work, just in case anyone is curious.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2

Updated to apply to mozilla-central tip, in case anyone else wants to play with it.  Added a number of fixes/hacks to get more functional stuff working, plus various modifications just for my debugging purposes.  Scrolling is not actually working in this version (something going wrong within the browser binding, I think) so I can't test smoothness.  Working on that now.

Comment 3

[Matt Brubeck (:mbrubeck)]

This WIP patch bitrots constantly because of all the code it moves around, but it should be fairly straightforward to rebase if someone wants to pick it up, or to re-apply the same idea from scratch.

A quick brain-dump before I leave for vacation:

Right now, all of our touch scrolling is handled in front-end code that is somewhat coupled to the browser binding and the <deck id="browsers">.  This makes it harder than it should be to introduce other touch-scrollable <browser> elements.  Maybe this will change when we integrate with AsyncPanZoomController.

Until then, I think the easiest way to fix this bug is to load about:start into the normal <browser> for the current tab just like a normal page, rather than keep it in a separate element that overlays the <browser>.  That is, just make about:start work like all our other about: pages.  This would not only make scrolling work very easily, but would also simplify a number of things related to showing/hiding the start screen during navigation.

This wasn't feasible when I first started working on this bug because of how the autocomplete popup was integrated into the start screen.  With the new design in bug 831910 and bug 883390, I think we will want to separate the autocomplete popup from the start screen, which should make it easier to move the start screen into a normal "web page".

Comment 4

[Jonathan Wilde [:jwilde]]

Additional information: as far as I can tell, desktop Firefox uses pages loaded into the standard <browser> elements for about:newtab, but has a preloader (http://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserNewTabPreloader.jsm).

Comment 5

[Jim Mathies [:jimm]]

Matt, would you like me to try and pick this up? I'm no expert in this area but should be able to figure something out. We either need to fix this or fix the flicker / scroll perf problems. Both seem hard, but this appears to be the shortest pole to climb.

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #5)
> Matt, would you like me to try and pick this up? I'm no expert in this area
> but should be able to figure something out. We either need to fix this or
> fix the flicker / scroll perf problems. Both seem hard, but this appears to
> be the shortest pole to climb.

Sure, that would be great.  Let me know if you have any questions about my earlier patches or comments.

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

When bug 866304 lands, we'll still have an issue with the start UI appearing on top of prompts (bug 876042).

To fix bug 876042, we'll need the start UI to not be implemented as a XUL overlay. That's also the goal of this bug!

Let's make sure that whatever implementation we come up with for this bug also fixes our issue in bug 876042.

Comment 8

[Jim Mathies [:jimm]]

Attachment: shuffling things around v.1

Comment 9

[Jim Mathies [:jimm]]

Attachment: basics v.1

Comment 10

[Jim Mathies [:jimm]]

Attachment: apzc v.1

Hey Brian, curious what this 'StartUI.isStartPageVisible' special case in apzc.js is for the start page? FWIW, with the start page in a tab, apzc work great with it (aside from the flicker problem).

Comment 11

[Jim Mathies [:jimm]]

A couple things left to do here, snapped view which I know is totally messed up at this point, and getting tests working. Plus general testing for issues.

Since this is going to bit rot like crazy, I think I'll try to get this done and then file follow ups for anything else.

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 786314 [details] [diff] [review]
apzc v.1

Review of attachment 786314 [details] [diff] [review]:
-----------------------------------------------------------------

Yep I think we want to remove this special handling now. Before it needed a diff window util and element than the rest of the code.

::: browser/metro/profile/metro.js
@@ +24,5 @@
>  pref("metro.debug.selection.dumpEvents", false);
>  
>  // Enable off main thread compositing
>  pref("layers.offmainthreadcomposition.enabled", true);
> +pref("layers.async-pan-zoom.enabled", true);

I don't think we to enable this yet, and turning it on is tracked in a diff bug.

Comment 13

[Jim Mathies [:jimm]]

Attachment: get about:start working right v.1

Comment 14

[Jim Mathies [:jimm]]

Attachment: rollup v.1

Comment 15

[Jim Mathies [:jimm]]

Attachment: part 1 - code reshuffle v.1

Comment 16

[Jim Mathies [:jimm]]

Attachment: part 2 - remove StartUI object v.1

Comment 17

[Jim Mathies [:jimm]]

Attachment: part 3 - fixup start tab components v.1

Comment 18

[Jim Mathies [:jimm]]

Attachment: part 4 - apzc fixes v.1

Comment 19

[Jim Mathies [:jimm]]

Attachment: part 5 - about:start v.1

Comment 20

[Jim Mathies [:jimm]]

Attachment: part 6 - tests

Comment 21

[Jim Mathies [:jimm]]

This is 95% of the way there. The only remaining issue I have is with the browser_topsites.js test. The test runs fine stand alone but when it runs in the suite, it always times out due to grid binding issues.

Comment 22

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 23

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 24

[Jim Mathies [:jimm]]

The error I'm getting in topsites is - 

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_topsites.js | uncaught exception - TypeError: grid.clearAll is not a function at chrome://browser/content/TopSitesView.js:148

which happens on the first call to TopSitesTestHelper.updatePagesAndWait - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_topsites.js#185

Visually it's clear the top sites grid isn't getting refreshed. Rather strange since on its own this test passes.

Comment 25

[Jim Mathies [:jimm]]

Attachment: part 6 - tests v.1

Comment 26

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 27

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=984972e16e89

Comment 28

[Jim Mathies [:jimm]]

Attachment: part 7 - misc.

fixes:
- selection bug on grid items
- close tab button

Comment 29

[Jim Mathies [:jimm]]

Attachment: part 6 - tests

Comment 30

[Jim Mathies [:jimm]]

Attachment: part 7 - misc.

Comment 31

[Brian R. Bondy [:bbondy]]

Comment on attachment 786924 [details] [diff] [review]
part 4 - apzc fixes v.1

Review of attachment 786924 [details] [diff] [review]:
-----------------------------------------------------------------

You can r=me with the changes

::: browser/metro/base/content/input.js
@@ +363,5 @@
>        if (Date.now() - this._dragStartTime > kStopKineticPanOnDragTimeout)
>          this._kinetic._velocity.set(0, 0);
>  
>        // Start kinetic pan if we i) aren't using async pan zoom or ii) if we
>        // are on the start page, iii) If the scroll element is not browsers

nit: Comment needs adjusting

@@ +365,5 @@
>  
>        // Start kinetic pan if we i) aren't using async pan zoom or ii) if we
>        // are on the start page, iii) If the scroll element is not browsers
>        let deck = document.getElementById("browsers");
> +      if (!Services.prefs.getBoolPref(kAsyncPanZoomEnabled) &&

We want || here

Comment 32

[Jim Mathies [:jimm]]

Attachment: part 7 - misc.

added a couple fixes for the two remaining test failures on try.

Comment 33

[Jim Mathies [:jimm]]

Attachment: part 6 - tests

Comment 34

[Jim Mathies [:jimm]]

Attachment: part 7 - content

this hides about:start from our frame scripts.

Comment 35

[Jim Mathies [:jimm]]

Attachment: part 8 - misc.

Comment 36

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 37

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=dab1b56a24f2

Comment 38

[Jim Mathies [:jimm]]

Somewhere in here I had the browser var _richgridTileSizes fix, but I seem to have lost it in patch shuffles. Will post a follow up.

Comment 39

[Matt Brubeck (:mbrubeck)]

Comment on attachment 786922 [details] [diff] [review]
part 2 - remove StartUI object v.1

Review of attachment 786922 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I'm curious/concerned about the input.js code and how scrolling is handled for the start page.  Maybe it doesn't matter though as we switch to APZC for scrolling.

::: browser/metro/base/content/browser-ui.js
@@ +326,5 @@
> +      ContextUI.displayNavbar();
> +      Elements.contentShowing.setAttribute("disabled", "true");
> +      Elements.windowState.setAttribute("startpage", "true");
> +    } else if (aURI != "about:blank") { // about:blank is loaded briefly for new tabs; ignore it
> +      Elements.contentShowing.removeAttribute("disabled");

I think we no longer need to set/remove contentShowing[disabled] here, since the start page no longer covers the content browser.

::: browser/metro/base/content/browser.js
@@ +167,5 @@
>  
>        let self = this;
>        function loadStartupURI() {
>          let uri = activationURI || commandURL || Browser.getHomePage();
> +        if (uri == kStartURI) {

Can we get rid of this "if" block and let the "else" case below handle it instead?

::: browser/metro/base/content/input.js
@@ +451,5 @@
>      let qinterface = null;
> +
> +    // if element is content (but not the startui page), get the browser scroll interface
> +    if (!BrowserUI.isStartTabVisible &&
> +        elem.ownerDocument == Browser.selectedBrowser.contentDocument) {

Why is the isStartTabVisible check needed?  Can't we scroll the start page like a normal web page?  (Or is this going away in a later part of the patch series?)

::: browser/metro/base/content/startui/BookmarksView.js
@@ +59,5 @@
>    },
>  
>    handleItemClick: function bv_handleItemClick(aItem) {
>      let url = aItem.getAttribute("value");
> +    StartUI.goToURI(url);

Nit: This should be in the previous patch.

Comment 40

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 786921 [details] [diff] [review]
part 1 - code reshuffle v.1

Review of attachment 786921 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this shuffle, its bugged me from day 1.

::: browser/metro/base/content/startui/TopSitesView.js
@@ +197,5 @@
> +    this.isUpdating = false;
> +  },
> +
> +  forceReloadOfThumbnail: function forceReloadOfThumbnail(url) {
> +      let nodes = this._set.querySelectorAll('richgriditem[value="'+url+'"]');

Nit: can you fix this indentation while you're in here

Comment 41

[Matt Brubeck (:mbrubeck)]

Comment on attachment 787114 [details] [diff] [review]
part 7 - content

Review of attachment 787114 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to avoid special-casing about:start as much as possible.

::: browser/metro/base/content/contenthandlers/Content.js
@@ +156,5 @@
>     */
>  
>    handleEvent: function handleEvent(aEvent) {
> +    if (content.location.href == "about:start")
> +      return;

Is this necessary to prevent buggy behavior, or is it just to improve performance by avoiding unnecessary code?

In general it seems like we should treat about:start as much like any other page as possible, so maybe we should bail out only in more specific problematic areas.

::: browser/metro/base/content/contenthandlers/ContextMenuHandler.js
@@ +23,5 @@
>    },
>  
>    handleEvent: function ch_handleEvent(aEvent) {
> +    if (content.location.href == "about:start")
> +      return;

We should just make sure that the start page calls preventDefault() on any contextmenu events it handles; then this module should not have any impact.

::: browser/metro/base/content/contenthandlers/FormHelper.js
@@ +211,5 @@
>    },
>  
>    handleEvent: function formHelperHandleEvent(aEvent) {
> +    if (content.location.href == "about:start")
> +      return;

It seems like formHelper should not cause problems for about:start, and disabling it would prevent us from using form controls in future versions of about:start (like if we wanted to add some search UI).

If there are real problems addressed by this patch, we could check in this special-case code temporarily but we should file bugs to fix the underlying problems.

Comment 42

[Matt Brubeck (:mbrubeck)]

Comment on attachment 787116 [details] [diff] [review]
part 8 - misc.

Review of attachment 787116 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/ContextUI.js
@@ +284,5 @@
>          this.dismiss();
>          break;
> +      case "touchstart":
> +        if (!BrowserUI.isStartTabVisible) {
> +          this.dismiss();

Should we do this for the ToolPanelShown/AlertActive/etc. case above too?

::: browser/metro/base/content/browser.js
@@ +465,5 @@
>        return;
>      }
>  
> +    if ((aOptions && "forceClose" in aOptions && aOptions.forceClose) ||
> +        tab.browser.currentURI.spec == kStartURI) {

This is only needed because the "part 7" patch disabled the normal message-handling code, right?  I'd prefer to drop both that change and this one if possible.

Comment 43

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #39)
> Comment on attachment 786922 [details] [diff] [review]
> part 2 - remove StartUI object v.1
> 
> Review of attachment 786922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I'm curious/concerned about the input.js code and how
> scrolling is handled for the start page.  Maybe it doesn't matter though as
> we switch to APZC for scrolling.

input.js handles the scrolling..

> ::: browser/metro/base/content/input.js
> @@ +451,5 @@
> >      let qinterface = null;
> > +
> > +    // if element is content (but not the startui page), get the browser scroll interface
> > +    if (!BrowserUI.isStartTabVisible &&
> > +        elem.ownerDocument == Browser.selectedBrowser.contentDocument) {
> 
> Why is the isStartTabVisible check needed?  Can't we scroll the start page
> like a normal web page?  (Or is this going away in a later part of the patch
> series?)

When the 'get scroller' code here was getting the scroll interface on the browser scrolling didn't work. This check insures it gets 'start-scrollbox' via a qi rather than the browser.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#194

If we remove start-scrollbox from the layout the browser scroller would take over, not sure. I'll experiment around with it. I think I'd like to investigate this in a follow up though.

Comment 44

[Jim Mathies [:jimm]]

*I think* the browser scroller would take over, not sure.

Comment 45

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #41)
> In general it seems like we should treat about:start as much like any other
> page as possible, so maybe we should bail out only in more specific
> problematic areas.

Sounds good, I put this in to be safe, I'll pull it out and look for issues.

Comment 46

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #42)
> Comment on attachment 787116 [details] [diff] [review]
> part 8 - misc.
> 
> Review of attachment 787116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/ContextUI.js
> @@ +284,5 @@
> >          this.dismiss();
> >          break;
> > +      case "touchstart":
> > +        if (!BrowserUI.isStartTabVisible) {
> > +          this.dismiss();
> 
> Should we do this for the ToolPanelShown/AlertActive/etc. case above too?

Don't think so, notifications should come up on the start page, and if you hit ctrl-shift-j on the start tab, you want the context ui to hide.

Comment 47

[Jim Mathies [:jimm]]

Updating all of these to tip. Note there's a clobber on mc this morning but I was able to build after a pull and a touch obj/CLOBBER.

Comment 48

[Jim Mathies [:jimm]]

Attachment: part 1 - code reshuffle v.2 [sfoster: r+]

Comment 49

[Jim Mathies [:jimm]]

Attachment: part 2 - remove StartUI object v.2 [mbrubeck: r+]

Comment 50

[Jim Mathies [:jimm]]

Attachment: part 3 - fixup start tab components v.1

Comment 51

[Jim Mathies [:jimm]]

Attachment: part 4 - apzc fixes v.2 [bbondy: r+]

Comment 52

[Jim Mathies [:jimm]]

Attachment: part 5 - about:start v.1 [mbrubeck: r+]

Comment 53

[Jim Mathies [:jimm]]

Attachment: part 6 - view teardown v.1

Comment 54

[Jim Mathies [:jimm]]

Attachment: part 7 - tests v.1

Comment 55

[Jim Mathies [:jimm]]

Attachment: part 8 - misc. v.2 [mbrubeck: r+]

Comment 56

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #43)
> If we remove start-scrollbox from the layout the browser scroller would take
> over, not sure. I'll experiment around with it. I think I'd like to
> investigate this in a follow up though.

I spent a good chunk of time last night trying to get the browser scroller to work on Start.xul and never got it to work, so this seems reasonable.  Does it mean that we'll need a special case to get APZC to handle scrolling on Start.xul?

Comment 57

[Matt Brubeck (:mbrubeck)]

Comment on attachment 787481 [details] [diff] [review]
part 8 - misc. v.2 [mbrubeck: r+]

Review of attachment 787481 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/browser.js
@@ +522,5 @@
>        return;
>      }
>  
> +    if ((aOptions && "forceClose" in aOptions && aOptions.forceClose) ||
> +        tab.browser.currentURI.spec == kStartURI) {

I think you can revert this change if you keep Content.receiveMessage enabled.

Comment 58

[Jim Mathies [:jimm]]

Attachment: part 8 - misc. v.2 [mbrubeck: r+]

- removed the special case for close tab handling, with the Content patch removed this wasn't needed.

Comment 59

[Jim Mathies [:jimm]]

Attachment: part 9 - close / switch tab handling v.1

This addresses a couple issues:

1) when the user closes a start tab with pending actions, those actions weren't getting flushed and the app bar remained visible.

2) when the user switches to another tab from a start tab, or navigates to a site on a start tab, pending actions need to be flushed.

Comment 60

[Jim Mathies [:jimm]]

Attachment: part 9 - close / switch tab handling v.1

That needed to include bookmarks and history.

I should write some tests for these.

Comment 61

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #56)
> (In reply to Jim Mathies [:jimm] from comment #43)
> > If we remove start-scrollbox from the layout the browser scroller would take
> > over, not sure. I'll experiment around with it. I think I'd like to
> > investigate this in a follow up though.
> 
> I spent a good chunk of time last night trying to get the browser scroller
> to work on Start.xul and never got it to work, so this seems reasonable. 
> Does it mean that we'll need a special case to get APZC to handle scrolling
> on Start.xul?

Seems to kick in, so looks like it's working.

Comment 62

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 63

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 64

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 787468 [details] [diff] [review]
part 3 - fixup start tab components v.1

Review of attachment 787468 [details] [diff] [review]:
-----------------------------------------------------------------

I seem to have lost remote tabs, but I don't see anything here that would cause it, so this bit should be good to go.

::: browser/metro/base/content/startui/TopSitesView.js
@@ +15,5 @@
>    this._set.controller = this;
>    this._topSitesMax = aMaxSites;
>  
>    // clean up state when the appbar closes
> +  StartUI.chromeWin.addEventListener('MozAppbarDismissing', this, false);

hmm, all this should be common to all start-ui views. Not something that needs fixing in this bug though.

Comment 65

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 787475 [details] [diff] [review]
part 6 - view teardown v.1

Review of attachment 787475 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Looking over this, I think we may have more baggage to remove from bookmarks/history/topsites now that we removed the snapped-state views. But not in scope here.

::: browser/metro/base/content/startui/TopSitesView.js
@@ +42,5 @@
> +  destruct: function destruct() {
> +    Services.obs.removeObserver(this, "Metro:RefreshTopsiteThumbnail");
> +    Services.obs.removeObserver(this, "metro_viewstate_changed");
> +    PageThumbs.removeExpirationFilter(this);
> +    NewTabUtils.allPages.unregister(this);

Good catch. Makes me wonder what else was missed that wasn't necessary while startUI was persistent in chrome. Trying to spot stuff as I review but its tough.

Comment 66

[Jim Mathies [:jimm]]

in part 9 -

<sfoster> jimm, I got a exception running the testsuite: 
<sfoster>  2:34.54 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_topsites.js | uncaught exception - TypeError: aEvent.lastTab is undefined at chrome://browser/content/appbar.js:37
<jimm> oh oops
<jimm> URLChanged doesn't have a lastTab in its event
<jimm> if (this.activeTileset && aEvent.lastTab.browser.currentURI.spec == kStartURI) { ->
<jimm> if (this.activeTileset && aEvent.lastTab && aEvent.lastTab.browser.currentURI.spec == kStartURI) {

Comment 67

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 787476 [details] [diff] [review]
part 7 - tests v.1

Review of attachment 787476 [details] [diff] [review]:
-----------------------------------------------------------------

I got this exception running the tests (discussed in #windev and addressed in a different patch)
2:34.54 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_topsites.js | uncaught exception - TypeError: aEvent.lastTab is undefined at chrome://browser/content/appbar.js:37

r+ with comments.

::: browser/metro/base/tests/mochitest/browser_topsites.js
@@ +7,5 @@
>  
>  //////////////////////////////////////////////////////////////////////////
>  // Test helpers
>  
> +function getGrid() {

nit: Why not put these on the TopSitesTestHelper as getters?

::: browser/metro/base/tests/mochitest/browser_urlbar.js
@@ +106,5 @@
>    Browser.closeTab(Browser.selectedTab, { forceClose: true });
>  }
>  
>  gTests.push({
> +  desc: "load about blank",

Is this setUp for the tests that follow? I'd prefer we do this in the test() function kinda like we do in browser_tiles.js before calling runTests, rather than having a test-with-side-effect that has no assertions.

Comment 68

[Jim Mathies [:jimm]]

Attachment: part 9 - close / switch tab handling v.2

This is ok with that slight update for TabSelect. If you navigate off the start tab using a tile, the context ui is dismissed and the context actions are triggered. There's no need to do under in URLChanged. The only time we do is if you switch tabs away from the start tab.

Comment 69

[Jim Mathies [:jimm]]

(In reply to Sam Foster [:sfoster] from comment #67)
> Comment on attachment 787476 [details] [diff] [review]
> part 7 - tests v.1
> 
> Review of attachment 787476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I got this exception running the tests (discussed in #windev and addressed
> in a different patch)
> 2:34.54 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/
> browser_topsites.js | uncaught exception - TypeError: aEvent.lastTab is
> undefined at chrome://browser/content/appbar.js:37
> 
> r+ with comments.
> 
> ::: browser/metro/base/tests/mochitest/browser_topsites.js
> @@ +7,5 @@
> >  
> >  //////////////////////////////////////////////////////////////////////////
> >  // Test helpers
> >  
> > +function getGrid() {
> 
> nit: Why not put these on the TopSitesTestHelper as getters?
> 
> ::: browser/metro/base/tests/mochitest/browser_urlbar.js
> @@ +106,5 @@
> >    Browser.closeTab(Browser.selectedTab, { forceClose: true });
> >  }
> >  
> >  gTests.push({
> > +  desc: "load about blank",
> 
> Is this setUp for the tests that follow? I'd prefer we do this in the test()
> function kinda like we do in browser_tiles.js before calling runTests,
> rather than having a test-with-side-effect that has no assertions.

updated.

Comment 70

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 787753 [details] [diff] [review]
part 9 - close / switch tab handling v.2

Review of attachment 787753 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 71

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=3be13f6e770c

Note I had to land one follow up to address an issue with broadcasters. Apparently these don't propagate down from browser.xul into Start.xul. So I added both manually. Maybe there's a way to get these to cascade down, not sure. Posthumously requesting review on - 

https://hg.mozilla.org/integration/fx-team/rev/ccc623264e93

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/115691c86ac4
https://hg.mozilla.org/mozilla-central/rev/27b0999a84b8
https://hg.mozilla.org/mozilla-central/rev/14f89cd51f67
https://hg.mozilla.org/mozilla-central/rev/c30cf5adb026
https://hg.mozilla.org/mozilla-central/rev/244c1ccec518
https://hg.mozilla.org/mozilla-central/rev/d3940269167b
https://hg.mozilla.org/mozilla-central/rev/8428461eabad
https://hg.mozilla.org/mozilla-central/rev/e92436526844
https://hg.mozilla.org/mozilla-central/rev/e5ff1e3e8e4a
https://hg.mozilla.org/mozilla-central/rev/ccc623264e93