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

RESOLVED FIXED in Firefox 26

Status

Firefox for Metro
Firefox Start
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mbrubeck, Assigned: jimm)

Tracking

Trunk
Firefox 26
All
Windows 8.1
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Metro Preview] feature=work)

Attachments

(9 attachments, 30 obsolete attachments)

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
(Reporter)

Description

5 years ago
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.)
(Reporter)

Comment 1

5 years ago
Created attachment 727897 [details] [diff] [review]
WIP

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

Comment 2

5 years ago
Created attachment 749927 [details] [diff] [review]
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.
(Reporter)

Comment 3

5 years ago
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]
(Assignee)

Comment 5

5 years ago
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)
(Reporter)

Comment 6

5 years ago
(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)

Updated

5 years ago
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
(Assignee)

Updated

5 years ago
Whiteboard: feature=work [preview-triage] → feature=work [preview]

Updated

5 years ago
Whiteboard: feature=work [preview] → [Metro Preview] feature=work
(Assignee)

Comment 8

5 years ago
Created attachment 786302 [details] [diff] [review]
shuffling things around v.1
Attachment #727897 - Attachment is obsolete: true
Attachment #749927 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 786303 [details] [diff] [review]
basics v.1
(Assignee)

Comment 10

5 years ago
Created attachment 786314 [details] [diff] [review]
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).
Attachment #786314 - Flags: feedback?(netzen)
(Assignee)

Comment 11

5 years ago
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+
(Assignee)

Comment 13

5 years ago
Created attachment 786378 [details] [diff] [review]
get about:start working right v.1
(Assignee)

Comment 14

5 years ago
Created attachment 786410 [details] [diff] [review]
rollup v.1
(Assignee)

Comment 15

5 years ago
Created attachment 786921 [details] [diff] [review]
part 1 - code reshuffle v.1
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
(Assignee)

Comment 16

5 years ago
Created attachment 786922 [details] [diff] [review]
part 2 - remove StartUI object v.1
(Assignee)

Comment 17

5 years ago
Created attachment 786923 [details] [diff] [review]
part 3 - fixup start tab components v.1
(Assignee)

Comment 18

5 years ago
Created attachment 786924 [details] [diff] [review]
part 4 - apzc fixes v.1
(Assignee)

Comment 19

5 years ago
Created attachment 786926 [details] [diff] [review]
part 5 - about:start v.1
(Assignee)

Comment 20

5 years ago
Created attachment 786927 [details] [diff] [review]
part 6 - tests
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Comment 22

5 years ago
Created attachment 786930 [details] [diff] [review]
rollup
(Assignee)

Comment 23

5 years ago
Created attachment 786936 [details] [diff] [review]
rollup
Attachment #786930 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
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.
(Assignee)

Comment 25

5 years ago
Created attachment 787009 [details] [diff] [review]
part 6 - tests v.1
Attachment #786927 - Attachment is obsolete: true
Attachment #786936 - Attachment is obsolete: true
(Assignee)

Comment 26

5 years ago
Created attachment 787011 [details] [diff] [review]
rollup
(Assignee)

Comment 28

5 years ago
Created attachment 787068 [details] [diff] [review]
part 7 - misc.

fixes:
- selection bug on grid items
- close tab button
Attachment #787009 - Attachment is obsolete: true
Attachment #787011 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #787068 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Created attachment 787069 [details] [diff] [review]
part 6 - tests
(Assignee)

Comment 30

5 years ago
Created attachment 787070 [details] [diff] [review]
part 7 - misc.
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 32

5 years ago
Created attachment 787109 [details] [diff] [review]
part 7 - misc.

added a couple fixes for the two remaining test failures on try.
Attachment #787070 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #786921 - Flags: review?(sfoster)
(Assignee)

Updated

5 years ago
Attachment #786922 - Flags: review?(mbrubeck)
(Assignee)

Updated

5 years ago
Attachment #786923 - Flags: review?(sfoster)
(Assignee)

Updated

5 years ago
Attachment #786926 - Flags: review?(mbrubeck)
(Assignee)

Comment 33

5 years ago
Created attachment 787113 [details] [diff] [review]
part 6 - tests
Attachment #787069 - Attachment is obsolete: true
Attachment #787109 - Attachment is obsolete: true
Attachment #787113 - Flags: review?(sfoster)
(Assignee)

Comment 34

5 years ago
Created attachment 787114 [details] [diff] [review]
part 7 - content

this hides about:start from our frame scripts.
Attachment #787114 - Flags: review?(mbrubeck)
(Assignee)

Comment 35

5 years ago
Created attachment 787116 [details] [diff] [review]
part 8 - misc.
Attachment #787116 - Flags: review?(mbrubeck)
(Assignee)

Comment 36

5 years ago
Created attachment 787118 [details] [diff] [review]
rollup
(Assignee)

