Closed Bug 851130 Opened 7 years ago Closed 6 years ago

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

Categories

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

All
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: mbrubeck, Assigned: jimm)

References

Details

(Whiteboard: [Metro Preview] feature=work)

Attachments

(9 files, 30 obsolete files)

69.87 KB, patch
jimm
: review+
Details | Diff | Splinter Review
14.52 KB, patch
jimm
: review+
Details | Diff | Splinter Review
14.16 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
4.90 KB, patch
jimm
: review+
Details | Diff | Splinter Review
2.69 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.90 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
25.21 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
1.04 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.97 KB, patch
sfoster
: review+
Details | Diff | Splinter Review
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.)
Attached patch WIP (obsolete) — Splinter Review
This is a totally non-working snapshot of my current work, just in case anyone is curious.
Attached patch WIP 2 (obsolete) — Splinter Review
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.
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".
Assignee: mbrubeck → nobody
Assignee: nobody → 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).
Assignee: jwilde → nobody
Whiteboard: feature=work → feature=work [preview-triage]
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.
Flags: needinfo?(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.
Flags: needinfo?(mbrubeck)
Assignee: nobody → jmathies
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.
Blocks: 876042
Whiteboard: feature=work [preview-triage] → feature=work [preview]
Whiteboard: feature=work [preview] → [Metro Preview] feature=work
Attached patch shuffling things around v.1 (obsolete) — Splinter Review
Attachment #727897 - Attachment is obsolete: true
Attachment #749927 - Attachment is obsolete: true
Attached patch basics v.1 (obsolete) — Splinter Review
Attached patch apzc v.1 (obsolete) — Splinter Review
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).
Attachment #786314 - Flags: feedback?(netzen)
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 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.
Attachment #786314 - Flags: feedback?(netzen) → feedback+
Attached patch rollup v.1 (obsolete) — Splinter Review
Attached patch part 1 - code reshuffle v.1 (obsolete) — Splinter Review
Attachment #786302 - Attachment is obsolete: true
Attachment #786303 - Attachment is obsolete: true
Attachment #786314 - Attachment is obsolete: true
Attachment #786378 - Attachment is obsolete: true
Attachment #786410 - Attachment is obsolete: true
Attached patch part 4 - apzc fixes v.1 (obsolete) — Splinter Review
Attached patch part 5 - about:start v.1 (obsolete) — Splinter Review
Attached patch part 6 - tests (obsolete) — Splinter Review
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.
Attached patch rollup (obsolete) — Splinter Review
Attached patch rollup (obsolete) — Splinter Review
Attachment #786930 - Attachment is obsolete: true
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.
Attached patch part 6 - tests v.1 (obsolete) — Splinter Review
Attachment #786927 - Attachment is obsolete: true
Attachment #786936 - Attachment is obsolete: true
Attached patch rollup (obsolete) — Splinter Review
Attached patch part 7 - misc. (obsolete) — Splinter Review
fixes:
- selection bug on grid items
- close tab button
Attachment #787009 - Attachment is obsolete: true
Attachment #787011 - Attachment is obsolete: true
Attachment #787068 - Attachment is obsolete: true
Attached patch part 6 - tests (obsolete) — Splinter Review
Attached patch part 7 - misc. (obsolete) — Splinter Review
Attachment #786924 - Flags: review?(netzen)
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
Attachment #786924 - Flags: review?(netzen)
Attached patch part 7 - misc. (obsolete) — Splinter Review
added a couple fixes for the two remaining test failures on try.
Attachment #787070 - Attachment is obsolete: true
Attachment #786921 - Flags: review?(sfoster)
Attachment #786922 - Flags: review?(mbrubeck)
Attachment #786923 - Flags: review?(sfoster)
Attachment #786926 - Flags: review?(mbrubeck)
Attached patch part 6 - tests (obsolete) — Splinter Review
Attachment #787069 - Attachment is obsolete: true
Attachment #787109 - Attachment is obsolete: true
Attachment #787113 - Flags: review?(sfoster)
Attached patch part 7 - content (obsolete) — Splinter Review
this hides about:start from our frame scripts.
Attachment #787114 - Flags: review?(mbrubeck)
Attached patch part 8 - misc. (obsolete) — Splinter Review
Attachment #787116 - Flags: review?(mbrubeck)
Attached patch rollup (obsolete) — Splinter Review
Attachment #787118 - Attachment is patch: true
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 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.
Attachment #786922 - Flags: review?(mbrubeck) → review+
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
Attachment #786921 - Flags: review?(sfoster) → review+
Attachment #786926 - Flags: review?(mbrubeck) → review+
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.
Attachment #787114 - Flags: review?(mbrubeck) → review-
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.
Attachment #787116 - Flags: review?(mbrubeck) → review+
(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.
*I think* the browser scroller would take over, not sure.
(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.
Attachment #787114 - Attachment is obsolete: true
(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.
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.
Attachment #786921 - Attachment is obsolete: true
Attachment #787464 - Flags: review+
Attachment #787464 - Attachment description: part 1 - code reshuffle v.2 → part 1 - code reshuffle v.2 r+ sfoster
Attachment #787464 - Attachment description: part 1 - code reshuffle v.2 r+ sfoster → part 1 - code reshuffle v.2 [sfoster: r+]
Attachment #786922 - Attachment is obsolete: true
Attachment #787467 - Flags: review+
Attachment #786923 - Attachment is obsolete: true
Attachment #786923 - Flags: review?(sfoster)
Attachment #787468 - Flags: review?(sfoster)
Attachment #786924 - Attachment is obsolete: true
Attachment #787470 - Flags: review+
Attachment #786926 - Attachment is obsolete: true
Attachment #787471 - Flags: review+
Attachment #787475 - Flags: review?(sfoster)
Attachment #787113 - Attachment is obsolete: true
Attachment #787113 - Flags: review?(sfoster)
Attachment #787476 - Flags: review?(sfoster)
Attachment #787116 - Attachment is obsolete: true
Attachment #787118 - Attachment is obsolete: true
Attachment #787481 - Flags: review+
(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 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.
- removed the special case for close tab handling, with the Content patch removed this wasn't needed.
Attachment #787481 - Attachment is obsolete: true
Attachment #787521 - Flags: review+
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.
Attachment #787523 - Flags: review?(sfoster)
That needed to include bookmarks and history.

I should write some tests for these.
Attachment #787523 - Attachment is obsolete: true
Attachment #787523 - Flags: review?(sfoster)
Attachment #787535 - Flags: review?(sfoster)
(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.
Attached patch rollup (obsolete) — Splinter Review
Attached patch rollup (obsolete) — Splinter Review
Attachment #787545 - Attachment is obsolete: true
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.
Attachment #787468 - Flags: review?(sfoster) → review+
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.
Attachment #787475 - Flags: review?(sfoster) → review+
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) {
Attachment #787535 - Flags: review?(sfoster)
Depends on: 902713
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.
Attachment #787476 - Flags: review?(sfoster) → review+
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.
Attachment #787535 - Attachment is obsolete: true
Attachment #787547 - Attachment is obsolete: true
Attachment #787753 - Flags: review?(sfoster)
(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 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.
Attachment #787753 - Flags: review?(sfoster) → review+
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
Flags: needinfo?(mbrubeck)
Flags: needinfo?(mbrubeck)
Depends on: 875924
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.