Updated

5 years ago
Attachment #787118 - Attachment is patch: true
(Assignee)

Comment 38

5 years ago
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.
(Reporter)

Comment 39

5 years ago
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+
(Reporter)

Updated

5 years ago
Attachment #786926 - Flags: review?(mbrubeck) → review+
(Reporter)

Comment 41

5 years ago
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-
(Reporter)

Comment 42

5 years ago
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+
(Assignee)

Comment 43

5 years ago
(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.
(Assignee)

Comment 44

5 years ago
*I think* the browser scroller would take over, not sure.
(Assignee)

Comment 45

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #787114 - Attachment is obsolete: true
(Assignee)

Comment 46

5 years ago
(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.
(Assignee)

Comment 47

5 years ago
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.
(Assignee)

Comment 48

5 years ago
Created attachment 787464 [details] [diff] [review]
part 1 - code reshuffle v.2 [sfoster: r+]
Attachment #786921 - Attachment is obsolete: true
Attachment #787464 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #787464 - Attachment description: part 1 - code reshuffle v.2 → part 1 - code reshuffle v.2 r+ sfoster
(Assignee)

Updated

5 years ago
Attachment #787464 - Attachment description: part 1 - code reshuffle v.2 r+ sfoster → part 1 - code reshuffle v.2 [sfoster: r+]
(Assignee)

Comment 49

5 years ago
Created attachment 787467 [details] [diff] [review]
part 2 - remove StartUI object v.2 [mbrubeck: r+]
Attachment #786922 - Attachment is obsolete: true
Attachment #787467 - Flags: review+
(Assignee)

Comment 50

5 years ago
Created attachment 787468 [details] [diff] [review]
part 3 - fixup start tab components v.1
Attachment #786923 - Attachment is obsolete: true
Attachment #786923 - Flags: review?(sfoster)
Attachment #787468 - Flags: review?(sfoster)
(Assignee)

Comment 51

5 years ago
Created attachment 787470 [details] [diff] [review]
part 4 - apzc fixes v.2 [bbondy: r+]
Attachment #786924 - Attachment is obsolete: true
Attachment #787470 - Flags: review+
(Assignee)

Comment 52

5 years ago
Created attachment 787471 [details] [diff] [review]
part 5 - about:start v.1 [mbrubeck: r+]
Attachment #786926 - Attachment is obsolete: true
Attachment #787471 - Flags: review+
(Assignee)

Comment 53

5 years ago
Created attachment 787475 [details] [diff] [review]
part 6 - view teardown v.1
Attachment #787475 - Flags: review?(sfoster)
(Assignee)

Comment 54

5 years ago
Created attachment 787476 [details] [diff] [review]
part 7 - tests v.1
Attachment #787113 - Attachment is obsolete: true
Attachment #787113 - Flags: review?(sfoster)
Attachment #787476 - Flags: review?(sfoster)
(Assignee)

Comment 55

5 years ago
Created attachment 787481 [details] [diff] [review]
part 8 - misc. v.2 [mbrubeck: r+]
Attachment #787116 - Attachment is obsolete: true
Attachment #787118 - Attachment is obsolete: true
Attachment #787481 - Flags: review+
(Reporter)

Comment 56

5 years ago
(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?
(Reporter)

Comment 57

5 years ago
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.
(Assignee)

Comment 58

5 years ago
Created attachment 787521 [details] [diff] [review]
part 8 - misc. v.2 [mbrubeck: r+]

- removed the special case for close tab handling, with the Content patch removed this wasn't needed.
Attachment #787481 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #787521 - Flags: review+
(Assignee)

Comment 59

5 years ago
Created attachment 787523 [details] [diff] [review]
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.
Attachment #787523 - Flags: review?(sfoster)
(Assignee)

Comment 60

5 years ago
Created attachment 787535 [details] [diff] [review]
part 9 - close / switch tab handling v.1

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)
(Assignee)

Comment 61

5 years ago
(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.
(Assignee)

Comment 62

5 years ago
Created attachment 787545 [details] [diff] [review]
rollup
(Assignee)

Comment 63

5 years ago
Created attachment 787547 [details] [diff] [review]
rollup
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+
(Assignee)

Comment 66

5 years ago
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) {
(Assignee)

Updated

5 years ago
Attachment #787535 - Flags: review?(sfoster)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 68

5 years ago
Created attachment 787753 [details] [diff] [review]
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.
Attachment #787535 - Attachment is obsolete: true
Attachment #787547 - Attachment is obsolete: true
Attachment #787753 - Flags: review?(sfoster)
(Assignee)

Comment 69

5 years ago
(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+
(Assignee)

Comment 71

5 years ago
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)
(Assignee)

Updated

5 years ago
Flags: needinfo?(mbrubeck)
(Reporter)

Updated

4 years ago
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.