1219725 - Add a button for session restore to the tab bar

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P3)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Restore Button.png

Session restore is a compelling feature for a large audience, but it is very hard to discover at the moment.

We can add a button in the tab bar when the user starts a new session that will restore the previous session. A spec is attached.

Behavior:
- The button only shows up if automatic session restore is off
- The button fades out once the user opens a new tab
- If the window is scaled down so that the button wouldn't fit anymore, it disappears temporarily (re-appears once the window is big enough again)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Cool.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Good idea.

Comment 3

[:Gijs (he/him)]

As a bonus, can we detect people doing this repeatedly and offer UI (notification bar? Something else?) that allows people to change the startup preference to always restore their session?

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #3)
> As a bonus, can we detect people doing this repeatedly and offer UI
> (notification bar? Something else?) that allows people to change the startup
> preference to always restore their session?

<3
Let's say, if you restore your third consecutive session, you immediately get a notification bar:
Do you want Firefox to automatically restore your tabs in the future? [Sure!] [No Thanks]

Comment 5

[Justin Dolske [:Dolske]]

I very much agree that Session Restore is a compelling feature that we should figure out how to expose/promote more, but I'm not sure jamming another button into the toolbar is the right approach. (I wouldn't really object to having a button that's just in the customization palette by default, but that doesn't really help the discoverability issue.)

A few alternative ideas:

* Just flip the default to "Restore my tabs" for new profiles. (But doesn't help existing users, and I'm not sure if we should worry about users being surprised, in a bad way, by having their tabs reopened.)

* Restore tabs by default, but show a notification bar (?) saying "Hey, we restored your stuff, if you don't want this [Click here] to turn it off."

* Don't (actually) restore by default, but show a notification bar the first N runs offering to do so. [This could be interesting for promoting the feature to _existing_ Firefox users in a gentile way.]

The last two have the downside of Yet Another Notification Bar that new users are exposed to.

Comment 6

[Linus Rorstad]

I have been waiting a long time for an easier way to restore last session but I would rather have a button on the newtab page. A button on the tab bar would also work but as I use it only occasionally it would be good to also see what sites were in the last session.

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Justin Dolske [:Dolske] from comment #5)
> I very much agree that Session Restore is a compelling feature that we
> should figure out how to expose/promote more, but I'm not sure jamming
> another button into the toolbar is the right approach. (I wouldn't really
> object to having a button that's just in the customization palette by
> default, but that doesn't really help the discoverability issue.)

I don't think that will be an issue since the button is pretty transient. By definition, it will only show up when the user has just one tab (or perhaps two when there's a first run page). We can make it even more transient by making it disappear once an additional tab is opened, if that's the issue.

If we notice that that button gets really high usage, flipping the pref to make session restore the default is a possible next step, but it will help us determine the usefulness of session restore to the broader public.

Comment 8

[Chris Peterson [:cpeterson]]

What is the harm in enabling session restore by default (for users that still have the default "Show my home page" value)? Users wouldn't lose their tabs. The disadvantages are a possible startup time regression and "eternal session" cookie bug 530594.

Comment 9

[Erica Wright [:ewright]]

Attachment: Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review commit: https://reviewboard.mozilla.org/r/60284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60284/

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

https://reviewboard.mozilla.org/r/60284/#review57470

Thanks for the patch, just some notes here that should help you on your second round. Sorry for the delay in review, please request review from :dao or :gijs on your next version as I will be on PTO next week.

This patch doesn't apply at all locally for me, and the patch doesn't specify the parent revision so I'm not sure what mozilla-central revision you're working off of.

Please remove the Restore Session button from about:home in this patch. We shouldn't have two places in our UI that do identical things.

We should have automated tests with this.

::: browser/base/content/browser.css:154
(Diff revision 1)
> +#restore-tabs-button-wrapper {
> +  padding: 4px;
> +  margin-left: 3px;
> +}
> +
> +#restore-tabs-button > .toolbarbutton-multiline-text{
> +  display: -moz-box;
> +  font-size: .8em;
> +  margin-bottom: 0;
> +}
> +
> +#restore-tabs-button {
> +  border: 1px solid var(--urlbar-separator-color);
> +  border-radius: 2px;
> +  display: none;
> +  transition: opacity 1s ease-out;
> +}
> +
> +#restore-tabs-button.hidden {
> +  opacity: 0;
> +}

These rules should all be in theme CSS, not in /browser/base/content/.

A 1 second transition is too long. As developers we often tend to make transitions too long because we like seeing our work and inherently want others to see our work. Please lower this to .3 seconds.

Please don't use 'hidden' as a className, as it is often used as a property which indirectly sets display:none. After the item has opacity:0; should it become display:none;? As it is now, elements with opacity:0; still consume space. You can see this by making the Firefox window very narrow.

::: browser/base/content/browser.js:7480
(Diff revision 1)
> +      this.showButton();
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> -
> +  showButton: function () {

As this code is only called from the init function, I would prefer if it remained inside of init.

::: browser/base/content/browser.js:7490
(Diff revision 1)
> +    function fadeRestoreTabsButton() {
> +      gBrowser.tabContainer.removeEventListener("TabOpen", fadeRestoreTabsButton, false);
> +      restore_tabs_button.addEventListener("transitionend", removeRestoreTabsButton, false);
> +      restore_tabs_button.classList.add("hidden");
> +    }
> +    let restore_tabs_button = document.querySelector("#restore-tabs-button");

Please use document.getElementById here.

::: browser/base/content/browser.js:7491
(Diff revision 1)
> +      gBrowser.tabContainer.removeEventListener("TabOpen", fadeRestoreTabsButton, false);
> +      restore_tabs_button.addEventListener("transitionend", removeRestoreTabsButton, false);
> +      restore_tabs_button.classList.add("hidden");
> +    }
> +    let restore_tabs_button = document.querySelector("#restore-tabs-button");
> +    restore_tabs_button.style.display = "-moz-box";

The button should have hidden="true" in the markup, and here you can call
`restore_tabs_button.hidden = false`

::: browser/base/content/tabbrowser.xml:4909
(Diff revision 1)
> +          <xul:toolbarbutton id="restore-tabs-button"
> +                             anonid="restore-tabs-button"
> +                             class="chromeclass-toolbar-additional"
> +                             onclick="restorePreviousTabs();"/>

The label for the button should be defined in the markup here, and thus the string would need to be defined in a DTD file.

Also, please double-check that you intend to use .toolbarbutton-multiline-text here. It seems to me that it would be preferable to use .toobarbutton-text since it will probably "break" our tabbar if the text starts to wrap.

::: browser/base/content/tabbrowser.xml:4930
(Diff revision 1)
>            var tab = this.firstChild;
>            tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle");
>            tab.setAttribute("crop", "end");
>            tab.setAttribute("onerror", "this.removeAttribute('image');");
>  
> +          var restore_tabs_button = this.querySelector("#restore-tabs-button");

nit, please use document.getElementById

::: browser/locales/en-US/chrome/browser/tabbrowser.properties:50
(Diff revision 1)
>  
>  # LOCALIZATION NOTE (tabs.allowTabFocusByPromptForSite):
>  # %S is the hostname of the site where dialogs are allowed to switch tabs
>  tabs.allowTabFocusByPromptForSite=Allow dialogs from %S to take you to their tab
> +
> +tabs.restoreTabs=Restore tabs from last time

Are we able to reuse the string from about:home?

Comment 11

[Erica Wright [:ewright]]

Do you have any advice on where/how to add tests for this? :bwinton is not much help ;p

Comment 12

[:Gijs (he/him)]

(In reply to Erica from comment #11)
> Do you have any advice on where/how to add tests for this? :bwinton is not
> much help ;p

You'd want to add a browser-chrome mochitest ( https://developer.mozilla.org/en-US/docs/Browser_chrome_tests ) in a subdir of:

browser/base/content/tests/

The right thing might be to create a tabbrowser directory there and put it in there. You'll then also need to create a 'browser.ini' file in there to list the new dir, and then add the browser.ini file in the browser/base/moz.build file's list:

http://searchfox.org/mozilla-central/source/browser/base/moz.build#17-28

We really need to get away from the dumping ground that the 'general' subdir in there has become (I'm pretty sure there's a fairly large number of bugs in there that we could then move to the newly-created directory in a separate bug).

Alternatively, because the bug is at least vaguely related to sessionstore, you could also add it in browser/components/sessionstore/test/ .

Please name the file browser_tabbar_sessionrestore_button.js or something like that, rather than using the browser_bug1219725.js pattern which we're trying to slowly move away from.

For a hopefully vaguely useful instance that uses some of the helpers you might want/need, see e.g. http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_newtab_userTypedValue.js .

Let me know if you need more help than that!

Comment 13

[Erica Wright [:ewright]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/1-2/

Comment 14

[Erica Wright [:ewright]]

https://reviewboard.mozilla.org/r/60284/#review57470

I wrote some tests, they don't cover all the things I would like to test as I struggled a lot in them (any suggestions appreciated). Shorlander has a different opinion on removing the Restore Session button as the one in the tab bar is temporary - have left it in for now till others make the decision.

Comment 15

[Erica Wright [:ewright]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/2-3/

Comment 16

[Erica Wright [:ewright]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/3-4/

Comment 17

[Stephen Horlander [:shorlander]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Looks good, I noticed a few problems:

- If you only restore one tab the button doesn't disappear
- We probably shouldn't offer to restore a session that would be the same as a default session
- It looks like a button but doesn't pick-up styling form any of our existing buttons
  -- Uses sentence case, buttons should use title case
  -- The square shape of the button looks a little bit odd next to the curved tab, so while I am not super excited about adding another one-off styling, I think we should make it rounded and a little subtle (see mockup)
- Not sure if it's due to this patch, but I am missing the new tab button?

Comment 18

[Stephen Horlander [:shorlander]]

Attachment: Restore Button Styling - 01

Comment 19

[Stephen Horlander [:shorlander]]

(In reply to Chris Peterson [:cpeterson] from comment #8)
> What is the harm in enabling session restore by default (for users that
> still have the default "Show my home page" value)? Users wouldn't lose their
> tabs. The disadvantages are a possible startup time regression and "eternal
> session" cookie bug 530594.

I think we should do this. I believe we turned this off by default a long time ago when we still tried to load all tabs at once. Is there a reason we can't do this?

Comment 20

[Erica Wright [:ewright]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/4-5/

Comment 21

[:Gijs (he/him)]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

https://reviewboard.mozilla.org/r/60284/#review58560

This is already a pretty nice patch. I've added feedback below to suggest tweaks to various things. However, I think it would be wise to await a decision about whether or not we can enable session restore by default before acting on the feedback. It would be a shame if you spend even more time and folks then decide to just do something else instead. :-(

::: browser/base/content/browser.css:137
(Diff revision 5)
>  }
>  
>  #tabbrowser-tabs:not([overflow="true"]) ~ #alltabs-button,
>  #tabbrowser-tabs:not([overflow="true"]) + #new-tab-button,
>  #tabbrowser-tabs[overflow="true"] > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
> -#TabsToolbar[currentset]:not([currentset*="tabbrowser-tabs,new-tab-button"]) > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
> +#TabsToolbar[currentset]:not([currentset*="tabbrowser-tabs"]):not([currentset*="alltabs-button"]) > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,

Can you elaborate on what this change accomplishes? I'm a bit confused as to how it relates to what you're doing here.

::: browser/base/content/browser.js:7493
(Diff revision 5)
> +      let restore_tabs_button_wrapper = document.getElementById("restore-tabs-button-wrapper");
> +      let restore_tabs_button = document.getElementById("restore-tabs-button");

Nit: please use camelCase, and see other notes about how to get these elements.

::: browser/base/content/browser.js
(Diff revision 5)
> +      gBrowser.tabContainer.addEventListener("TabOpen", fadeRestoreTabsButton, false);
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> -

Nit: leave the blank line alone. :-)

::: browser/base/content/tabbrowser.xml:4934
(Diff revision 5)
> +        <xul:hbox id="restore-tabs-button-wrapper"
> +                  hidden="true">
> +          <xul:toolbarbutton id="restore-tabs-button"

We shouldn't have elements with an id inside XBL bindings. You can find elements inside the content from the XBL binding by giving them an anonid attribute and using:

```
document.getAnonymousElementByAttribute(referenceToContainerElement, "anonid", "myanonidvalue");
```

Note that in this case, for the `referenceToContainerElement` you could likely use the `tabsContainer` property of the tabbrowser binding, or perhaps `this` if you're inside the tabbrowser-tabs binding.

You will also see that the binding has some <field>s declared that point to its anonymous content. It seems like it might be useful to create one for this button as well, to make it easier for outside code to modify its classes, as the session restore code is doing.

::: browser/base/content/tabbrowser.xml:4938
(Diff revision 5)
> +
> +        <xul:hbox id="restore-tabs-button-wrapper"
> +                  hidden="true">
> +          <xul:toolbarbutton id="restore-tabs-button"
> +                             anonid="restore-tabs-button"
> +                             class="chromeclass-toolbar-additional"

Do we need this class? I don't think it's quite right...

::: browser/base/content/tabbrowser.xml:4939
(Diff revision 5)
> +        <xul:hbox id="restore-tabs-button-wrapper"
> +                  hidden="true">
> +          <xul:toolbarbutton id="restore-tabs-button"
> +                             anonid="restore-tabs-button"
> +                             class="chromeclass-toolbar-additional"
> +                             onclick="restorePreviousTabs();"

I think you can just call `restoreLastSession()`, and you don't need to add a method on the other binding. :-)

::: browser/base/content/tabbrowser.xml:5005
(Diff revision 5)
>          </getter>
>        </property>
>  
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;

Unfortunately this particular line has several problems. Our toolbars are customizable, so the first child of the tabstoolbar isn't necessarily the tabstrip. If you want the tabstrip, you can actually just refer to `this` in this method, as it lives on the XBL binding for the tabstrip.

::: browser/base/content/tabbrowser.xml:5006
(Diff revision 5)
>        </property>
>  
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;
> +          var should_hide = true;

Nit: use camelCase.

Also, naming-wise, I'm a bit confused that we have a method called "checkTabStripLength" and it returns "shouldHide", which isn't a natural answer to the question. Maybe rename the method to something that reflects what question it's trying to answer?

::: browser/base/content/tabbrowser.xml:5007
(Diff revision 5)
>  
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;
> +          var should_hide = true;
> +          let wrapper = document.getElementById("restore-tabs-button-wrapper");

As noted elsewhere, you want to be using getAnonymousElementByAttribute. Sorry for the double feedback compared to what Jared said earlier. :-(

::: browser/base/content/tabbrowser.xml:5008
(Diff revision 5)
> +      <method name="checkTabStripLength">
> +        <body><![CDATA[
> +          var node = document.getElementById("TabsToolbar").firstChild;
> +          var should_hide = true;
> +          let wrapper = document.getElementById("restore-tabs-button-wrapper");
> +          if (wrapper.classList.contains('session-exists') && this.boxObject.width > 500 ) {

The 500 here (and the 600 below) is a bit of a code smell. What does the magic number mean, and why is it here? :-)

Nit: feels like you could have an outer 'if' on `wrapper.classList.contains('session-exists')` and an inner if...else for the other contents here...

Nit: when storing state, we tend to use an attribute rather than a class. So you could set the "session-exists" attribute to "true" instead.

::: browser/base/content/tabbrowser.xml:5011
(Diff revision 5)
> +            //if the search bar is in the toolbar, show the restore tabs button
> +            //even though it encroaches on the tab

I'm a little confused by this comment. You're mentioning the search bar, but you're not directly referring to it, as far as I can tell? Can you explain what's going on here?

::: browser/base/content/tabbrowser.xml:5013
(Diff revision 5)
> +          if (wrapper.classList.contains('session-exists') && this.boxObject.width > 500 ) {
> +            should_hide = false;
> +
> +            //if the search bar is in the toolbar, show the restore tabs button
> +            //even though it encroaches on the tab
> +          } else if (wrapper.classList.contains("session-exists") && this.parentNode.boxObject.width > 600) {

The 600 here is a bit of a code smell. What does the magic number mean, and why is it here? :-)

::: browser/base/content/tabbrowser.xml:5018
(Diff revision 5)
> +          } else if (wrapper.classList.contains("session-exists") && this.parentNode.boxObject.width > 600) {
> +            while (node && node.nodeType === 1) {
> +              if (node.flex > 1) {
> +                should_hide = false;
> +              }
> +              node = node.nextElementSibling || node.nextSibling;

Nit: because you're also checking node.nodeType, there are two ways this can pan out:

1) we get a nextElementSibling and we go through the loop once more
2) we don't get a nextElementSibling. We assign node.nextSibling instead, but even if there were a non-node sibling, the nodeType check in the loop condition will fail and we break out of the loop anyway.

IOW, I don't think you need the `|| node.nextSibling` bit. I'm also not sure there are actual non-element nodes in the tabstrip at any point, tbh...

::: browser/base/content/tabbrowser.xml:5458
(Diff revision 5)
>                if (width != this.mTabstripWidth) {
>                  this.adjustTabstrip();
>                  this._fillTrailingGap();
>                  this._handleTabSelect();
>                  this.mTabstripWidth = width;
> +                document.getElementById('restore-tabs-button-wrapper').hidden = this.checkTabStripLength();

All the callers to this method do this. Feels like the method should just be called something like "updateSessionRestoreVisibility()" and set .hidden itself, instead of burdening the consumers to all do exactly the same thing.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

We don't normally license test files anymore, or if we do, we use public domain.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:11
(Diff revision 5)
> +  let win = yield BrowserTestUtils.openNewBrowserWindow();
> +  let restore_tabs_button_wrapper = win.document.getElementById('restore-tabs-button-wrapper');
> +  let new_tab_button = win.document.getAnonymousElementByAttribute(win.gBrowser.tabContainer, 'class', 'tabs-newtab-button');
> +  is(new_tab_button.style.visibility !== 'collapse', true, 'new tab button is not visibility: collapse');
> +
> +  is(restore_tabs_button_wrapper.hidden, false, 'restore_tabs_button_wrapper shows initially');

I'm confused by this... do we show the button even when there's no session to restore?

Note also that you're implicitly relying on the window being > 500 or 600 pixels or whatever it is... we should probably not do that because if the machine on which the test runs doesn't have windows that large, this won't be the case.

::: browser/modules/BrowserUITelemetry.jsm:68
(Diff revision 5)
>      ],
>      "TabsToolbar": [
>        "tabbrowser-tabs",
>        "new-tab-button",
>        "alltabs-button",
> +      "restore-tabs-button",

I'm a bit confused. The button isn't in the placements at all (it isn't a customizable button, and it's not in the #TabsToolbar but inside #tabbrowser-tabs). Why did you need to add this?

::: browser/themes/shared/toolbarbuttons.inc.css:103
(Diff revision 5)
>  
>  #print-button[cui-areatype="toolbar"] {
>    -moz-image-region: rect(0, 414px, 18px, 396px);
>  }
>  
> +#restore-tabs-button > .toolbarbutton-icon {

Can you put all the CSS you added here at the end of tabs.inc.css instead? toolbarbuttons.inc.css is only used to pick icons for toolbarbuttons, and this code does other things. :-)

::: browser/themes/shared/toolbarbuttons.inc.css:115
(Diff revision 5)
> +  transition: opacity 300ms ease-out;
> +}
> +
> +#restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;
> +  font-size: .8em;

AFAICT this will be evaluated against font: message-box and so the font-size will be different depending on OS settings. Are we sure this will be readable and/or not too big? This might be a question for shorlander...

::: browser/themes/shared/toolbarbuttons.inc.css:121
(Diff revision 5)
> +  margin-bottom: 0;
> +  padding: 0 5px;
> +}
> +
> +#restore-tabs-button {
> +  border: 1px solid var(--urlbar-separator-color);

We shouldn't use urlbar-separator-color outside of the urlbar. I'm not sure what else to use - tabs separators use `currentColor`, but that also relies on a very low opacity, which would hide the text here. Can you see if there's something else we can rely on, or alternatively, pick 2 rgba colors, one for the 'normal' state and one for the inverted (ie dark high contrast or lightweight themes) ?

Comment 22

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/60284/#review58560

> The 600 here is a bit of a code smell. What does the magic number mean, and why is it here? :-)

Whoops, forgot to remove this duplicate.

Comment 23

[Justin Dolske [:Dolske]]

(In reply to Stephen Horlander [:shorlander] from comment #19)
> (In reply to Chris Peterson [:cpeterson] from comment #8)
> > What is the harm in enabling session restore by default (for users that
> > still have the default "Show my home page" value)? Users wouldn't lose their
> > tabs. The disadvantages are a possible startup time regression and "eternal
> > session" cookie bug 530594.
> 
> I think we should do this. I believe we turned this off by default a long
> time ago when we still tried to load all tabs at once. Is there a reason we
> can't do this?

I don't think "Restore my tabs" was ever the default. There were some performance issues like you mention long ago (which perhaps was a limiting factor to enabling it by default), but that's fixed now that we only load restored tabs on-demand.

I'm also in favor of basically just enabling the pref (or some variation, see comment 5). But Philipp seemed to really want the button. :)

If we've basically got a finished patch, seeing how it's used is fine. Although it would be helpful to gather usage telemetry, and a plan for how we'd use that to get to a decision on enabling it for everyone (as comment 7 suggests). I'm not really clear what that would look like.

Comment 24

[Stephen Horlander [:shorlander]]

(In reply to Erica from comment #20)
> Comment on attachment 8764330 [details]
> Bug 1219725 - Add a button for session restore to the tab bar. ,
> ui-r=shorlander
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/60284/diff/4-5/

Sorry, missed this. Does this patch address Comment 17?

Comment 25

[Erica Wright [:ewright]]

(In reply to Stephen Horlander [:shorlander] from comment #24)
> (In reply to Erica from comment #20)
> > Comment on attachment 8764330 [details]
> > Bug 1219725 - Add a button for session restore to the tab bar. ,
> > ui-r=shorlander
> > 
> > Review request updated; see interdiff:
> > https://reviewboard.mozilla.org/r/60284/diff/4-5/
> 
> Sorry, missed this. Does this patch address Comment 17?

Yes it does. 
 `If you only restore one tab the button doesn't disappear` - this is fixed (I forgot to test with the other restore button).

` We probably shouldn't offer to restore a session that would be the same as a default session` - I agree, but that is also the current behavior of the other restore session button.

`Uses sentence case, buttons should use title case` - now is picking up the same text from the other button 

`Not sure if it's due to this patch, but I am missing the new tab button?` - should be fixed - was behaving differently on different profiles (let me know if it's not)

Comment 26

[Stephen Horlander [:shorlander]]

(In reply to Erica from comment #25)
> `Uses sentence case, buttons should use title case` - now is picking up the
> same text from the other button 

We should use the "Restore Tabs From Last Time" wording. It's an improvement over "Restore Previous Session" because "Session" is pretty jargon-y and ill-defined. The important thing to know is that it will give you the stuff (Tabs) that you had before.

Comment 27

[Erica Wright [:ewright]]

(In reply to Stephen Horlander [:shorlander] from comment #26)
> (In reply to Erica from comment #25)
> > `Uses sentence case, buttons should use title case` - now is picking up the
> > same text from the other button 
> 
> We should use the "Restore Tabs From Last Time" wording. It's an improvement
> over "Restore Previous Session" because "Session" is pretty jargon-y and
> ill-defined. The important thing to know is that it will give you the stuff
> (Tabs) that you had before.

Will do. Waiting for some sort of consensus on whether or not it's worth continuing. (but if it is I'll change it)

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

You should proceed here with the work, don't wait for consensus. When you're patch is ready to land we can experiment with ways that we want to tweak it.

Comment 29

[Stephen Horlander [:shorlander]]

Yeah, we would want this regardless of what we decide on for the default behavior.

Comment 30

[Erica Wright [:ewright]]

https://reviewboard.mozilla.org/r/60284/#review58560

> Unfortunately this particular line has several problems. Our toolbars are customizable, so the first child of the tabstoolbar isn't necessarily the tabstrip. If you want the tabstrip, you can actually just refer to `this` in this method, as it lives on the XBL binding for the tabstrip.

I am not specifically looking for the tab strip. I'm looking for all of the objects within the #TabsToolBar in order to find if any of the children have the flex attr > 1 (specifically the search bar.

> I'm a little confused by this comment. You're mentioning the search bar, but you're not directly referring to it, as far as I can tell? Can you explain what's going on here?

Edited the comment to be more clear. We are only showing the restoreTabsButton if there is room for it to not encroach on the existing tab's space. If any custom object with flex > 1 is in the toolbar, specifically the search bar, it will push the restoreTabsButton into the existing tab's space - in this case, this is allowed.

> I'm confused by this... do we show the button even when there's no session to restore?
> 
> Note also that you're implicitly relying on the window being > 500 or 600 pixels or whatever it is... we should probably not do that because if the machine on which the test runs doesn't have windows that large, this won't be the case.

agreed, since the first test doesn't set the windows size there is no telling what it may be, so it may or may not show up. For the other tests setting the window size is rather important because I need to be able to tell if there is room for the button to exist or not. Any advice on how to get around this?

> I'm a bit confused. The button isn't in the placements at all (it isn't a customizable button, and it's not in the #TabsToolbar but inside #tabbrowser-tabs). Why did you need to add this?

It was my impression this is where I would add it to track clicks for metrics. Is this wrong?

Comment 31

[Erica Wright [:ewright]]

https://reviewboard.mozilla.org/r/60284/#review58560

> Can you elaborate on what this change accomplishes? I'm a bit confused as to how it relates to what you're doing here.

when a foreign thing gets added to the toolbar the `+` (new tab) button hides. A replacement new tab button will show up on the far right corner. I want the two buttons to co-exist. So changing the allowable `currentset` accomplishes this. I could see it possibly having a negative effect as it allows other people to add other things there too, which I believe is what is trying to be prevented.

Comment 32

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/60284/#review58560

> I am not specifically looking for the tab strip. I'm looking for all of the objects within the #TabsToolBar in order to find if any of the children have the flex attr > 1 (specifically the search bar.

So I don't think this is the right way to accomplish this goal, and I also don't think this happens only with the search bar - or conversely, that it would happen with any element with flex > 1. It ends up having similar problems to the ones with the hardcoded 500/600 pixel required width.

What does the "encroaching" of the button actually end up meaning? In XUL you don't normally get overlapping of two siblings without a lot of work.

> agreed, since the first test doesn't set the windows size there is no telling what it may be, so it may or may not show up. For the other tests setting the window size is rather important because I need to be able to tell if there is room for the button to exist or not. Any advice on how to get around this?

You're generally assuming that an absolute window size is sufficient to determine whether or not there is room for the button. I think that is incorrect (because of OS font sizes, because of localization concerns, because tab widths are configurable via prefs, because of custom themes, etc.), and needs to be addressed in the code (and, after that, in the tests).

> It was my impression this is where I would add it to track clicks for metrics. Is this wrong?

Yeah, this list indicates which customizable widgets are in a particular area. What you're adding isn't a customizable widget, so it shouldn't be here.

Comment 33

[Erica Wright [:ewright]]

https://reviewboard.mozilla.org/r/60284/#review58560

> So I don't think this is the right way to accomplish this goal, and I also don't think this happens only with the search bar - or conversely, that it would happen with any element with flex > 1. It ends up having similar problems to the ones with the hardcoded 500/600 pixel required width.
> 
> What does the "encroaching" of the button actually end up meaning? In XUL you don't normally get overlapping of two siblings without a lot of work.

The tab has a full-width state, and when the tabbar is filled up it has a smaller state that it switches to, I'm trying to avoid causing this smaller state to reduce a visual jump when the restoreButton fades away.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Can I suggest that if you are worried about a visual jump when the restoreButton fades away, maybe as part of the fade you can first transition the width of the restoreButton to 0 so that the tabs will animate their expansion?

Comment 35

[Erica Wright [:ewright]]

https://reviewboard.mozilla.org/r/60284/#review58560

> I think you can just call `restoreLastSession()`, and you don't need to add a method on the other binding. :-)

if I want to call it from `tabbrowser.xml` then I need to include the line `let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);` before calling `restoreLastSession()` I suppose I could add a click listener to the element instead from `browser.js`

Comment 36

[:Gijs (he/him)]

(In reply to Erica from comment #35)
> https://reviewboard.mozilla.org/r/60284/#review58560
> 
> > I think you can just call `restoreLastSession()`, and you don't need to add a method on the other binding. :-)
> 
> if I want to call it from `tabbrowser.xml` then I need to include the line
> `let ss =
> Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);`
> before calling `restoreLastSession()` I suppose I could add a click listener
> to the element instead from `browser.js`

Other code in tabbrowser.xml already relies on the SessionStore global existing in the window, so I don't think that's necessary. You can just call it directly via SessionStore.restoreLastSession() - the code in the binding has access to globals in the window where it's running.

Comment 37

[Erica Wright [:ewright]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/5-6/

Comment 38

[Erica Wright [:ewright]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/6-7/

Comment 39

[Erica Wright [:ewright]]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60284/diff/7-8/

Comment 40

[Erica Wright [:ewright]]

https://reviewboard.mozilla.org/r/60284/#review65094

::: browser/locales/en-US/chrome/browser/browser.dtd:261
(Diff revision 8)
>  
>  <!ENTITY historyButton.label            "History">
>  <!ENTITY historySidebarCmd.commandKey   "h">
>  
>  <!ENTITY toolsMenu.label              "Tools">
> -<!ENTITY toolsMenu.accesskey          "T"> 
> +<!ENTITY toolsMenu.accesskey          "T">

note: my editor stripped the trailing whitespace in this file, figured I'd leave it.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8764330 [details]
Bug 1219725 - Add a button for session restore to the tab bar. , ui-r=shorlander

https://reviewboard.mozilla.org/r/60284/#review65164

I only reviewed the diff from v5-v8.

The button has the wrong styling on Windows 10. It is missing the border that is shown on the spec, http://screencast.com/t/6vkYTRaRre and it shows a blue background when hovered, http://screencast.com/t/P7AxkJiyS

::: browser/locales/en-US/chrome/browser/browser.dtd:6
(Diff revision 8)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- LOCALIZATION NOTE : FILE This file contains the browser main menu items -->
> -<!-- LOCALIZATION NOTE : FILE Do not translate commandkeys --> 
> +<!-- LOCALIZATION NOTE : FILE Do not translate commandkeys -->

Although it's not bad to remove the trailing whitespace, it should be done in a standalone bug.

Combining it with this work will make it confusing if someone finds something broken and it points to your patch, or if someone wonders why a patch about session restore just modified the localization line for the tools menu.

::: browser/themes/shared/tabs.inc.css:584
(Diff revision 8)
> +
> +.restore-tabs-button-wrapper {
> +  visibility: hidden;
> +  position:fixed;
> +  padding: 4px;
> +  margin-left: 3px;

margin-inline-start instead of margin-left

::: browser/themes/shared/tabs.inc.css:585
(Diff revision 8)
> +  transition: opacity 300ms ease-out;
> +}
> +
> +.restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;
> +  font-size: .8em;
> +  margin-bottom: 0;
> +  padding: 0 5px;
> +}
> +
> +.restore-tabs-button {
> +  border: 1px solid var(--restore-tabs-button-color);
> +  border-radius: 2px;
> +}
> +
> +.restore-tabs-button-wrapper.restore-fade-out {
> +  opacity: 0;

I think we should remove the fade-out. It doesn't accomplish much and might make the user interface feel slow. Opening a tab is already not super smooth, and introducing this extra animation will increase the "jank"-factor.

Let's just move forward with immediately hiding the button in all cases.

Comment 42

[Katie Broida [:ktbee]]

Attachment: Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

Review commit: https://reviewboard.mozilla.org/r/72698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72698/

Comment 43

[Katie Broida [:ktbee]]

I'm picking up where Erica left off! I added a declaration for a background-color on Linux because I found that I couldn't see the button against the dark background.

Comment 44

[:Gijs (he/him)]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review70894

I've not really checked the styling here, because there's enough tweaking to do with the behaviour. We can look at the styling nearer the end (or perhaps Jared has comments on that).

::: browser/base/content/browser.css:142
(Diff revision 1)
> +/*#TabsToolbar[currentset*="tabbrowser-tabs,alltabs-button,new-tab-button"] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button,
> +#TabsToolbar[currentset*="tabbrowser-tabs,new-tab-button,alltabs-button"] > #tabbrowser-tabs > .tabbrowser-arrowscrollbox > .tabs-newtab-button {
> +  visibility: visible;
> +}*/

Did you mean to have this commented out? Or should this hunk just be removed?

::: browser/base/content/browser.js:7571
(Diff revision 1)
>  
>  var RestoreLastSessionObserver = {
>    init: function () {
>      if (SessionStore.canRestoreLastSession &&
>          !PrivateBrowsingUtils.isWindowPrivate(window)) {
> +      let restoreTabsButtonWrapper = document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button-wrapper");

I'd add a getter for this, because right now we're repeating it 3 times:

```
get restoreTabsButtonWrapper() {
  delete this.restoreTabsButtonWrapper;
  return this.restoreTabsButtonWrapper =
    document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid",
                                            "restore-tabs-button-wrapper");
}
```


In fact, reading further down the patch, it probably makes more sense to add a `<field>` for this on the relevant bit of the tabcontainer binding. Then you can just fetch it once and do:

`let {restoreTabsButtonWrapper} = gBrowser.tabContainer;`

to fetch it in here, and you can simplify the code in the tabbrowser that also currently uses the anonid fetching.

::: browser/base/content/browser.js:7591
(Diff revision 1)
> +    restoreTabsButtonWrapper.removeEventListener("transitionend", this.removeRestoreTabsButton, false);
> +    restoreTabsButtonWrapper.hidden = true;
> +  },
> +  fadeRestoreTabsButton: function () {
> +    let restoreTabsButtonWrapper = document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button-wrapper");
> +    gBrowser.tabContainer.removeEventListener("TabOpen", this.fadeRestoreTabsButton, false);

This unfortunately won't work, because you're adding the listener with a bound reference rather than the original function.

The easiest way to fix this is to just use a handleEvent method on this object to deal with the two event listeners. Then you can just pass `this` as the listener argument to add/removeEventListener. Inside handleEvent, `this` will be correct (so you won't need to `bind` anything). You'll want code that checks what event we're getting and calls the `fadeRestoreTabsButton` or `removeRestoreTabsButton` as appropriate.  See e.g. https://dxr.mozilla.org/mozilla-central/rev/f97a056ae6235de7855fd8aaa04fb1c8d183bd06/browser/components/customizableui/content/panelUI.js#181 as an example.

Though... actually, AFAICT you've kept the transitionend listener but there's no longer any transition or "fade out"? You should probably get rid of that level of indirection completely, and as soon as the new tab is opened just call `restoreTabsButtonWrapper.hidden = true` (or even just `restoreTabsButtonWrapper.remove()` !). That will let you simplify this code a lot, and remove the fadeout/transition references.

::: browser/base/content/browser.js:7600
(Diff revision 1)
>    observe: function () {
>      // The last session can only be restored once so there's
>      // no way we need to re-enable our menu item.
>      Services.obs.removeObserver(this, "sessionstore-last-session-cleared");
>      goSetCommandEnabled("Browser:RestoreLastSession", false);
> +    gBrowser.tabContainer.removeEventListener("TabOpen", this.fadeRestoreTabsButton, false);

You also do this inside `fadeRestoreTabsButton`, so doing it here is probably not necessary?

::: browser/base/content/tabbrowser.xml:4955
(Diff revision 1)
> +        <xul:hbox class="restore-tabs-button-wrapper"
> +                  anonid="restore-tabs-button-wrapper">
> +          <xul:toolbarbutton anonid="restore-tabs-button"
> +                             class="restore-tabs-button"
> +                             onclick="SessionStore.restoreLastSession();"
> +                             label="&restoreLastTabs.label;"/>

The last code I saw that needed to do something like this set the attribute at runtime (from the binding's constructor) based on an attribute (like "restorelasttabslabel") set on the element that we bind here. Then we don't need to include an extra DTD and the browser.xul markup can include the value that we then set in the binding.

::: browser/base/content/tabbrowser.xml:5010
(Diff revision 1)
> +      <field name="buttonWidth">
> +        document.getAnonymousElementByAttribute(this, "anonid", "restore-tabs-button-wrapper").clientWidth;
> +      </field>

This will only be evaluated once. Is that actually what you want? Shouldn't this be a property with a getter?

Also, we should use a more specific name than just "buttonWidth". Maybe "restoreTabsButtonWidth" ?

::: browser/base/content/tabbrowser.xml:5027
(Diff revision 1)
> +      <method name="updateSessionRestoreVisibility">
> +        <body><![CDATA[
> +          let shouldHide = true;
> +          let buttonPadding = 10;
> +
> +          //measure the button width initially on the DOM - it changes due to localization

Super-picky nit: general convention is leaving a space after "//" and using sentence case + punctuation, so please add a space, s/measure/Measure/, and add a period/dot at the end of the sentence. Same comment on some of the other comments.

::: browser/base/content/tabbrowser.xml:5033
(Diff revision 1)
> +          let buttonWidth = this.buttonWidth;
> +          let wrapper = document.getAnonymousElementByAttribute(this, "anonid", "restore-tabs-button-wrapper");
> +
> +          // if there is a flex > 1 element in the TabToolbar then there will not be a visual
> +          // jump when the button fades, so we can show it so long as there is some room.
> +          if (!wrapper.classList.contains("restore-fade-out") && wrapper.getAttribute('session-exists') === "true") {

We don't normally use strict-equals, and the session-exists attribute just won't be there if there isn't a session, so this can be simplified to:

if (!wrapper.classList.contains("restore-fade-out") && wrapper.hasAttribute("session-exists"))

(Also: please use double quotes rather than single quotes.)

::: browser/base/content/tabbrowser.xml:5034
(Diff revision 1)
> +            let toolbarNode = document.getElementById("TabsToolbar").firstChild;
> +            while (toolbarNode && toolbarNode.nodeType === 1) {
> +              if (toolbarNode.flex > 1) {
> +                if ((wrapper.hidden === true && toolbarNode.boxObject.width >= (buttonWidth * 2.5)) || (wrapper.hidden !== true && toolbarNode.boxObject.width >= buttonWidth * 1.5)) {
> +                  shouldHide = false;
> +                }
> +              }
> +              toolbarNode = toolbarNode.nextElementSibling;
> +            }

If we keep this, rather than the manual loop and having to worry about element types, I would do something like this:

```
let flexNodes = document.getElementById("TabsToolbar").querySelector("[flex]");
for (let flexNode of flexNodes) {
  if (flexNode.flex > 1 &&
      /* Long condition here */) {
  }
}
```

There are a couple of other notes here to your existing code:

First, where do the magic `buttonWidth * 1.5` and `buttonWidth * 2.5` values come from? Did you just test with the search box and determine that these values were about right? Or something else? The values (1.5/2.5) are likely to be different for different things with flex>1, depending on their minimum intrinsic width etc.  There is no really sane way of determining those minimum widths from JS.

**IMO we should not care about the 'search bar in the tabstrip' case. If the user has customized their browser like this, they likely don't need telling about the possibilities of session restore. If other people think this is important, we can delay dealing with this to a followup bug - right now, iterating over this is taking a lot of time when it's comparatively not very important in the "grand scheme" of the feature itself (ie very few people will hit this codepath anyway).**

Second, comparing to `true` and `false` literals in almost any language is a "code smell". It is effectively almost always unnecessary. It is less code, and easier to read to do

```
if (something)
```
than
```
if (something === true)
```
and likewise
```
if (!something)
```
rather than
```
if (something === false)
```

While the meanings of strict-comparison to booleans and just the condition are subtly different in JS, the difference rarely matters if your variables are sane (that is, something should either be a boolean or a string or a number or an object/null, not a mixture where the type of thing it is matters over the value of the thing and you have to be "on your guard" all the time). In this particular case, we can be pretty sure that element.hidden is a "real" boolean, so the strict check is unnecessary.

::: browser/base/content/tabbrowser.xml:5051
(Diff revision 1)
> +            let newTabButton = document.getAnonymousElementByAttribute(this, "anonid", "tabs-newtab-button");
> +            let childNodesTotalWidth = firstTab.clientWidth + newTabButton.clientWidth + wrapper.clientWidth;
> +
> +            //if there is enough extra room in the flex-element "tabbrowser-tabs"
> +            //then there is room for the button.
> +            if ((wrapper.hidden && document.getElementById("tabbrowser-tabs").clientWidth - childNodesTotalWidth >= buttonWidth + buttonPadding) || (!wrapper.hidden === true && document.getElementById("tabbrowser-tabs").clientWidth - childNodesTotalWidth >= buttonPadding)) {

This would love to be split over several lines so it's easier to read. In particular, I suspect it'll help if you add a temp for the width of the tabstrip. Or actually, can we just use `this.(tabContainer.)mTabstripWidth` instead? It's also a little odd that the binding here relies on the document ID of the element which we're binding (ie tabbrowser-tabs). You should be able to use `this` or `this.tabContainer` or something like this, if `mTabstripWidth` doesn't work out.

Otherwise, it looks like the code currently does approximately this:

if (no wrapper and tabstrip width - (tab + newtab + wrapper width) >= wrapper width + padding)
OR
if (have wrapper and tabstrip width - (tab + newtab + wrapper width) >= button padding)

I'm struggling to follow this. I'm guessing wrapper width (as part of `childNodesTotalWidth`) is 0 when the button is hidden. So then we check whether effectively, we have room for the width + padding of the button in the "spare" space we have.

When we've already shown the button, we check if the total width of the wrapper (which includes the button and its padding) and the new tab button and the single tab is greater than or equal to... the padding on the button? Why do we need to do that? Is this just to check that we're not making the single tab smaller? In that case, I expect the comparison should simply be to check against > 0  (ie there should be at least some space remaining). Or does that not work for some reason?

Finally, I think this code is harder to read because both comparisons include the width of the child nodes, but in one case that width includes the width of the wrapper and in one case it does not, plus we have two ways of measuring the width of the wrapper. It would help understandability of the code if the "meaning" of `childNodesTotalWidth` didn't change, and if we used just one way of measuring the width of the wrapper. Is that possible or is there another subtlety I'm missing?


Also note the creeping in of another:

`!foo === true`

(which is even semantically exactly the same as just `!foo` because the `!` is coercing the value to a boolean anyway, and so the comparison to `true` is redundant.)

::: browser/themes/linux/browser.css:1574
(Diff revision 1)
> +/* Restore tabs button */
> +.restore-tabs-button {
> +  background-color: -moz-Dialog;
> +}

What's the foreground color here? We should make sure it is `-moz-dialogtext`, or we should make the background color you're declaring here match whatever the foreground color is. Have you checked if we need special styling on any other OS? It might need `color: inherit` somewhere (and in fact, maybe that would help on Linux?), but I've not checked.

::: browser/themes/shared/tabs.inc.css:11
(Diff revision 1)
>  
>  :root {
>    --tab-toolbar-navbar-overlap: 1px;
>    --navbar-tab-toolbar-highlight-overlap: 1px;
>    --tab-min-height: 31px;
> +  --restore-tabs-button-color: hsla(0,0%,16%,.2);

This should probably be called restore-tabs-button-border-color , as it only controls the border and not the foreground color.

::: browser/themes/shared/tabs.inc.css:14
(Diff revision 1)
>    --navbar-tab-toolbar-highlight-overlap: 1px;
>    --tab-min-height: 31px;
> +  --restore-tabs-button-color: hsla(0,0%,16%,.2);
>  }
> +
> +:root[lwthemetextcolor="dark"] {

Nit: you can use the :-moz-lwtheme-brighttext / :-moz-lwtheme-darktext pseudoselectors instead.

Comment 45

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review71134

Clearing review, I'll wait until a new patch is attached with Gijs' feedback addressed.

::: browser/base/content/tabbrowser.xml:4950
(Diff revision 1)
> +        <xul:hbox class="restore-tabs-button-wrapper"
> +                  anonid="restore-tabs-button-wrapper">
> +          <xul:toolbarbutton anonid="restore-tabs-button"

Can you test to see what happens when Customize Mode is entered and this button is visible?

We should hide the button in customize mode.

Comment 46

[Peter Dolanjski [:pdol]]

I'd like to test how release users perceive the session restore button implementation and whether or not it meets their needs before shipping in the core product.  I'll define what we want to test and post it here for input/discussion.

Comment 47

[Katie Broida [:ktbee]]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72698/diff/1-2/

Comment 48

[Katie Broida [:ktbee]]

Thanks for the feedback, Gijs! Coming in as the second person on this patch, I've realized I was a little lost as well in terms of where the button width numbers were coming from. I believe a lot of the complexity had to do with making sure there was enough space for the new tab button to show and also to allow for the fade away animation. I've added a simpler one (as Jared suggested) that transitions the button's max-width to zero to hopefully avoid jank. I also simplified some of the tabs math to reflect the way I've understood this patch's goals. Let me know if I've missed something/it's not what we're going for. 

One issue I haven't figure out yet is how to make sure the restore tabs button appears when running tests. Currently some of my tests are failing and I believe it's because the restore tabs button isn't appearing reliably (when I do the same tests manually I get the results I expect). To get the button to appear, I mimicked how I've been testing it manually by opening a window, closing it, then re-opening it again. Let me know if there's a better way to go about this.

Comment 49

[Katie Broida [:ktbee]]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review71134

> Can you test to see what happens when Customize Mode is entered and this button is visible?
> 
> We should hide the button in customize mode.

When in Customize Mode, I found that the restore tabs button was already hidden even without specifying it should be hidden in updateSessionRestoreVisibility().

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Katie Broida [:ktbee] from comment #48)
> One issue I haven't figure out yet is how to make sure the restore tabs
> button appears when running tests. Currently some of my tests are failing
> and I believe it's because the restore tabs button isn't appearing reliably
> (when I do the same tests manually I get the results I expect). To get the
> button to appear, I mimicked how I've been testing it manually by opening a
> window, closing it, then re-opening it again. Let me know if there's a
> better way to go about this.

Just a hunch, but you might need to wait for session store to be saved. Walking through the steps manually would give enough time for state to get saved but through automation it might be moving too fast.

Comment 51

[:Gijs (he/him)]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review74350

Closer! (and simpler!)

But still a few issues. I didn't look at the test yet - I hope Jared's feedback will help you get that going. If not, please do feel free to ping me or Jared on IRC or via email or on this bug.

::: browser/base/content/browser.js:7652
(Diff revision 2)
>      if (SessionStore.canRestoreLastSession &&
>          !PrivateBrowsingUtils.isWindowPrivate(window)) {
> +      let {restoreTabsButtonWrapper} = gBrowser.tabContainer;
> +      restoreTabsButtonWrapper.setAttribute("session-exists", "true");
> +      // After restoreTabsButtonWrapper's width has been measured in tabbrowser.xml,
> +      // we need remove the styling in tabs.inc.css that hides it.

Nit: "override" rather than "remove".

::: browser/base/content/browser.js:7661
(Diff revision 2)
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> -
> +  removeRestoreButton: function () {
> +    let {restoreTabsButtonWrapper} = gBrowser.tabContainer;

Can you add a comment here saying the method isn't bound to |this| because it's invoked from an event handler? Or use the handleEvent() stuff I mentioned in the last review?

::: browser/base/content/browser.js:7671
(Diff revision 2)
> +  },
>    observe: function () {
>      // The last session can only be restored once so there's
>      // no way we need to re-enable our menu item.
>      Services.obs.removeObserver(this, "sessionstore-last-session-cleared");
> +    gBrowser.tabContainer.removeEventListener("TabOpen", this.removeRestoreButton);

I think I suggested doing the removing should happen inside the removeRestoreButton code, because now the listener will not be removed if a tab is opened. Because `this` will be wrong inside the listener right now, I guess that's a little tricky if you're not refactoring to use the handleEvent things... but we do need to make sure the listener gets removed. :-)

::: browser/base/content/tabbrowser.xml:5013
(Diff revision 2)
> +          let restoreTabsButton = document.getAnonymousElementByAttribute(
> +            gBrowser.tabContainer, "anonid", "restore-tabs-button");

I think this can just be restoreTabsButtonWrapper.firstChild. :-)

::: browser/base/content/tabbrowser.xml:5025
(Diff revision 2)
> +          // Get the widths of the elements in the tabs area
> +          let restoreTabsButtonWidth = restoreTabsButtonWrapper.clientWidth;
> +          let tabs = document.getElementById("tabbrowser-tabs");
> +          let tabsChildNodes = Array.from(tabs.children);
> +          let tabsUsedSpace = 0;
> +          tabsChildNodes.forEach(node => tabsUsedSpace += node.clientWidth);

There are issues here where the tab resizes to become smaller, and if you resize the window slowly, you can see flickering with the tabstrip intermittently going to overflow mode.

I'm not sure this is sufficient to fix all of these problems, but a way to get this code to provide a more consistent result is to reuse the `_tabDefaultMaxWidth` value from this binding, which you can then just multiply with the number of tabs we have, which avoids the loop an dreading all the tabs' client width.

You'd have to then check manually if you can reproduce the flickering still...

::: browser/themes/linux/browser.css:1605
(Diff revision 2)
> +  background-color: -moz-Dialog;
> +  color: -moz-dialogText;

These don't seem like the colors we typically use for a button. I think we probably use ThreeDFace and ButtonText, if anything. Does that work or does it look really ugly or something?

Alternatively, maybe we can make it use the same background + foreground colors as the tabs, without making it connect to the navbar? That feels like it would work relatively well.

::: browser/themes/shared/tabs.inc.css:547
(Diff revision 2)
> +  margin-inline-start: 3px;
> +}
> +
> +.restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;
> +  font-size: .8em;

The text looks very small to me on Windows, with antialiasing artifacts as well. Why do we need to give this item a custom text size? We don't normally do that, and it feels like the font and text size we use for a tab would be fine to use here.

::: browser/themes/shared/tabs.inc.css:554
(Diff revision 2)
> +  /* The button will likely never reach this width, but it is specified here for the transition */
> +  max-width: 400px;

While I admire your optimism, I worry that on locales with long text (German, Russian, Ukrainian) we will end up breaking this limit, if not with the default font size then definitely with some other windows font sizes. Using `em` for the units would alleviate some of that problem, I suspect.

::: browser/themes/shared/tabs.inc.css:570
(Diff revision 2)
> +  border-color: hsl(0, 0%, 70%);
> +}
> +
> +.restore-tabs-button[animate=true] {
> +  max-width: 0;
> +  transition: max-width 300ms;

I noticed the juggling of the attribute and that looks a bit odd. Can't we just set transition: on the button at all times and do away with the extra attribute? Does that not work?

Comment 52

[Peter Dolanjski [:pdol]]

Before landing, can we have a feature flag (or equivalent) so that this can be tested before letting it ride the trains?  I'm drafting a test plan here: https://docs.google.com/document/d/1KLVhbeovQAc-B-M95jx5UdTXRzte3WvGsK6Le0UqrF0/edit#heading=h.23kbtdsiyvwa

Comment 53

[Jared Wein [:jaws] (please needinfo? me)]

Gijs pushed the patch on this bug to tryserver so we could get some builds available for user testing. The trypush is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bf809af9dd7

Click on the green (B) next to a platform then click on Job Details to find a link to all of the uploaded files for that specific build.

Peter and Philipp, flagging you so you both see this. Let Gijs or I know if you need anything else.

Comment 54

[Peter Dolanjski [:pdol]]

Thanks Jared.  Philipp, I'll let you give it a try to see if it is sufficient for usertesting.com.

Comment 55

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yep, the feature works as expected! I do have a few comments on styling, but that's not relevant for testing.

There's on issue though: could we sign the build?
We've seen in previous tests that these prompts (https://cl.ly/0c0T222a2A1H) are often big barriers for users and might make them drop out of tests.

Comment 56

[Katie Broida [:ktbee]]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72698/diff/2-3/

Comment 57

[Katie Broida [:ktbee]]

(In reply to :Gijs Kruitbosch from comment #51)
> Comment on attachment 8782591 [details]
> Bug 1219725 - Add a button for session restore to the tab bar.

> I didn't look at the test yet - I hope Jared's
> feedback will help you get that going. If not, please do feel free to ping
> me or Jared on IRC or via email or on this bug.

Thanks for taking a look at this Gijs! I haven't finished writing tests yet, but I thought it would be good to get some feedback on your other points in the meantime. 

> There are issues here where the tab resizes to become smaller, and if you
> resize the window slowly, you can see flickering with the tabstrip
> intermittently going to overflow mode.
> 
> I'm not sure this is sufficient to fix all of these problems, but a way to
> get this code to provide a more consistent result is to reuse the
> `_tabDefaultMaxWidth` value from this binding, which you can then just
> multiply with the number of tabs we have, which avoids the loop an dreading
> all the tabs' client width.
> 
> You'd have to then check manually if you can reproduce the flickering
> still...

I saw the flickering effect you mentioned, and found that it was because the restoreTabsButton's width was toggling between 0 and its width when it wasn't hidden. Because of this changing width, the if statement comparing `(tabs.clientWidth - tabsUsedSpace) > restoreTabsButtonWidth` sometimes evaluated to `true` when it shouldn't have. The solution I found for this was to just save the button's initial width in a field in tabbrowser.xml so that it wouldn't change as the browser resizes. 

> ::: browser/themes/linux/browser.css:1605
> (Diff revision 2)
> > +  background-color: -moz-Dialog;
> > +  color: -moz-dialogText;
> 
> These don't seem like the colors we typically use for a button. I think we
> probably use ThreeDFace and ButtonText, if anything. Does that work or does
> it look really ugly or something?

There was no aesthetic reason I chose those colors, I had just thought those were the defaults. These color suggestions work also. 

> ::: browser/themes/shared/tabs.inc.css:570
> (Diff revision 2)
> > +  border-color: hsl(0, 0%, 70%);
> > +}
> > +
> > +.restore-tabs-button[animate=true] {
> > +  max-width: 0;
> > +  transition: max-width 300ms;
> 
> I noticed the juggling of the attribute and that looks a bit odd. Can't we
> just set transition: on the button at all times and do away with the extra
> attribute? Does that not work?

I'm not sure what you mean by setting transition on the button at all times. Doesn't it need a change in state to trigger the transition? Toggling the animate attribute was my attempt at triggering the transition using JavaScript. 

More soon on the tests ...

Comment 58

[:Gijs (he/him)]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

https://reviewboard.mozilla.org/r/72698/#review83762

Closer! We should work on the tests and the finer points of the transition, but I think we're almost there.

We should probably put this behind a feature pref. To do this, you would add a pref("browser.tabs.restorebutton", true); line to browser/app/profile/firefox.js, and you'd check the browser.tabs.restorebutton pref in the updateSessionRestoreButton() method (and not show the button if the pref is set to false). Then we can turn this on/off for experiments and on beta/release/whatever if we decide to ship it or not ship it or just turn session restore on for everybody or something.

::: browser/base/content/browser.js:7780
(Diff revision 3)
> +      gBrowser.tabContainer.addEventListener("TabOpen", this, false);
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
> +  handleEvent: function(aEvent) {

Nit: empty lines between the methods, please.

::: browser/base/content/browser.js:7784
(Diff revision 3)
>    },
> +  handleEvent: function(aEvent) {
> +    switch (aEvent.type) {
> +      case "TabOpen":
> +        this.removeRestoreButton();
>  

Nit: and you can remove this empty line.

::: browser/base/content/browser.js:7792
(Diff revision 3)
> +  removeRestoreButton: function () {
> +    let {restoreTabsButtonWrapper} = gBrowser.tabContainer;
> +    let restoreTabsButton = restoreTabsButtonWrapper.firstChild;
> +    restoreTabsButton.setAttribute("animate", true);
> +    window.setTimeout(function(){restoreTabsButtonWrapper.hidden = true;
> +                                 restoreTabsButton.removeAttribute("animate");}, 290);

It probably isn't necessary to remove the attribute (or the style property when that's set).

Nit: spaces around the { brace and before the } brace, please. This could be shortened to:

window.setTimeout(() => restoreTabsButtonWrapper.hidden = true, 290);

Speaking of 290... what's that magical value? :-)
It looks like the animation is 300ms, so shouldn't this just be 300ms?

Oh... I think our feedback has been confusing here, sorry. If we're keeping a transition, we should continue to use a transitionend listener to remove the item when the transition finishes.

::: browser/base/content/tabbrowser.xml:5235
(Diff revision 3)
> +          let tabsUsedSpace = 0;
> +          tabsChildNodes.forEach(node => tabsUsedSpace += node.clientWidth);

Nit: let tabsUsedSpace = tabsChildNodes.reduce((sum, node) => sum + node.clientWidth, 0);

Note that I still think using `_tabDefaultMaxWidth` and multiplying with the number of tabs would be a good idea.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:2
(Diff revision 3)
> +// Ensure new tab button is visible after resizing window
> +add_task(function* (){

None of the tests here check that the tab restore button is actually visible, or that clicking it works. Can we simulate the case where it appears? This might require mocking some session restore stuff. Maybe Mike de Boer can help - you could needinfo him and ask?

This first task seems to not do very much - it resizes, and then it checks that the resize works, but crucially it doesn't check anything after that.

::: browser/base/content/test/tabbrowser/browser_tabbar_sessionrestore_button.js:11
(Diff revision 3)
> +  win = yield BrowserTestUtils.openNewBrowserWindow();
> +
> +  let tabBrowser = win.document.getElementById('tabbrowser-tabs');
> +  let {restoreTabsButtonWrapper} = gBrowser.tabContainer;
> +  let newTabButton = win.document.getAnonymousElementByAttribute(win.gBrowser.tabContainer, 'class', 'tabs-newtab-button');
> +  is(newTabButton.style.visibility != 'collapse', true, 'new tab button is not visibility: collapse');

Nit: isnot(newTabButton.style.visibility, "collapse", "new tab button is not visibility: collapse");

::: browser/base/moz.build:25
(Diff revision 3)
>      'content/test/newtab/browser.ini',
>      'content/test/plugins/browser.ini',
>      'content/test/popupNotifications/browser.ini',
>      'content/test/referrer/browser.ini',
>      'content/test/social/browser.ini',
> +    'content/test/tabbrowser/browser.ini',

Since we started, the test dir now has a tabs/ subdir. Can we reuse that instead for the test?

::: browser/themes/linux/browser.css:1364
(Diff revision 3)
> +/* Restore tabs button */
> +.restore-tabs-button {
> +  background-color: ThreeDFace;
> +  color: ButtonText;
> +}

We'll need similar styling on OS X and Windows, I think.

::: browser/themes/shared/tabs.inc.css:548
(Diff revision 3)
> +  display: none;
> +}
> +
> +.restore-tabs-button-wrapper {
> +  visibility: hidden;
> +  position:fixed;

Can you add a comment about why we're using position: fixed in the CSS here?

::: browser/themes/shared/tabs.inc.css:576
(Diff revision 3)
> +.restore-tabs-button:-moz-lwtheme-brighttext {
> +  border-color: hsl(0, 0%, 70%);
> +}
> +
> +.restore-tabs-button[animate=true] {
> +  max-width: 0;

So my point was that you can set the transition: CSS property in the .restore-tabs-button selector, and instead of setting this "animate" attribute, you could use element.style.maxWidth = 0 instead. Then we don't need this rule.

Comment 59

[Blake Winton (:bwinton) (:☕️)]

Hey Katie, are you still working on this bug?  I've been talking with some people about running some tests with it, and wondered if I could help get the patch finished and landed…  Thanks!

Comment 60

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #59)
> Hey Katie, are you still working on this bug?  I've been talking with some
> people about running some tests with it, and wondered if I could help get
> the patch finished and landed…  Thanks!

She is, she's looking at automated tests. She's been in touch with Jared, Mike de Boer and me via e-mail.

From the comments from Philipp et al, I thought that the patch as-is could already be tested? I would suggest that further conversation about doing a test run of this feature might be best over email / IRC rather than in the bug where the feature is being implemented.

Comment 61

[:Gijs (he/him)]

Per discussion with Jared, it feels like it might make sense to pre-land the strings so this can make the 52 train even if the tests take a little longer to sort out.

Katie, could you create a patch file with just the tabbrowser.properties change? Then Jared or I can rubberstamp that and we can land it in 52. When the patch is done, we can ask for uplift to aurora if we don't make the merge day cutoff (next Monday).

Comment 62

[Katie Broida [:ktbee]]

Sure thing, Gijs. I'll make those changes now and continue working on the tests after submitting the patch you need for 52.

Comment 63

[Katie Broida [:ktbee]]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72698/diff/3-4/

Comment 64

[:Gijs (he/him)]

Philipp can review the label, I think. :-)

Comment 65

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8782591 [details]
Bug 1219725 - This patch adds a label for the forthcoming Restore Tabs Button.

Looks good!

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a3f82cfc81
This patch adds a label for the forthcoming Restore Tabs Button. r=phlsa,gijs

Comment 67

[:Gijs (he/him)]

Pushed to inbound to avoid issues with reusing the reviewboard thingy for autoland.

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6a3f82cfc81

Comment 69

[Katie Broida [:ktbee]]

Hi everyone, I wanted to let you know that I'll be starting a new job soon and won't be able to keep up with this bug. I don't want to hold the process up, so I'll go ahead an un-assign myself now :) Thank you for all your guidance on it!

Comment 70

[Erica Wright [:ewright]]

Attachment: Bug 1219725 - Add a button for session restore to the tab bar. ui-r=shorlander

Review commit: https://reviewboard.mozilla.org/r/143224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143224/

Comment 71

[Erica Wright [:ewright]]

TODO: I'm still going to add a pref and default it off, coming shortly

Comment 72

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/1-2/

Comment 73

[:Gijs (he/him)]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149192

This is pretty good, but I'd like to avoid this code causing layout flushes on startup when the button is enabled, if possible.

I haven't really reviewed the test yet, also because I'm not very familiar with the marionette python framework. Hopefully Mike can help with that.

::: browser/base/content/browser.js:207
(Diff revision 2)
>    }
>    return null;
>  });
>  
>  const nsIWebNavigation = Ci.nsIWebNavigation;
> +const TABBAR_RESTORE_SESSION_PREF = "browser.tabs.restorebutton";

Please don't add a global. You only use this once, so I'm not sure a const is really necessary, but if it is, you can put it on the RestoreLastSessionObserver object.

::: browser/base/content/browser.js:8073
(Diff revision 2)
>  
>  var RestoreLastSessionObserver = {
>    init() {
>      if (SessionStore.canRestoreLastSession &&
>          !PrivateBrowsingUtils.isWindowPrivate(window)) {
> +      if (Services.prefs.getBoolPref(TABBAR_RESTORE_SESSION_PREF)){

Nit: space after ). I'd expect eslint to pick this up - does it not?

::: browser/base/content/browser.js:8081
(Diff revision 2)
> +        // After restoreTabsButtonWrapper's width has been measured in tabbrowser.xml,
> +        // we need to override the styling in tabs.inc.css that hides it.
> +        restoreTabsButtonWrapper.style.position = "initial";
> +        restoreTabsButtonWrapper.style.visibility = "visible";
> +        gBrowser.tabContainer.updateSessionRestoreVisibility();
> +        gBrowser.tabContainer.addEventListener("TabOpen", this, false);

You don't need to pass `false` as the last parameter.

::: browser/base/content/tabbrowser.xml:5868
(Diff revision 2)
> +      <method name="updateSessionRestoreVisibility">
> +        <body><![CDATA[
> +          let shouldHide = true;
> +          let { restoreTabsButtonWrapper } = gBrowser.tabContainer;
> +
> +          if (!restoreTabsButtonWrapper.getAttribute('session-exists')) {

Nit: double quotes please.

::: browser/base/content/tabbrowser.xml:5875
(Diff revision 2)
> +          let newTabButton = document.getAnonymousElementByAttribute(
> +          gBrowser.tabContainer, "anonid", "tabs-newtab-button");

Nit: indent the second line, please.

::: browser/base/content/tabbrowser.xml:5877
(Diff revision 2)
> +          }
> +
> +          let restoreTabsButton = restoreTabsButtonWrapper.firstChild;
> +          let newTabButton = document.getAnonymousElementByAttribute(
> +          gBrowser.tabContainer, "anonid", "tabs-newtab-button");
> +          restoreTabsButton.setAttribute("label", this.tabbrowser.mStringBundle.getString("tabs.restoreLastTabs"));

Hmm. So, we're doing a sync reflow a bit later because we're getting the client width of a number of items, and this label setting will have dirtied layout.

I'm wondering if we can put this label in a DTD, and ensure the button always has the label, and then use DOMWindowUtils to get the bounds without flushing layout. I don't know if that works, it would depend on whether layout has already happened at least once by this point.

::: browser/base/content/tabbrowser.xml:5883
(Diff revision 2)
> +          // There will only ever be one tab when the restore button is showing.
> +          let firstTab = tabs.firstChild;
> +          let tabbarUsedSpace = firstTab.clientWidth + newTabButton.clientWidth;

This can almost definitely use nsIDOMWindowUtils to get the width without flushing.

::: browser/base/content/tabbrowser.xml:5889
(Diff revision 2)
> +          let firstTab = tabs.firstChild;
> +          let tabbarUsedSpace = firstTab.clientWidth + newTabButton.clientWidth;
> +
> +          // Subtract the elements' widths from the available space to ensure the restoreTabsButton
> +          // won't cause any overflow.
> +          if ((tabs.clientWidth - tabbarUsedSpace) > restoreTabsButtonWidth) {

Same here for tabs.clientWidth.

::: browser/themes/linux/browser.css:1014
(Diff revision 2)
> +/* Restore tabs button */
> +.restore-tabs-button {
> +  background-color: ThreeDFace;
> +  color: ButtonText;
> +}

Should this live in shared? How is the button styled (color-wise) on other platforms?

I would suggest doing a trypush and getting Stephen or ...someone... to do a final design pass (unless that already happened and I missed it - in which case please say so on the bug? :-) ).

::: browser/themes/shared/tabs.inc.css:597
(Diff revision 2)
>  
>  .alltabs-endimage[blocked] {
>    list-style-image: url(chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-blocked);
>  }
> +
> +.restore-tabs-button > .toolbarbutton-icon {

Super-nitty nit: please put this after the rule for `.restore-tabs-button`. Ditto for the rule for that item's `.toolbarbutton-text`.

::: browser/themes/shared/tabs.inc.css:615
(Diff revision 2)
> +  padding: 0 5px;
> +}
> +
> +.restore-tabs-button {
> +  -moz-appearance: none;
> +  /* The button will likely never reach this width, but it is specified here for the transition */

Please put this comment next to the line it applies to.

::: browser/themes/shared/tabs.inc.css:622
(Diff revision 2)
> +  border-radius: 15px;
> +  max-width: 25em;
> +  transition: max-width 300ms;
> +}
> +
> +.restore-tabs-button:-moz-lwtheme-darktext {

Should there be hover/active styling of sorts?

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:16
(Diff revision 2)
> +        self.marionette.enforce_gecko_prefs({'browser.tabs.restorebutton': True})
> +
> +        # Each list element represents a window of tabs loaded at
> +        # some testing URL, the URLS are arbitrary.
> +        self.test_windows = set([
> +            (self.marionette.absolute_url('macbeth.html'),

I approve!

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

As maintainer of the fx-ui tests I had a quick look over this test. My comments you find inline.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:18
(Diff revision 2)
> +        # Each list element represents a window of tabs loaded at
> +        # some testing URL, the URLS are arbitrary.
> +        self.test_windows = set([
> +            (self.marionette.absolute_url('macbeth.html'),
> +             self.marionette.absolute_url('formPage.html'),
> +             self.marionette.absolute_url('clicks.html')),

Please do not cross boundaries between Marionette and Firefox UI tests. The latter have their own testcase files under /testing/firefox-ui/resources

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:22
(Diff revision 2)
> +        self.setup_session()
> +        restore_tabs_button_wrapper = self.marionette.find_element('id', 'tabbrowser-tabs').find_element('anon attribute', {'anonid': 'restore-tabs-button-wrapper'})

Firefox ui tests are using puppeteer to work with elements. Please change that here, or move the test to a plain Marionette tests like test_refresh_firefox.py.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:34
(Diff revision 2)
> +        # Set the window small enough that the button disappears.
> +        self.marionette.set_window_size(335, 335)
> +        self.assertEqual(restore_tabs_button_wrapper.value_of_css_property('display'), 'none')
> +
> +    def test_session_exists(self):
> +        self.setup_session()

Keep in mind that you are doing two unnecessary restarts between each of the tests. This takes a lot of extra time. By setting a class variable we could get around this.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:47
(Diff revision 2)
> +        self.setup_session()
> +        restore_tabs_button = self.marionette.find_element('id', 'tabbrowser-tabs').find_element('anon attribute', {'anonid': 'restore-tabs-button'})
> +
> +        # The new browser window is not the same window as last time,
> +        # and did not automatically restore the session, so there is only one tab.
> +        self.assertTrue(len(self.puppeteer.windows.all[0].tabbar.tabs) == 1)

There is always `self.browser.tabbar.tabs` available. Also use `assertEqual` instead with a message what did not work.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:53
(Diff revision 2)
> +
> +        restore_tabs_button.click()
> +
> +        # After clicking on the restore button, there are multiple tabs
> +        # created from the previous session.
> +        self.assertTrue(len(self.puppeteer.windows.all[0].tabbar.tabs) > 1)

Better check for the correct windows and tabs being opened? Or don't we care about the order in the Marionette test, and is all that done in the Mochitest?

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:56
(Diff revision 2)
> +        # After clicking on the restore button, there are multiple tabs
> +        # created from the previous session.
> +        self.assertTrue(len(self.puppeteer.windows.all[0].tabbar.tabs) > 1)
> +
> +    def test_pref_off_button_does_not_show(self):
> +        self.marionette.enforce_gecko_prefs({'browser.tabs.restorebutton': False})

I think you may want to have two different classes in this test module. That way you wouldn't need a separate `setup_session` method.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:68
(Diff revision 2)
> +
> +    def setup_session(self):
> +        self.open_windows(self.test_windows)
> +
> +        # We need to delay here in order to allow the session to be created.
> +        time.sleep(1)

This is bad. We should never do this in a test. Why is there no observer notification?

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:92
(Diff revision 2)
> +                    if index > 0:
> +                        with self.marionette.using_context('chrome'):
> +                            win.tabbar.open_tab()
> +                    self.marionette.navigate(url)
> +
> +    def tearDown(self):

Please move all the helper methods up to the top of the class. That's how it is done for all other fx ui tests.

Comment 75

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149192

> Nit: space after ). I'd expect eslint to pick this up - does it not?

There's a linter!? (ah, I found it, `./mach lint`)

> Hmm. So, we're doing a sync reflow a bit later because we're getting the client width of a number of items, and this label setting will have dirtied layout.
> 
> I'm wondering if we can put this label in a DTD, and ensure the button always has the label, and then use DOMWindowUtils to get the bounds without flushing layout. I don't know if that works, it would depend on whether layout has already happened at least once by this point.

in a previous life, this was defined in a DTD file, but then I believe after this comment from you:
https://reviewboard.mozilla.org/r/72698/diff/1/
"The last code I saw that needed to do something like this set the attribute at runtime (from the binding's constructor) based on an attribute (like "restorelasttabslabel") set on the element that we bind here. Then we don't need to include an extra DTD and the browser.xul markup can include the value that we then set in the binding."

led ktbee to change it to how it is currently. 

So, what I've done for now is move it to the constructor, so it will no longer be re set on each resize event.

> Same here for tabs.clientWidth.

should I do the same for `restoreTabsButtonWidth` property and field? though they only get run once?

> Should this live in shared? How is the button styled (color-wise) on other platforms?
> 
> I would suggest doing a trypush and getting Stephen or ...someone... to do a final design pass (unless that already happened and I missed it - in which case please say so on the bug? :-) ).

This was leftover from the old patch https://reviewboard.mozilla.org/r/72698/diff/3/. I haven't tested it on linux yet nor on anything other than mac.
I'll add it to my TODOs

> Should there be hover/active styling of sorts?

I'll ping @shorlander for that

Comment 76

[:Gijs (he/him)]

(In reply to Erica from comment #75)
> > Nit: space after ). I'd expect eslint to pick this up - does it not?
> 
> There's a linter!? (ah, I found it, `./mach lint`)

Yes, ./mach eslint also works and should be sufficient (that is, this is frontend-only and I don't think you care about python/C++ linting). You can also pass it paths, and you can set it up to run on commit/amend - there's an hg extension that I think Mossop blogged about recently.

> > Hmm. So, we're doing a sync reflow a bit later because we're getting the client width of a number of items, and this label setting will have dirtied layout.
> > 
> > I'm wondering if we can put this label in a DTD, and ensure the button always has the label, and then use DOMWindowUtils to get the bounds without flushing layout. I don't know if that works, it would depend on whether layout has already happened at least once by this point.
> 
> in a previous life, this was defined in a DTD file, but then I believe after
> this comment from you:

Well, this is embarrassing!

> https://reviewboard.mozilla.org/r/72698/diff/1/
> "The last code I saw that needed to do something like this set the attribute
> at runtime (from the binding's constructor) based on an attribute (like
> "restorelasttabslabel") set on the element that we bind here. Then we don't
> need to include an extra DTD and the browser.xul markup can include the
> value that we then set in the binding."
> 
> led ktbee to change it to how it is currently. 
> 
> So, what I've done for now is move it to the constructor, so it will no
> longer be re set on each resize event.

Ah, yes, I forgot there wasn't really a good dtd file to use here because we're in a XBL binding, sorry. You could probably do it by setting it directly on the <tabs> element in browser.xul, but using the constructor is simpler.

The main aim is ensuring the label is set by the time the first layout runs, for which the constructor /should/ work, I think. I'm afraid that when I last reviewed this, we weren't paying attention to the sync layout stuff as much as we are now (and arguably should be). :-(

> > Same here for tabs.clientWidth.
> 
> should I do the same for `restoreTabsButtonWidth` property and field? though
> they only get run once?

Well... the reason I didn't suggest it there is that, as it is the label was set right before then, and so the button's width would not be affected by the label unless you forced a reflow (ie the non-flushing bounds width would return 0, or maybe the horizontal padding+border total if there is any).

Assuming that setting the label in the constructor is effective and the width of the button has been updated by the time this code runs (a guarantee which there might be, though I haven't checked - as the tabbrowser constructor runs when constructing the document, and IIRC session restore runs after delayedStartup (I think? Mike should be able to confirm/deny), which itself, again IIRC, runs after mozafterpaint, which would guarantee layout has happened, even if the button is visibility:hidden. Please check my memory is serving me well... 10pm on a Friday is maybe not the best time for this)

Comment 77

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

Thank you for the review! I'm new to both Python and Marionette, so I appreciate you pointing things out. I've asked for clarification on a few things though.

> Keep in mind that you are doing two unnecessary restarts between each of the tests. This takes a lot of extra time. By setting a class variable we could get around this.

Can you give me more detail on where these "two unnecessary restarts" are occuring? do you mean the restarts during tearDown?

> Better check for the correct windows and tabs being opened? Or don't we care about the order in the Marionette test, and is all that done in the Mochitest?

I agree, and I attempted to do so, following similar code from http://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py.
```
opened_windows = set()
    for win in windows:
        urls = tuple()
        for tab in win.tabbar.tabs:
            urls = urls + tuple([tab.location])
        opened_windows.add(urls)
```
 However, I was recieving an error when calling `tab.location`, `win.outerWindowID is undefined`. I figured I am not actually checking the functionality of session restore, but that the button activates it. So I assumed that there being more tabs would be enough - I'm happy to tackle this though if it's insufficient.

> This is bad. We should never do this in a test. Why is there no observer notification?

Agreed. There is no session saved notification. It was only used after mamy other options emplying `Wait` were exhausted. Do you know of another way I could notice that the session has been recorded?

Comment 78

[Jared Wein [:jaws] (please needinfo? me)]

I *think* sessionstore-state-write-complete is what you want? http://searchfox.org/mozilla-central/rev/507743376d1ba753d14ab6b9305b7c6358570be8/browser/components/sessionstore/SessionSaver.jsm#345

Comment 79

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> Please do not cross boundaries between Marionette and Firefox UI tests. The latter have their own testcase files under /testing/firefox-ui/resources

I was attempting to get files from layout, for example `self.marionette.absolute_url('layout/mozilla_projects.html')` but they all come back as a 404

Comment 80

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/2-3/

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> I was attempting to get files from layout, for example `self.marionette.absolute_url('layout/mozilla_projects.html')` but they all come back as a 404

It depends on how you are running this test. Did you use the `marionette` or `firefox-ui-functional` command?

> Can you give me more detail on where these "two unnecessary restarts" are occuring? do you mean the restarts during tearDown?

In `setUp` you are calling `enforce_gecko_prefs()` which will retart Firefox behind the scenes. Then you have another explicit call to `restart` at the end of `setUp`. Then in `tearDown` you are resetting the profile with a restart. 

All of those restarts happen for each and every test in this module, or your new class structure. But ideally you might only want to enforce the pref once, run all of the tests which rely on it, and then reset it.

Given that Marionette tests doesn't support the `setUpClass` and `tearDownClass` methods yet, it might not be that easy to get rid of. So for now you might want to move those tests which do not rely on this pref, to it's own class. And that shouldn't depend on the `TestBaseTabbarSessionRestoreButton`. Or you make a special class for the tests, which rely on that pref, and extend the base class for that. I would prefer the latter one.

> I agree, and I attempted to do so, following similar code from http://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py.
> ```
> opened_windows = set()
>     for win in windows:
>         urls = tuple()
>         for tab in win.tabbar.tabs:
>             urls = urls + tuple([tab.location])
>         opened_windows.add(urls)
> ```
>  However, I was recieving an error when calling `tab.location`, `win.outerWindowID is undefined`. I figured I am not actually checking the functionality of session restore, but that the button activates it. So I assumed that there being more tabs would be enough - I'm happy to tackle this though if it's insufficient.

If the button only appears when session store is enabled it should be fine. Also if the browser chrome test ensures that clicking the button, causes the right set of windows/tabs to be restored.

> Agreed. There is no session saved notification. It was only used after mamy other options emplying `Wait` were exhausted. Do you know of another way I could notice that the session has been recorded?

If there is no event nor observer notification, you might want to poll the Gecko API and check the session state? I cannot give a r+ with a sleep, sorry.

Comment 82

[:Gijs (he/him)]

(In reply to Erica from comment #77)
> > This is bad. We should never do this in a test. Why is there no observer notification?
> 
> Agreed. There is no session saved notification. It was only used after mamy
> other options emplying `Wait` were exhausted. Do you know of another way I
> could notice that the session has been recorded?

comment #78 seems to contain a useful suggestion. If that doesn't work for some reason, you could use a JS mutation observer to wait for whatever style/attribute to be set on the button in question.

Comment 83

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150154

Great to see progress happening here again, Erica!

I think Gijs and Henrik have already provided a good amount of feedback for you to make many improvements to this patch. Perhaps it's an idea to split the patch into two parts; 1. the browser code and 2. the test for Henrik to review separately. What do you think?

Please let me know if you'd like me to write down additional comments on this patch, instead of waiting for the next version.

Comment 84

[:Gijs (he/him)]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150148

Some more comments below - I think Henrik can help with the test.

::: browser/base/content/browser.js
(Diff revisions 2 - 3)
>          let {restoreTabsButtonWrapper} = gBrowser.tabContainer;
>          restoreTabsButtonWrapper.setAttribute("session-exists", "true");
> -        // After restoreTabsButtonWrapper's width has been measured in tabbrowser.xml,
> -        // we need to override the styling in tabs.inc.css that hides it.
> -        restoreTabsButtonWrapper.style.position = "initial";
> -        restoreTabsButtonWrapper.style.visibility = "visible";

Why did you remove the code that initially hides this item? It seems like having it be hidden initially is the right thing...

::: browser/base/content/browser.js:8083
(Diff revisions 2 - 3)
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
>  
> -  handleEvent: function(aEvent) {
> +  handleEvent: (aEvent) => {

Not sure why this change - did the linter pick something up and was this the fix you went with?

I think what you really want is:

```js
handleEvent(aEvent) {
  ...
}
```

and also `removeRestoreButton() {` - you don't need an arrow function here.

::: browser/base/content/tabbrowser.xml:5860
(Diff revisions 2 - 3)
>  
> -
>        <property name="restoreTabsButtonWidth" readonly="true">
>          <getter>
>            if (!this._restoreTabsButtonWidth) {
>              this._restoreTabsButtonWidth = this.restoreTabsButtonWrapper.clientWidth;

This still synchronously flushes the first time you access it. Was that unavoidable? I'm sorry if my long rambly tail-end of comment #76 was unhelpful - what I meant was, we should make this use windowUtils if we can, but I don't know for sure if that works. Did you test?

::: browser/base/content/tabbrowser.xml:5869
(Diff revisions 2 - 3)
>        </property>
>  
>        <method name="updateSessionRestoreVisibility">
>          <body><![CDATA[
> +          let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                              .getInterface(Ci.nsIDOMWindowUtils);

Nit: indentation - either 2-space indent, or align the `.` with the `.` on the previous line.

Comment 85

[Erica Wright [:ewright]]

Thanks everyone. I pushed version 3 last night, which included a fix for the `sleep(1)` line. It used the advice from comment #78, and a roundabout way to add an attribute that marionette can see.  
:mikedeboer It's ready for more feedback, as the unfixed issues remaining are waiting on other things.

Comment 86

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> It depends on how you are running this test. Did you use the `marionette` or `firefox-ui-functional` command?

oh, that's what the difference was, I was wondering. thank you!

> In `setUp` you are calling `enforce_gecko_prefs()` which will retart Firefox behind the scenes. Then you have another explicit call to `restart` at the end of `setUp`. Then in `tearDown` you are resetting the profile with a restart. 
> 
> All of those restarts happen for each and every test in this module, or your new class structure. But ideally you might only want to enforce the pref once, run all of the tests which rely on it, and then reset it.
> 
> Given that Marionette tests doesn't support the `setUpClass` and `tearDownClass` methods yet, it might not be that easy to get rid of. So for now you might want to move those tests which do not rely on this pref, to it's own class. And that shouldn't depend on the `TestBaseTabbarSessionRestoreButton`. Or you make a special class for the tests, which rely on that pref, and extend the base class for that. I would prefer the latter one.

In the `enforce_gecko_prefs` docs it says that it will only restart if the running instance does not have the given pref. 

For the one negative test, this means there will be no extra restart there as it defaults to negative. 

For the positive tests, every single test depends on both the session existing (coming from the restart in setUp), and the pref being True. So I don't see how any further rearranging could benefit it. 

I've thought about removing the restart in tearDown, because the tests all rely on a session anyway, but then it runs the risk of all of them failing if only one does.

> If the button only appears when session store is enabled it should be fine. Also if the browser chrome test ensures that clicking the button, causes the right set of windows/tabs to be restored.

There is no mochitest, and we've learned so far that it is impossible to test this sort of thing in mochitest. 
It seems that when restarting the browser instance something does not properly get reset on the new window.

Comment 87

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150148

> Why did you remove the code that initially hides this item? It seems like having it be hidden initially is the right thing...

`updateSessionRestoreVisibility` gets called when the browser starts up (I believe a resize event must be in there somewhere during restart). And since it's being told in `updateSessionRestoreVisibility` whether it should show or hide it seemed incorrect to have other attributes which also show/hide it in other places. 
Even if I default the css styles to hide it, that spot is the incorrect spot to remove that styling, since it only knows that there is a session, it does not know if there is room for the button, and both must but true for it to show.
I've added in a new rule in the css to do this since there was a flicker when the screen is too small.

> Not sure why this change - did the linter pick something up and was this the fix you went with?
> 
> I think what you really want is:
> 
> ```js
> handleEvent(aEvent) {
>   ...
> }
> ```
> 
> and also `removeRestoreButton() {` - you don't need an arrow function here.

that is exaclty what happened ;)

> This still synchronously flushes the first time you access it. Was that unavoidable? I'm sorry if my long rambly tail-end of comment #76 was unhelpful - what I meant was, we should make this use windowUtils if we can, but I don't know for sure if that works. Did you test?

Implemented it now, I interpreted from before that it was irrelevant so I didn't change it.

Comment 88

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/3-4/

Comment 89

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> In the `enforce_gecko_prefs` docs it says that it will only restart if the running instance does not have the given pref. 
> 
> For the one negative test, this means there will be no extra restart there as it defaults to negative. 
> 
> For the positive tests, every single test depends on both the session existing (coming from the restart in setUp), and the pref being True. So I don't see how any further rearranging could benefit it. 
> 
> I've thought about removing the restart in tearDown, because the tests all rely on a session anyway, but then it runs the risk of all of them failing if only one does.

In `tearDown` you are resetting the profile, which means that also the preferences will be rest to the default ones. So the changed one will not be present anymore. Also you cannot remove this extra restart in `tearDown` because you have to make sure that default values are getting restored for the next tests. It means we might have to live with the longer test duration.

> There is no mochitest, and we've learned so far that it is impossible to test this sort of thing in mochitest. 
> It seems that when restarting the browser instance something does not properly get reset on the new window.

In that case it would be really helpful to check for the correct windows and tabs. Maybe have a look at the other sessionrestore test and how it's done there. Regarding the mentioned issue, did you wait long enough until all tabs have been restored after clicking the button? I assume there is a observer notification for that?

Comment 90

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150652

::: testing/firefox-ui/tests/functional/sessionstore/manifest.ini:6
(Diff revision 4)
>  [DEFAULT]
>  tags = local
>  
> +[test_tabbar_session_restore_button.py]
>  [test_restore_windows_after_restart.py]
>  skip-if = (os == "win" || e10s) # Bug 1291844 and Bug 1228446

Please keep in mind that we faced issues with sessionrestore in the other test on Windows and e10s turned on. Maybe this is what you are also facing here? If yes, we would also have to use this skip line.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:77
(Diff revision 4)
> +        super(TestTabbarSessionRestoreButton, self).setUp()
> +
> +    def test_session_exists(self):
> +        restore_tabs_button_wrapper = (
> +                self.puppeteer.windows.current.tabbar.element
> +                .find_element('anon attribute', {'anonid': 'restore-tabs-button-wrapper'})

Don't use `find_element` in the test directly. Instead create a new property for each of the elements you need in the TabBar class:

https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/tabbar.py#17

See the `newtab_button` for example.

Comment 91

[:Gijs (he/him)]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150660

I'm sorry about how many iterations this is taking. :-(

In terms of approach, I think ideally we should ensure that we don't listen for paint events until we have the session, and that we listen for paint events only once (with {once: true} passed to addEventListener), if/when we need to. So I don't think the current patch (which adds a permanent MozAfterPaint listener - at least, I don't see it getting removed?) is OK.

I'm assuming you found that you needed the paint events in order to ensure that layout had happened and the button had a non-0 width for the computations in updateSessionRestoreVisibility? I don't know if this was needed both for the resize handling or also for the initial showing after the session is restored. More generally I'm a bit confused about how this currently works / how best to implement it, as a result. It looks like visibility:collapse will make the width 0, which would throw off the calculations until the element is visible, but if we wait for a paint in that case, it'll flash.

Perhaps we can discuss over IRC/vidyo later today to work out how best to get this shipped?

::: browser/base/content/tabbrowser.xml:5802
(Diff revisions 3 - 4)
> +          this.windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                   .getInterface(Ci.nsIDOMWindowUtils);

Ideally this should be a lazy getter. You can easily define one using XPCOMUtils.defineLazyGetter. Ideally we wouldn't start using this until after the session restore observer has fired, and until after a paint has happened, so we can avoid instantiating it until then.

::: browser/base/content/browser.js:8087
(Diff revision 4)
>  
> +  handleEvent(aEvent) {
> +    switch (aEvent.type) {
> +     case "TabOpen":
> +        this.removeRestoreButton();
> +      }

Nit: brace should align with the start of the switch statement (tabs instead of spaces issue, maybe?)

::: browser/base/content/tabbrowser.xml:5856
(Diff revision 4)
> +      <field name="_restoreTabsButtonWidth">
> +        this.windowUtils.getBoundsWithoutFlushing(this.restoreTabsButtonWrapper).width
> +      </field>

This looks wrong - when this runs, it'll always return 0, I expect, and then we overwrite it from the `restoreTabsButtonWidth` property anyway...

::: browser/base/content/tabbrowser.xml:5875
(Diff revision 4)
> +        <body><![CDATA[
> +          let shouldHide = true;
> +          let { restoreTabsButtonWrapper } = gBrowser.tabContainer;
> +
> +          if (!restoreTabsButtonWrapper.getAttribute("session-exists")) {
> +            gBrowser.tabContainer.removeEventListener("TabOpen", this);

I think this is a leftover? There's no TabOpen listener in this bit of code - it lives in the .js file bits.

::: browser/base/content/tabbrowser.xml:5876
(Diff revision 4)
> +          let shouldHide = true;
> +          let { restoreTabsButtonWrapper } = gBrowser.tabContainer;
> +
> +          if (!restoreTabsButtonWrapper.getAttribute("session-exists")) {
> +            gBrowser.tabContainer.removeEventListener("TabOpen", this);
> +            restoreTabsButtonWrapper.hidden = true;

Does the resize event run this code immediately and hide the button when it's not initially hidden=true? Because that would be unfortunate... ideally the resize handler shouldn't call this code until/unless the wrapper has the session-exists attribute, right? Perhaps a check for this at the callsite would be the easiest.

::: browser/base/content/tabbrowser.xml:5886
(Diff revision 4)
> +          if (restoreTabsButtonWidth > 0) {
> +            window.addEventListener("MozAfterPaint", this);
> +          }

This adds the listener a second time. Maybe this is an oversight?

Separately, should we continue running the code here if the width is 0? I expect it won't make the right decisions.

::: browser/base/content/tabbrowser.xml:5902
(Diff revision 4)
> +          // the restoreTabsButton won't cause any overflow.
> +          if ((this.windowUtils.getBoundsWithoutFlushing(tabs).width - tabbarUsedSpace) > restoreTabsButtonWidth) {
> +            shouldHide = false;
> +          }
> +
> +          restoreTabsButtonWrapper.hidden = shouldHide;

Instead of hiding it, should we set it back to visibility: hidden ? That will ensure (I think!) that the bounds of the button will still be available even when it's collapsed. You could set overflow:hidden on the wrapper as well, to ensure that it won't affect the rest of the tabstrip, but you would still be able to read the width of the button inside the overflow:hidden container.

Comment 92

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150798

Thanks again, Erica, for picking this back up! Your reviewers might come across as a bit harsh or nit-picky, but that's simply because we want the end result to be as awesome as possible. Just like you, right? :-)

I'm looking forward to the next iteration.

::: browser/base/content/tabbrowser.xml:5789
(Diff revision 4)
>                             onmouseover="document.getBindingParent(this)._enterNewTab();"
>                             onmouseout="document.getBindingParent(this)._leaveNewTab();"
>                             tooltip="dynamic-shortcut-tooltip"/>
> +        <xul:hbox class="restore-tabs-button-wrapper"
> +                  anonid="restore-tabs-button-wrapper">
> +          <xul:toolbarbutton anonid="restore-tabs-button"

Why do you need the wrapper here? Regardless, I'd recommend to keep the anonid on the button itself and reference the wrapper through `button.parentNode`. That'd also make the browser.js code a bit more easy to read.

::: browser/base/content/tabbrowser.xml:5816
(Diff revision 4)
>            tab.label = this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle");
>            tab.setAttribute("onerror", "this.removeAttribute('image');");
>  
>            window.addEventListener("resize", this);
>            window.addEventListener("load", this);
> +          window.addEventListener("MozAfterPaint", this);

We can't really do this, because MozAfterPaint is called _a lot_! So you'll want to throttle this or have another, more precise event trigger the resize.

::: browser/base/content/tabbrowser.xml:6434
(Diff revision 4)
>              case "mousemove":
>                if (document.getElementById("tabContextMenu").state != "open")
>                  this._unlockTabSizing();
>                break;
> +            case "MozAfterPaint":
> +              this.updateSessionRestoreVisibility();

So this will be called for _every_ paint of the browser window, which is basically _all the time_. Can you see why?
So it's probably best to start with more deterministic callsites, like `adjustTabstrip`, which  may be even the right place to put a call to `updateSessionRestoreVisibility`.
Nit: can we change that name to `updateSessionRestoreButton`? I just had difficulty just now to write 'visibility', but that's prolly my own problem :P And the method does a bit more than just determine the showing state of the button.
Your call, of course.

::: browser/themes/linux/browser.css:1016
(Diff revision 4)
>    background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 96, 16, 80);
>  }
>  
> +/* Restore tabs button */
> +.restore-tabs-button {
> +  background-color: ThreeDFace;

I think you didn't write this, but it'd be awesome to have a comment here that explains _why_ this is needed on Linux only.
Because the comment right now just repeats the className, which is, well, hum, a bit unhelpful.

::: browser/themes/shared/tabs.inc.css:600
(Diff revision 4)
>  }
> +
> +.restore-tabs-button-wrapper {
> +  visibility: collapse;
> +  padding: 4px;
> +  margin-inline-start: 3px;

I don't see why these can't be on the button itself...

::: browser/themes/shared/tabs.inc.css:610
(Diff revision 4)
> +}
> +
> +.restore-tabs-button {
> +  -moz-appearance: none;
> +  border: 1px solid hsla(0,0%,16%,.2);
> +  border-radius: 15px;

If you want the button pill-shaped, the convention is to use `10000px`. If you want it square, but with proportionally rounded corners, use `.5em` or another `em` unit that you think looks great.

::: browser/themes/shared/tabs.inc.css:628
(Diff revision 4)
> +.restore-tabs-button > .toolbarbutton-icon {
> +  display: none;
> +}
> +
> +.restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;

My guess is that you can remove this statement.

Comment 93

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150660

> This adds the listener a second time. Maybe this is an oversight?
> 
> Separately, should we continue running the code here if the width is 0? I expect it won't make the right decisions.

yes, that was a typo, supposed to be `removeEventListener`

Comment 94

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150798

> Why do you need the wrapper here? Regardless, I'd recommend to keep the anonid on the button itself and reference the wrapper through `button.parentNode`. That'd also make the browser.js code a bit more easy to read.

I attempted to remove the wrapper. But I've changed the strategy on how to hide the button, and am now using `position: fixed`. it seems that a `xul:toolbarbutton` won't behave correctly with `position: fixed` though, so I'm keeping the wrapper around for that.

> My guess is that you can remove this statement.

this is needed for the label to show, since it is a toolbarbutton "Restore Tabs from Last Time"

Comment 95

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review150660

> Does the resize event run this code immediately and hide the button when it's not initially hidden=true? Because that would be unfortunate... ideally the resize handler shouldn't call this code until/unless the wrapper has the session-exists attribute, right? Perhaps a check for this at the callsite would be the easiest.

since it's called in more than one place, I think the check for the session-exists attribute is a bit better where it is in the `updateSessionRestoreVisibility` function itself.

Comment 96

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/4-5/

Comment 97

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review151272

Getting ever closer! Let's nail it for the next iteration.

::: browser/app/profile/firefox.js:70
(Diff revision 5)
>  
>  // Disable screenshots for now, Shield will enable this.
>  pref("extensions.screenshots.system-disabled", true);
>  
> +// Disable the tabbar session restore button
> +pref("browser.tabs.restorebutton", false);

I did not notice this before, but can you please put this pref next to the other 'browser.tabs.' ones?

::: browser/base/content/tabbrowser.xml:5787
(Diff revision 5)
>                             command="cmd_newNavigatorTab"
>                             onclick="checkForMiddleClick(this, event);"
>                             onmouseover="document.getBindingParent(this)._enterNewTab();"
>                             onmouseout="document.getBindingParent(this)._leaveNewTab();"
>                             tooltip="dynamic-shortcut-tooltip"/>
> +        <xul:hbox class="restore-tabs-button-wrapper"

I tried removing this wrapper locally and using the button throughout the code (JS and CSS) locally and everything worked just dandy-fine.
So I'd really like to know what snag you hit and perhaps work through it with you on IRC/ Slack or something else you're comfortable with?

::: browser/base/content/tabbrowser.xml:5858
(Diff revision 5)
>        <field name="_afterHoveredTab">null</field>
>        <field name="_hoveredTab">null</field>
> +      <field name="restoreTabsButton">
> +        document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button");
> +      </field>
> +      <field name="_restoreTabsButtonWidth">0</field>

nit: initializing to `null` makes it follow the same pattern as above, which is mildly better.

::: browser/base/content/tabbrowser.xml:5860
(Diff revision 5)
> +      <field name="restoreTabsButton">
> +        document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button");
> +      </field>
> +      <field name="_restoreTabsButtonWidth">0</field>
> +
> +      <property name="restoreTabsButtonWidth" readonly="true">

You forgot to change the name of this method.

::: browser/base/content/tabbrowser.xml:6423
(Diff revision 5)
>                var width = this.mTabstrip.boxObject.width;
>                if (width != this.mTabstripWidth) {
>                  this.adjustTabstrip();
>                  this._handleTabSelect(false);
>                  this.mTabstripWidth = width;
> +                this.updateSessionRestoreVisibility();

Cool! The sweat has disappeared from my forehead ;-)

::: browser/themes/linux/browser.css:1015
(Diff revision 5)
>  .tab-close-button:not(:hover):-moz-lwtheme-darktext {
>    background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 96, 16, 80);
>  }
>  
> +/* Restore tabs button */
> +.restore-tabs-button {

You forgot to update the comment here.

::: browser/themes/shared/tabs.inc.css:598
(Diff revision 5)
>  .alltabs-endimage[blocked] {
>    list-style-image: url(chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-blocked);
>  }
> +
> +.restore-tabs-button-wrapper {
> +  visibility: hidden;

So at the moment, this button doesn't have any styling for the hover or active states. You will get this for 'free' by adding the 'toolbarbutton-1' className to the toolbarbutton.
You will need to update the button selectors here a bit due to precendence rules, but that's certainly worth it. You can also get rid of the `-moz-appearance: none` rule below, because it'll inherit that too.

::: browser/themes/shared/tabs.inc.css:599
(Diff revision 5)
>    list-style-image: url(chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-blocked);
>  }
> +
> +.restore-tabs-button-wrapper {
> +  visibility: hidden;
> +  position: fixed; /* so the button does not take up actual space and cause overflow buttons in the tab bar when hidden */

well, I *think* this is because you're using `visibility: hidden` and not `visibility: collapsed`. So can you please try that instead of introducing another positioning method in the tabstrip?

Comment 98

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #97)
> ::: browser/base/content/tabbrowser.xml:5787
> (Diff revision 5)
> >                             command="cmd_newNavigatorTab"
> >                             onclick="checkForMiddleClick(this, event);"
> >                             onmouseover="document.getBindingParent(this)._enterNewTab();"
> >                             onmouseout="document.getBindingParent(this)._leaveNewTab();"
> >                             tooltip="dynamic-shortcut-tooltip"/>
> > +        <xul:hbox class="restore-tabs-button-wrapper"
> 
> I tried removing this wrapper locally and using the button throughout the
> code (JS and CSS) locally and everything worked just dandy-fine.
> So I'd really like to know what snag you hit and perhaps work through it
> with you on IRC/ Slack or something else you're comfortable with?

I haven't had a chance to look yet, but I don't see how you could accomplish the following things without an extra wrapper, and I think the discussion Erica, Blake and I had about this yesterday is why the wrapper is still there:

* ensure the button disappears when the window is narrow
* ensure we can measure the button for that purpose, without a layout flush
* ensure we don't show the button for 1 frame and then hide it the next (which is what would happen if you don't hide the button somehow).

> ::: browser/base/content/tabbrowser.xml:5858
> (Diff revision 5)
> >        <field name="_afterHoveredTab">null</field>
> >        <field name="_hoveredTab">null</field>
> > +      <field name="restoreTabsButton">
> > +        document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button");
> > +      </field>
> > +      <field name="_restoreTabsButtonWidth">0</field>
> 
> nit: initializing to `null` makes it follow the same pattern as above, which
> is mildly better.

Well, it's an int, and we set it to another int later, so I think 0 is correct. This got talked about this on Vidyo yesterday...

> ::: browser/base/content/tabbrowser.xml:5860
> (Diff revision 5)
> > +      <field name="restoreTabsButton">
> > +        document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button");
> > +      </field>
> > +      <field name="_restoreTabsButtonWidth">0</field>
> > +
> > +      <property name="restoreTabsButtonWidth" readonly="true">
> 
> You forgot to change the name of this method.

This isn't a method. I also don't see earlier comments about updating this...

> ::: browser/themes/shared/tabs.inc.css:599
> (Diff revision 5)
> >    list-style-image: url(chrome://browser/skin/tabbrowser/tab-audio.svg#tab-audio-blocked);
> >  }
> > +
> > +.restore-tabs-button-wrapper {
> > +  visibility: hidden;
> > +  position: fixed; /* so the button does not take up actual space and cause overflow buttons in the tab bar when hidden */
> 
> well, I *think* this is because you're using `visibility: hidden` and not
> `visibility: collapsed`. So can you please try that instead of introducing
> another positioning method in the tabstrip?

It was visibility:collapsed in the previous versions of the patch, and that still gives 0 width from getBoundsWithoutFlushing.

Comment 99

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #97)
> So at the moment, this button doesn't have any styling for the hover or
> active states. You will get this for 'free' by adding the 'toolbarbutton-1'
> className to the toolbarbutton.
> You will need to update the button selectors here a bit due to precendence
> rules, but that's certainly worth it. You can also get rid of the
> `-moz-appearance: none` rule below, because it'll inherit that too.

Use toolbarbutton-1 only if you want to give up control over the button styling. It looks like you don't want that here, so don't use toolbarbutton-1.

Comment 100

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [::dao] from comment #99)
> Use toolbarbutton-1 only if you want to give up control over the button
> styling. It looks like you don't want that here, so don't use
> toolbarbutton-1.

Why is adding toobarbutton-1 like 'giving up all styling'?
I was hoping for conformant styling that it'll look, feel and work like a Firefox button, not some kind of alien spaceship. So adding a visible border and rounded corner seemed OK to me.

Comment 101

[Dão Gottwald [:dao]]

<dao> mikedeboer: the problem is that when adding custom stuff to our toolbarbutton-1 styling, you rely on specifics of that styling. e.g. what you proposed might currently work (although I'm not even sure it does) only because we implement toolbarbutton-1 differently in the tabs toolbar than in other toolbars
I don't want to get into a situation where changing toolbarbutton-1 requires fixing up various UI pieces that wanted to pick up stuff from toolbarbutton-1 "for free"
<mikedeboer> dao: well, that's kinda the point as well; it needs to look like it's an integral part of the toolbar and indeed adding toolbarbutton-1 seems to be doing that right now. I don't know of a better way at all other than adding the button class to the list of other selectors that define the hover and active styling of buttons in the tabbar. 
<dao> you basically just want a background on hover, right?
<mikedeboer> And that's prolly because of my ignorance and/ or lazyness
<dao> var(--toolbarbutton-hover-background) is your friend then
<mikedeboer> well, yes that and :hover:active, and perhaps text-shadow and other goodies that a toolbar button needs in the tabstrip
<dao> there's --toolbarbutton-active-background too
I don't think you want the box-shadow since you already set a border
what other goodies?
<Gijs> padding changes for compact mode? rounded corners that fit with all the other buttons?
I think arguably the button *should* be styled like other .toolbarbutton-1 items in the tabstrip
<Gijs> we don't show it when the tabstrip overflows, or when there's no space
and it has text rather than an icon
but otherwise it's just a button
<dao> the toolbarbutton-1 styling in the tabs toolbar actually assumes that these buttons are outside of the tab strip and glues them to the bottom toolbar border
I don't think you want that here
<Gijs> I'm not sure - there's no design :)
<dao> well, there's https://bug1219725.bmoattachments.org/attachment.cgi?id=8766890

tl;dr: figure out the intended look. if you want this button to look like a regular one, use toolbarbutton-1. if you want custom look and just pick up the hover and active backgrounds, use var(--toolbarbutton-hover-background) and var(--toolbarbutton-active-background)

Comment 102

[:Gijs (he/him)]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review151280

This is effectively r=me on the JS/XBL, with some nits below to tighten up various bits.

Otherwise, did you hear back from shorlander on the hover/active styling? I haven't actually poked at this in practice yet, but barring any styling issues I'm pretty happy.

::: browser/base/content/browser.js:8096
(Diff revision 5)
> +
> +  removeRestoreButton() {
> +    let {restoreTabsButton} = gBrowser.tabContainer;
> +    let restoreTabsButtonWrapper = restoreTabsButton.parentNode;
> +    restoreTabsButtonWrapper.removeAttribute("session-exists");
> +    gBrowser.tabContainer.addEventListener("transitionend", function maxWidthTransitionnHandler(e) {

Nit: '...itionnHandler' should be 'itionHandler'. :-)

(Make sure to update the removeEventListener as well...)

::: browser/base/content/browser.js:8097
(Diff revision 5)
> +      if (e.propertyName == "max-width") {
> +        window.gBrowser.tabContainer.updateSessionRestoreVisibility();

You shouldn't need the explicit `window` here.

::: browser/base/content/tabbrowser.xml:5885
(Diff revision 5)
> +          let tabs = document.getElementById("tabbrowser-tabs");
> +
> +          // There will only ever be one tab when the restore button is showing.
> +          let firstTab = tabs.firstChild;

Nit: I think this can be replaced with `let firstTab = this._firstTab` - or omitted, and you can pass `this._firstTab` to `getBoundsWithoutFlushing` directly.

::: browser/base/content/tabbrowser.xml:5891
(Diff revision 5)
> +
> +          // There will only ever be one tab when the restore button is showing.
> +          let firstTab = tabs.firstChild;
> +
> +          // Get the widths of the elements in the tabs area
> +          let tabbarUsedSpace = this.windowUtils.getBoundsWithoutFlushing(firstTab).width + this.windowUtils.getBoundsWithoutFlushing(newTabButton).width

Nit: please split and wrap this appropriately.

::: browser/base/content/tabbrowser.xml:5895
(Diff revision 5)
> +          if ((this.windowUtils.getBoundsWithoutFlushing(tabs).width - tabbarUsedSpace) > restoreTabsButtonWidth) {
> +            shouldHide = false;
> +          }

Nit:

Can we use this.mTabstripWidth here instead of getting the bounds? If not, please put the width in a variable here.

Omit the shouldHide declaration earlier (as it's not used up to here anyway). Then:

```js
let shouldHide = restoreTabsButtonWidth > tabstripWidth - tabbarUsedSpace;
```

and in fact, maybe that can go straight into the condition for the `if (...) {}` block lower down.

::: browser/themes/shared/tabs.inc.css:613
(Diff revision 5)
> +  max-width: 25em; /* The button will likely never reach this width, but it is specified here for the transition */
> +  transition: max-width 300ms;
> +}
> +
> +.restore-tabs-button:-moz-lwtheme-darktext {
> +  border-color: hsla(0,0%,16%,.2);

This is the same as no lwtheme - do we need this rule?

Comment 103

[Dão Gottwald [:dao]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review151298

::: browser/base/content/browser.js:8087
(Diff revision 5)
>    },
>  
> +  handleEvent(aEvent) {
> +    switch (aEvent.type) {
> +     case "TabOpen":
> +        this.removeRestoreButton();

Has it been taken into account that this will remove the button when Firefox automatically opens a second tab such as post-update "what's new" pages, the privacy policy (when the policy changes), etc.. This will surprise people once they got used to the button.

::: browser/base/content/tabbrowser.xml:5802
(Diff revision 5)
>      </content>
>  
>      <implementation implements="nsIDOMEventListener, nsIObserver">
>        <constructor>
>          <![CDATA[
> +          XPCOMUtils.defineLazyGetter(

use <field> instead of XPCOMUtils.defineLazyGetter

::: browser/base/content/tabbrowser.xml:5810
(Diff revision 5)
> +              return window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +          });
> +
>            this.mTabClipWidth = Services.prefs.getIntPref("browser.tabs.tabClipWidth");
>  
> +          let { restoreTabsButton } = gBrowser.tabContainer;

s/gBrowser.tabContainer/this/

::: browser/base/content/tabbrowser.xml:5856
(Diff revision 5)
>        <field name="_afterSelectedTab">null</field>
>        <field name="_beforeHoveredTab">null</field>
>        <field name="_afterHoveredTab">null</field>
>        <field name="_hoveredTab">null</field>
> +      <field name="restoreTabsButton">
> +        document.getAnonymousElementByAttribute(gBrowser.tabContainer, "anonid", "restore-tabs-button");

s/gBrowser.tabContainer/this/

::: browser/base/content/tabbrowser.xml:5872
(Diff revision 5)
> +      </property>
> +
> +      <method name="updateSessionRestoreVisibility">
> +        <body><![CDATA[
> +          let shouldHide = true;
> +          let { restoreTabsButton } = gBrowser.tabContainer;

s/gBrowser.tabContainer/this/

(same again below, not gonna repeat it)

::: browser/base/content/tabbrowser.xml:5876
(Diff revision 5)
> +          let shouldHide = true;
> +          let { restoreTabsButton } = gBrowser.tabContainer;
> +          let restoreTabsButtonWrapper = restoreTabsButton.parentNode;
> +
> +          if (!restoreTabsButtonWrapper.getAttribute("session-exists")) {
> +            restoreTabsButtonWrapper.style.visibility = "hidden";

Can you set an attribute on the button / wrapper and move all direct style modifications to the stylesheet?

::: browser/base/content/tabbrowser.xml:5885
(Diff revision 5)
> +
> +          let newTabButton = document.getAnonymousElementByAttribute(
> +            gBrowser.tabContainer, "anonid", "tabs-newtab-button");
> +
> +          let {restoreTabsButtonWidth} = gBrowser.tabContainer;
> +          let tabs = document.getElementById("tabbrowser-tabs");

document.getElementById("tabbrowser-tabs") is the same as 'this'

::: browser/base/content/tabbrowser.xml:5891
(Diff revision 5)
> +
> +          // There will only ever be one tab when the restore button is showing.
> +          let firstTab = tabs.firstChild;
> +
> +          // Get the widths of the elements in the tabs area
> +          let tabbarUsedSpace = this.windowUtils.getBoundsWithoutFlushing(firstTab).width + this.windowUtils.getBoundsWithoutFlushing(newTabButton).width

nit: semicolon, and please break up this line

::: browser/themes/linux/browser.css:1017
(Diff revision 5)
>  }
>  
> +/* Restore tabs button */
> +.restore-tabs-button {
> +  background-color: ThreeDFace;
> +  color: ButtonText;

You shouldn't need this rule. Bug 1368672 may have fixed the issue you were working around?

::: browser/themes/shared/tabs.inc.css:608
(Diff revision 5)
> +  -moz-appearance: none;
> +  border: 1px solid hsla(0,0%,16%,.2);
> +  border-radius: 10000px;
> +  margin: 4px;
> +  margin-inline-start: 7px;
> +  max-width: 25em; /* The button will likely never reach this width, but it is specified here for the transition */

Considering localization, this comment is very likely already mistaken.

::: browser/themes/shared/tabs.inc.css:613
(Diff revision 5)
> +  max-width: 25em; /* The button will likely never reach this width, but it is specified here for the transition */
> +  transition: max-width 300ms;
> +}
> +
> +.restore-tabs-button:-moz-lwtheme-darktext {
> +  border-color: hsla(0,0%,16%,.2);

Please drop this since you already set the same color above.

::: browser/themes/shared/tabs.inc.css:616
(Diff revision 5)
> +
> +.restore-tabs-button:-moz-lwtheme-darktext {
> +  border-color: hsla(0,0%,16%,.2);
> +}
> +
> +.restore-tabs-button:-moz-lwtheme-brighttext {

Please use #TabsToolbar[brighttext] instead of :-moz-lwtheme-brighttext to support dark OS themes

::: browser/themes/shared/tabs.inc.css:626
(Diff revision 5)
> +  display: none;
> +}
> +
> +.restore-tabs-button > .toolbarbutton-text {
> +  display: -moz-box;
> +  margin-bottom: 0;

What's the point of margin-bottom: 0 here? Are you overriding toolbarbutton.css? Looks like that already sets all margins to 0 on all platforms.

Comment 104

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review151272

> I did not notice this before, but can you please put this pref next to the other 'browser.tabs.' ones?

Sure, I added it next to the screenshots on 'cause I figured similarities with shield testing.

> You forgot to update the comment here.

Yes, I'll get around to the styling specifics once I have some more concrete images to work off of.

Comment 105

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #103)
> ::: browser/base/content/tabbrowser.xml:5802
> (Diff revision 5)
> >      </content>
> >  
> >      <implementation implements="nsIDOMEventListener, nsIObserver">
> >        <constructor>
> >          <![CDATA[
> > +          XPCOMUtils.defineLazyGetter(
> 
> use <field> instead of XPCOMUtils.defineLazyGetter

Why? That will initialize immediately, which seems like a bad idea.

Comment 106

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #105)
> (In reply to Dão Gottwald [::dao] from comment #103)
> > ::: browser/base/content/tabbrowser.xml:5802
> > (Diff revision 5)
> > >      </content>
> > >  
> > >      <implementation implements="nsIDOMEventListener, nsIObserver">
> > >        <constructor>
> > >          <![CDATA[
> > > +          XPCOMUtils.defineLazyGetter(
> > 
> > use <field> instead of XPCOMUtils.defineLazyGetter
> 
> Why? That will initialize immediately, which seems like a bad idea.

Nope, XBL fields are lazy.

Comment 107

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #106)
> Nope, XBL fields are lazy.

Welp, TIL.

Comment 108

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review151298

> Has it been taken into account that this will remove the button when Firefox automatically opens a second tab such as post-update "what's new" pages, the privacy policy (when the policy changes), etc.. This will surprise people once they got used to the button.

My understanding was that that was not a concern. The button is fairly ephemeral already, so I don't think it will be missed. Perhaps best to ask :shorlander about this?

> Considering localization, this comment is very likely already mistaken.

One other idea from bwinton is to set the max-width in the constructor as whatever the current width is. Would that be acceptable? or does anyone have any suggestions?

Comment 109

[Dão Gottwald [:dao]]

(In reply to Erica from comment #108)
> Comment on attachment 8873568 [details]
> Bug 1219725 - Add a button for session restore to the tab bar. 
> ui-r=shorlander
> 
> https://reviewboard.mozilla.org/r/143224/#review151298
> 
> > Has it been taken into account that this will remove the button when Firefox automatically opens a second tab such as post-update "what's new" pages, the privacy policy (when the policy changes), etc.. This will surprise people once they got used to the button.
> 
> My understanding was that that was not a concern. The button is fairly
> ephemeral already, so I don't think it will be missed.

I don't understand what you mean here. In said situation the user wouldn't get a chance to see the button at all unless I misread your patch.

> Perhaps best to ask :shorlander about this?
> 
> > Considering localization, this comment is very likely already mistaken.
> 
> One other idea from bwinton is to set the max-width in the constructor as
> whatever the current width is. Would that be acceptable? or does anyone have
> any suggestions?

Could you set the max-width just before you start the animation?

Related to that, how well does your patch cope with long strings in general?

Comment 110

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> In that case it would be really helpful to check for the correct windows and tabs. Maybe have a look at the other sessionrestore test and how it's done there. Regarding the mentioned issue, did you wait long enough until all tabs have been restored after clicking the button? I assume there is a observer notification for that?

It would be helpful, but since the win.outerWindowID is undefined, we can't. It doesn't seem to have to do with waiting, as even in a debugger it remains undefined. I believe this is a bug in marionette/puppeteer.

I have been comparing it to `/test_restore_windows_after_restart.py` (which is the only other session restore test) which calls `self.restart()`, this actually opens the same browser window since `clean=True` has not been declared. I believe, therefore, that that test is actually invalid. I tested this by forcing the pref `'browser.startup.page': 1` and the test still continues to pass - when it should have failed.

Comment 111

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/5-6/

Comment 112

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149192

> This was leftover from the old patch https://reviewboard.mozilla.org/r/72698/diff/3/. I haven't tested it on linux yet nor on anything other than mac.
> I'll add it to my TODOs

I haven't touched any of the styling, but I think now that it's closer i am ready to start those fixes. can you push it to try for me, I'm unable to?

Comment 113

[:Gijs (he/him)]

(In reply to Erica from comment #112)
> Comment on attachment 8873568 [details]
> Bug 1219725 - Add a button for session restore to the tab bar. 
> ui-r=shorlander
> 
> https://reviewboard.mozilla.org/r/143224/#review149192
> 
> > This was leftover from the old patch https://reviewboard.mozilla.org/r/72698/diff/3/. I haven't tested it on linux yet nor on anything other than mac.
> > I'll add it to my TODOs
> 
> I haven't touched any of the styling, but I think now that it's closer i am
> ready to start those fixes. can you push it to try for me, I'm unable to?

Pushed. I'll try to get to the review either tomorrow or on Monday. :-)

Comment 114

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> It would be helpful, but since the win.outerWindowID is undefined, we can't. It doesn't seem to have to do with waiting, as even in a debugger it remains undefined. I believe this is a bug in marionette/puppeteer.
> 
> I have been comparing it to `/test_restore_windows_after_restart.py` (which is the only other session restore test) which calls `self.restart()`, this actually opens the same browser window since `clean=True` has not been declared. I believe, therefore, that that test is actually invalid. I tested this by forcing the pref `'browser.startup.page': 1` and the test still continues to pass - when it should have failed.

> It would be helpful, but since the win.outerWindowID is undefined, we can't. It doesn't seem to have to do with waiting, as even in a debugger it remains undefined. I believe this is a bug in marionette/puppeteer.

Would you mind to pastebin a trace log for this failure? You would have to run marionette with `-vv` and then check gecko.log as usual. I would need the Python and JS traceback. It could give me an indication what's wrong. Thanks.

> test is actually invalid. I tested this by forcing the pref 'browser.startup.page': 1 and the test still continues to pass - when it should have failed.

Can you please check the value of this preference after the restart? Is it really set to `1` or has it been reset to `3`. I would assume it's the latter, because otherwise Firefox would not restore the former session.

Comment 115

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #109)
> (In reply to Erica from comment #108)
> > Comment on attachment 8873568 [details]
> > Bug 1219725 - Add a button for session restore to the tab bar. 
> > ui-r=shorlander
> > 
> > https://reviewboard.mozilla.org/r/143224/#review151298
> > 
> > > Has it been taken into account that this will remove the button when Firefox automatically opens a second tab such as post-update "what's new" pages, the privacy policy (when the policy changes), etc.. This will surprise people once they got used to the button.
> > 
> > My understanding was that that was not a concern. The button is fairly
> > ephemeral already, so I don't think it will be missed.
> 
> I don't understand what you mean here. In said situation the user wouldn't
> get a chance to see the button at all unless I misread your patch.

Whether this is correct depends on whether session init finishes before or after these tabs have been opened.

I agree that this might be a concern, but I also think this can wait for a followup bug, especially now this has missed 55.

Comment 116

[:Gijs (he/him)]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review152428

The JS/logic looks fine to me. The styling probably wants another look by someone (doesn't need to be me) when it's finished (I'm assuming it isn't in this patch).

Comment 117

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> > It would be helpful, but since the win.outerWindowID is undefined, we can't. It doesn't seem to have to do with waiting, as even in a debugger it remains undefined. I believe this is a bug in marionette/puppeteer.
> 
> Would you mind to pastebin a trace log for this failure? You would have to run marionette with `-vv` and then check gecko.log as usual. I would need the Python and JS traceback. It could give me an indication what's wrong. Thanks.
> 
> > test is actually invalid. I tested this by forcing the pref 'browser.startup.page': 1 and the test still continues to pass - when it should have failed.
> 
> Can you please check the value of this preference after the restart? Is it really set to `1` or has it been reset to `3`. I would assume it's the latter, because otherwise Firefox would not restore the former session.

I checked the value of the pref before and after restarting in that particular test, and they both remain at 1. In that case I believe it is re-opening the same window, not actually restoring the window/tabs.

you can see an etherpad of the gecko.log here https://public.etherpad-mozilla.org/p/geckolog
line 568 seems to be the restart. 844 is the error.
There are calls to `getCurrentChromeWindowHandle` all over and they return the same number (3) before and after the restart.

Comment 118

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/6-7/

Comment 119

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review149272

> I checked the value of the pref before and after restarting in that particular test, and they both remain at 1. In that case I believe it is re-opening the same window, not actually restoring the window/tabs.
> 
> you can see an etherpad of the gecko.log here https://public.etherpad-mozilla.org/p/geckolog
> line 568 seems to be the restart. 844 is the error.
> There are calls to `getCurrentChromeWindowHandle` all over and they return the same number (3) before and after the restart.

Since this seems to be problematic, could we merge as is and fix it in a follow up bug, or separate this into two patches as suggested by someone else? Unless someone has other ideas on how to compare the opened tabs to the expected tabs?

Comment 120

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Erica from comment #117)
> you can see an etherpad of the gecko.log here
> https://public.etherpad-mozilla.org/p/geckolog
> line 568 seems to be the restart. 844 is the error.

Thank you for that trace output. It was helpful and showed me that bug 1351940 is still present. I will have a look at it today and hope that I can reproduce the problem with your patch / test.

> There are calls to `getCurrentChromeWindowHandle` all over and they return
> the same number (3) before and after the restart.

This is fine. `3` should always be outer window id of the first chrome window which gets opened.

Comment 121

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Erica from comment #119)
> Since this seems to be problematic, could we merge as is and fix it in a
> follow up bug, or separate this into two patches as suggested by someone
> else? Unless someone has other ideas on how to compare the opened tabs to
> the expected tabs?

Depending on how long this can wait, I will fix the underlying Marionette issue as filed as bug 1373191 today. So once this has been landed it should be fine. Otherwise file a new bug for those particular test steps.

Comment 122

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review153896

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:29
(Diff revision 7)
> +            let observer = () => {
> +                Services.obs.removeObserver(observer, 'sessionstore-state-write-complete');
> +                document.documentElement.setAttribute('sessionstore-state-write-complete', 'true');
> +            };
> +            Services.obs.addObserver(observer, 'sessionstore-state-write-complete');
> +            '''

Not sure what this execute_script should do, but it's most likely not doing what you expect! If it should wait until the 'sessionstore-state-write-complete' notification has been fired, you have to use `execute_async_script`. Right now, it will immediately return.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:35
(Diff revision 7)
> +        )
> +        self.open_windows(self.test_windows)
> +
> +        def session_recorded(self):
> +            return self.marionette.find_elements(
> +                    'css selector', '[sessionstore-state-write-complete]'

Import `By` from Marionette and use `By.CSS_SELECTOR` instead of a hardcoded string. Also please use 4 char indendation. Maybe it still fits all in one line? We allow 99 chars.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:38
(Diff revision 7)
> +        def session_recorded(self):
> +            return self.marionette.find_elements(
> +                    'css selector', '[sessionstore-state-write-complete]'
> +                )
> +
> +        Wait(self).until(session_recorded)

Wait should get `self.marionette` as parameter. I wonder how this doesn't fail. You could even use a lambda function here:

```
Wait(self.marionette).until(
    lambda _: self.marionette.find_elements(...),
    message="What did not work")
```

Also `sessionstore-state-write-complete` is not only an observer but we also add it as attribute to an element?

Keep in mind to always add a message, so a failure will be more descriptive.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:77
(Diff revision 7)
> +        super(TestTabbarSessionRestoreButton, self).setUp()
> +
> +    def test_session_exists(self):
> +        restore_tabs_button_wrapper = (
> +                self.puppeteer.windows.current.tabbar.restore_tabs_button_wrapper
> +            )

Just name the variable wrapper, so everything fits into the same line. Similar to other usages of this.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:111
(Diff revision 7)
> +
> +        restore_tabs_button.click()
> +
> +        # After clicking the button to restore the session,
> +        # there is more than one tab.
> +        self.assertTrue(len(self.puppeteer.windows.current.tabbar.tabs) > 1)

The test might go away for the initial landing but I want to point out that due to delays in opening the tabs, you have to use Wait().until() here too.

Comment 123

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review153992

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:39
(Diff revision 7)
> +            return self.marionette.find_elements(
> +                    'css selector', '[sessionstore-state-write-complete]'
> +                )
> +
> +        Wait(self).until(session_recorded)
> +        self.marionette.restart()

Oh, also keep in mind that `self.marionette.restart` is killing the process from the OS side and starting the application again. What you want is to use `self.restart` which is mixed in by Puppeteer, and is doing an in_app restart.

Comment 124

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review153896

> Wait should get `self.marionette` as parameter. I wonder how this doesn't fail. You could even use a lambda function here:
> 
> ```
> Wait(self.marionette).until(
>     lambda _: self.marionette.find_elements(...),
>     message="What did not work")
> ```
> 
> Also `sessionstore-state-write-complete` is not only an observer but we also add it as attribute to an element?
> 
> Keep in mind to always add a message, so a failure will be more descriptive.

So now I see how this is used. And I think all that should not be necessary when you would use an in_app restart which internally is calling quitApplication with the restart flag. I assume this always waits for the session state to be written.

Comment 125

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review153992

> Oh, also keep in mind that `self.marionette.restart` is killing the process from the OS side and starting the application again. What you want is to use `self.restart` which is mixed in by Puppeteer, and is doing an in_app restart.

Apparently you cannot use an `in_app` restart here, because that would force Firefox to always restore the session (as what I just have learned - thank you Mike):

http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/browser/components/sessionstore/SessionStore.jsm#1691

So Firefox has to be closed and started again. But you can still do a safe shutdown via: 

```
self.marionette.quit(in_app=True)
self.marionette.start_session()
```

With those two lines you can still remove all the code related to `sessionstore-state-write-complete`.

Comment 126

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review153992

> Apparently you cannot use an `in_app` restart here, because that would force Firefox to always restore the session (as what I just have learned - thank you Mike):
> 
> http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/browser/components/sessionstore/SessionStore.jsm#1691
> 
> So Firefox has to be closed and started again. But you can still do a safe shutdown via: 
> 
> ```
> self.marionette.quit(in_app=True)
> self.marionette.start_session()
> ```
> 
> With those two lines you can still remove all the code related to `sessionstore-state-write-complete`.

leaving as is after conversation. Since other Marionette bugs need to be fixed in order for the suggestion to work.

Comment 127

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/7-8/

Comment 128

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review154148

Looks fine to me with the two issues addressed. Please do not forget to file a follow-up bug. 

Also did you recently rebase against m-c? If not you should do because the former patch failed to apply correctly. And I don't see updates for those failing hunks in this update.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:95
(Diff revisions 7 - 8)
> -                self.puppeteer.windows.current.tabbar.restore_tabs_button
> -            )
>  
>          # The new browser window is not the same window as last time,
>          # and did not automatically restore the session, so there is only one tab.
>          self.assertTrue(len(self.puppeteer.windows.current.tabbar.tabs) == 1)

Please change this to assertEqual.

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:102
(Diff revisions 7 - 8)
> -        restore_tabs_button.click()
> +        button.click()
> +
> +        def ensure_multiple_tabs(self):
> +            return len(self.puppeteer.windows.current.tabbar.tabs) > 1
> +        Wait(self).until(
> +            ensure_multiple_tabs,

Just make this a lambda please:

```
Wait(self.marionette).until(
    lambda _: len(self.puppeteer.windows.current.tabbar.tabs) > 1,
    message="..")
```

Comment 129

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/8-9/

Comment 130

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/9-10/

Comment 131

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #109)
> Related to that, how well does your patch cope with long strings in general?

Erica?

(In reply to :Gijs from comment #115)
> > > > Has it been taken into account that this will remove the button when Firefox automatically opens a second tab such as post-update "what's new" pages, the privacy policy (when the policy changes), etc.. This will surprise people once they got used to the button.
> > > 
> > > My understanding was that that was not a concern. The button is fairly
> > > ephemeral already, so I don't think it will be missed.
> > 
> > I don't understand what you mean here. In said situation the user wouldn't
> > get a chance to see the button at all unless I misread your patch.
> 
> Whether this is correct depends on whether session init finishes before or
> after these tabs have been opened.

Ah, so this code may actually show the button with multiple tabs open. I missed that. Then again, in that case there would be less room for the button, which makes the above question more pressing.

> I agree that this might be a concern, but I also think this can wait for a
> followup bug, especially now this has missed 55.

I don't follow your logic here. If there are fundamental flaws, possibly inherent to the design, we should at least have a good understanding of this and a realistic plan before landing. 56, or any other cycle really, doesn't have that much room for big followups especially with Aurora being gone.

Comment 132

[Dão Gottwald [:dao]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review154502

::: browser/base/content/tabbrowser.xml:5882
(Diff revision 10)
>      <implementation implements="nsIDOMEventListener, nsIObserver">
>        <constructor>
>          <![CDATA[
>            this.mTabClipWidth = Services.prefs.getIntPref("browser.tabs.tabClipWidth");
>  
> +          let { restoreTabsButton } = gBrowser.tabContainer;

s/gBrowser.tabContainer/this/

::: browser/base/content/tabbrowser.xml:5965
(Diff revision 10)
> +          // note, there will only ever be one tab
> +          let tabbarUsedSpace = windowUtils.getBoundsWithoutFlushing(this.firstChild).width
> +            + windowUtils.getBoundsWithoutFlushing(newTabButton).width;
> +
> +          // Subtract the elements' widths from the available space to ensure
> +          // that showing the restoreTabsButton won't cause any overflow.

Here's an idea: Instead of putting the button immediately after tabs, we put it at the far end. If the button is overly wide, if there are multiple tabs, and/or if the window is narrow, tabs (but not the button) may overflow, which isn't great but at least failing gracefully and maybe the best we can do here. updateSessionRestoreVisibility can completely go away.

Comment 133

[Dão Gottwald [:dao]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review154504

::: browser/base/content/tabbrowser.xml:5960
(Diff revision 10)
> +
> +          let newTabButton = document.getAnonymousElementByAttribute(
> +            this, "anonid", "tabs-newtab-button");
> +
> +          // Get the widths of the elements in the tabs area
> +          // note, there will only ever be one tab

Forgot to note, based on the earlier conversation in the bug, this comment is potentially wrong.

Comment 134

[Erica Wright [:ewright]]

:dao,

For long strings, the button will become the size the string requires, if the button is too large to fit, it will not show itself.

For your other idea. This is being landed behind a pref - and will be turned on and used in a study. Depending on the result, I believe we could change anything about it (including the way it handle's longer strings).

> Forgot to note, based on the earlier conversation in the bug, this comment is potentially wrong.

I'll rewrite that, and include potential other tabs in the measurement.

Comment 135

[Dão Gottwald [:dao]]

(In reply to Erica from comment #134)
> :dao,
> 
> For long strings, the button will become the size the string requires, if
> the button is too large to fit, it will not show itself.
> 
> For your other idea. This is being landed behind a pref - and will be turned
> on and used in a study. Depending on the result, I believe we could change
> anything about it (including the way it handle's longer strings).

We already know about the edge cases the patch doesn't handle well. Why would we want to wait until after the user study to fix this? Even if no user in the study hits these cases, we should still do something about the fickle nature of updateSessionRestoreVisibility, and doing it before the study would be better, because it would be closer to what we'd eventually want to ship.

Comment 136

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #135)
> Why would we want to wait until after the user study to fix this?

Because if the study shows we don't want to do this / there is no benefit, then it would be a waste of time to make it perfect right now. Better check first, and then invest, rather than the other way around.

Comment 137

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #136)
> (In reply to Dão Gottwald [::dao] from comment #135)
> > Why would we want to wait until after the user study to fix this?
> 
> Because if the study shows we don't want to do this / there is no benefit,
> then it would be a waste of time to make it perfect right now. Better check
> first, and then invest, rather than the other way around.

I covered this in the rest of my comment. There's no way the study can tell us that narrow windows or locales with strings significantly longer than the en-US one aren't something to worry about.

Comment 138

[Dão Gottwald [:dao]]

I also don't buy that we'd need to "invest" to take care of this. Implementing my proposed solution would be mostly code removal.

Comment 139

[Erica Wright [:ewright]]

(In reply to Dão Gottwald [::dao] from comment #137)
> (In reply to :Gijs from comment #136)
> > (In reply to Dão Gottwald [::dao] from comment #135)
> > > Why would we want to wait until after the user study to fix this?
> > 
> > Because if the study shows we don't want to do this / there is no benefit,
> > then it would be a waste of time to make it perfect right now. Better check
> > first, and then invest, rather than the other way around.
> 
> I covered this in the rest of my comment. There's no way the study can tell
> us that narrow windows or locales with strings significantly longer than the
> en-US one aren't something to worry about.

I agree there is benefit to building the best possible thing, but I think that is the spec we have. For example, the solution you suggested does not actually avoid the same pitfalls of this one. If the button remains when there is not enough room for it there will be overflow in the tab area, which will contain (likely) one tab, or the tab could possibly not be visible at all, which is not a great experience. As well, when the browser gets really tiny, the button could potentially be larger than it, so it does not actually remove the need for `updateSessionRestoreVisibility`, as we'd still have to do something differently at smaller window sizes. I would argue that the open tab is more important than this button.

Comment 140

[Dão Gottwald [:dao]]

(In reply to Erica from comment #139)
> (In reply to Dão Gottwald [::dao] from comment #137)
> > (In reply to :Gijs from comment #136)
> > > (In reply to Dão Gottwald [::dao] from comment #135)
> > > > Why would we want to wait until after the user study to fix this?
> > > 
> > > Because if the study shows we don't want to do this / there is no benefit,
> > > then it would be a waste of time to make it perfect right now. Better check
> > > first, and then invest, rather than the other way around.
> > 
> > I covered this in the rest of my comment. There's no way the study can tell
> > us that narrow windows or locales with strings significantly longer than the
> > en-US one aren't something to worry about.
> 
> I agree there is benefit to building the best possible thing, but I think
> that is the spec we have.

The spec may need a revision.

> For example, the solution you suggested does not
> actually avoid the same pitfalls of this one.

It's failing in a better way, imho. Just removing the button is not good if we want users to actually use the button.

> If the button remains when
> there is not enough room for it there will be overflow in the tab area,
> which will contain (likely) one tab, or the tab could possibly not be
> visible at all, which is not a great experience.

We do scroll the selected tab into few when overflowing.

> As well, when the browser
> gets really tiny, the button could potentially be larger than it, so it does
> not actually remove the need for `updateSessionRestoreVisibility`, as we'd
> still have to do something differently at smaller window sizes. I would
> argue that the open tab is more important than this button.

"Really tiny" is an edge case we can worry about less, I'm more concerned about reasonably small windows. In tiny windows we could just hide the button, possibly with a simple CSS media query rather than JS.

Comment 141

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #140)
> > If the button remains when
> > there is not enough room for it there will be overflow in the tab area,
> > which will contain (likely) one tab, or the tab could possibly not be
> > visible at all, which is not a great experience.
> 
> We do scroll the selected tab into few when overflowing.

I meant "scroll into view" in case you hadn't figured that out already.

I had another conversation with bwinton, he'll post an abstract here for the sake of not having to repeat arguments again.

Comment 142

[Blake Winton (:bwinton) (:☕️)]

The abstract:
---------------------
We would rather not show the button than scroll the tabs if there is no room, because it looks better and because there are other ways to restore the session.

But we would rather scroll the tab because if the user gets used to the button and then one day firefox opens a second tab on startup and doesn't show the button, it would confuse them

At least, we should ensure the study will test cases such as firefox opening more then one tab on startup, and the study will give you a good idea of how likely users will run into these issues.
For instance, we open the data policy in a new tab when it changes. we may never hit this case in the study, we may not even hit this case in release for a year or two. and then when the policy changes, we'd potentially fail a lot of users

We want to know if _anyone_ will click the button.  If the answer is "no", then we don't need to fix any of the issues, and can back the code out.
We can also run follow-up studies, since we are likely to uncover further places where the design fails, which we will also have to take into account.

Addressing the issue before the study is simple enough, and if anything should make this study _more_ useful rather than following up with a second one.
---------------------

Comment 143

[Blake Winton (:bwinton) (:☕️)]

Further points, since I'm here typing anyways:

I disagree that addressing the issue is simple, since it will require a re-design, and the one alternative posted has issues of its own.

This version of the button will be preffed off, and only shown to people in the Shield study for a short time, so people won't get used to it.

I think we should definitely take into account the failure cases Dao raises when we design the next iteration of the button (if the data shows that enough people use it to make it worth implementing).

I also think that we shouldn't block testing the code we have on the issues Dao raises.  (Dao, obviously, disagrees. :)

Comment 144

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/10-11/

Comment 145

[Peter Dolanjski [:pdol]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #143)
> I also think that we shouldn't block testing the code we have on the issues

I think there are a few things we want to know:
Does the new design improve the discoverability of the session restore functionality?
If so, do more people make use of the functionality? (compared to a control)
If so, does exposing the functionality increase the utility of or satisfaction with Firefox? (effect might be too small to notice)

I think the current design generally gives us answers to at least the first two questions.

Comment 146

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #125)
> ```
> self.marionette.quit(in_app=True)
> self.marionette.start_session()
> ```
> 
> With those two lines you can still remove all the code related to
> `sessionstore-state-write-complete`.

Erica, this code works fine now, and if you still want you can clean up the test a lot. The fix in Marionette will also be uplifted to beta soon.

Comment 147

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review152826

r=me on the sessionstore related parts!

::: browser/base/content/browser.js:8181
(Diff revision 11)
>        Services.obs.addObserver(this, "sessionstore-last-session-cleared", true);
>        goSetCommandEnabled("Browser:RestoreLastSession", true);
>      }
>    },
>  
> +  handleEvent(aEvent) {

nit: please use `event` (without the 'a' for 'argument' prefix) in new methods like this.

::: browser/base/content/browser.js:8200
(Diff revision 11)
> +        gBrowser.tabContainer.updateSessionRestoreVisibility();
> +        gBrowser.tabContainer.removeEventListener("transitionend", maxWidthTransitionHandler);
> +      }
> +    });
> +    restoreTabsButton.style.maxWidth = `${restoreTabsButtonWrapperWidth}px`;
> +    requestAnimationFrame(function() {

nit: you can write this as `requestAnimationFrame(() => restoreTabsButton.style.maxWidth = 0);`.

Comment 148

[Dão Gottwald [:dao]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #142)
> The abstract:
> ---------------------
> We would rather not show the button than scroll the tabs if there is no
> room, because it looks better and because there are other ways to restore
> the session.

Even I couldn't tell with certainty what these other ways to restore the previous session would be. I guess there might be something somewhere in the history menus, but that's probably not widely known, since about:home currently covers that use case.

I think we need to get the use case covered in a consistent manner, and then we can worry about how it looks in edge cases. Simply refusing to offer the option when we can't make it look perfectly is not helpful to the user.

Comment 149

[Dão Gottwald [:dao]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

https://reviewboard.mozilla.org/r/143224/#review155102

::: browser/base/content/tabbrowser.xml:5963
(Diff revisions 10 - 11)
>  
> -          // Get the widths of the elements in the tabs area
> -          // note, there will only ever be one tab
> -          let tabbarUsedSpace = windowUtils.getBoundsWithoutFlushing(this.firstChild).width
> +          // If there are no pinned tabs it will multiply by 0 and result in 0
> +          let pinnedTabsWidth = windowUtils.getBoundsWithoutFlushing(this.firstChild).width * this._lastNumPinned;
> +
> +          let numUnpinnedTabs = this.childNodes.length - this._lastNumPinned;
> +          let unpinnedTabsWidth = windowUtils.getBoundsWithoutFlushing(this.lastChild).width * numUnpinnedTabs

nit: missing semicolon

I think the existance of updateSessionRestoreVisibility is a mistake, but I can r+ on the assumption that we won't ship this.

Comment 150

[Erica Wright [:ewright]]

(In reply to Henrik Skupin (:whimboo) from comment #146)
> (In reply to Henrik Skupin (:whimboo) from comment #125)
> > ```
> > self.marionette.quit(in_app=True)
> > self.marionette.start_session()
> > ```
> > 
> > With those two lines you can still remove all the code related to
> > `sessionstore-state-write-complete`.
> 
> Erica, this code works fine now, and if you still want you can clean up the
> test a lot. The fix in Marionette will also be uplifted to beta soon.

I gave it a shot, but the newly created window now has an error on `self.puppeteer.windows.current.tabbar` with the error: 
```
wrapper = self.puppeteer.windows.current.tabbar.restore_tabs_button_wrapper
  File "/Users/ewright/src/mozilla-central/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py", line 143, in tabbar
    tabbrowser = self.window_element.find_element(By.ID, 'tabbrowser-tabs')
NoSuchElementException: Unable to locate element: tabbrowser-tabs
```

For now I'll keep it as is

Comment 151

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/11-12/

Comment 152

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c829f7a6cc0d
Add a button for session restore to the tab bar. r=dao,Gijs,mikedeboer,whimboo ui-r=shorlander

Comment 153

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for flake8 linting failure at tabbar.py:53: line too long:

https://hg.mozilla.org/integration/autoland/rev/7f529ce07c1aa90f1b2429eaf069b559bc7f1ce0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c829f7a6cc0d79c4a583990d614d2ef17b6c618d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=108274157&repo=autoland

> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/tabbar.py:53:100 | line too long (102 > 99 characters) (E501)

Comment 154

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/12-13/

Comment 155

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4482dcd48309
Add a button for session restore to the tab bar. r=dao,Gijs,mikedeboer,whimboo ui-r=shorlander

Comment 156

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for functional test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=108321917&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/c858a132ec9b743d0a1b1af13aaa99d6fbe53c98

Comment 157

[Blake Winton (:bwinton) (:☕️)]

Just as a note, Erica kicked off a try job at https://treeherder.mozilla.org/#/jobs?repo=try&revision=19febd5ab2edc375e1df50af189de7074d2e23e7&filter-searchStr=functional with some test changes suggested by Whimboo that look like they fix the problem, so hopefully we'll be good to land when she throws that up on ReviewBoard.

Comment 158

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #157)
> Just as a note, Erica kicked off a try job at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=19febd5ab2edc375e1df50af189de7074d2e23e7&filter-
> searchStr=functional with some test changes suggested by Whimboo that look

It looks good. But where you able to identify/solve your issue with the accidentally started safe mode?

Comment 159

[Blake Winton (:bwinton) (:☕️)]

No, but it seems very intermittent, and likely to be solved by bug 1374762, so I'm not _really_ worried about it.  ;)

Comment 160

[Erica Wright [:ewright]]

Pushed to review once more, with the code that ran on try as bwinton posted

Comment 161

[Erica Wright [:ewright]]

Comment on attachment 8873568 [details]
Bug 1219725 - Add a button for session restore to the tab bar.  ui-r=shorlander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143224/diff/13-14/

Comment 162

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/462f695ded76
Add a button for session restore to the tab bar. r=dao,Gijs,mikedeboer,whimboo ui-r=shorlander

Comment 163

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/462f695ded76

Comment 164

[:Gijs (he/him)]

I should have removed the leave-open keyword after the strings had landed. Anyway, this is now on m-c, so -> FIXED. \o/

Comment 165

[Mohammad Maruf Rahman]

This bug was about "adding a button for session restore to the tab bar" and I have seen the feature being implemented with latest Beta 56.0b12 on Ubuntu 16.04 LTS!

Build ID   : 20170914024831
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 166

[Sajedul Islam]

This bug was about "adding a button for session restore to the tab bar" and I have seen the feature being implemented with latest Beta 56.0b12 on Windows 8.1, 64-bit !

Build ID   : 20170914024831
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

[testday-20170915]

Comment 167

[Mohammad Maruf Rahman]

As per comment 165 and comment 166, I am marking this bug as Verified Fixed!

Comment 168

[Dão Gottwald [:dao]]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #143)
> I think we should definitely take into account the failure cases Dao raises
> when we design the next iteration of the button (if the data shows that
> enough people use it to make it worth implementing).
> 
> I also think that we shouldn't block testing the code we have on the issues
> Dao raises.  (Dao, obviously, disagrees. :)

What's the status of this?

Comment 169

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #168)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #143)
> > I think we should definitely take into account the failure cases Dao raises
> > when we design the next iteration of the button (if the data shows that
> > enough people use it to make it worth implementing).
> > 
> > I also think that we shouldn't block testing the code we have on the issues
> > Dao raises.  (Dao, obviously, disagrees. :)
> 
> What's the status of this?

There's an active shield study, see bug 1400942. I expect it'll run a bit longer and then there'll be data/conclusion-gathering. So, "in progress", I guess? :-)

Comment 170

[Blake Winton (:bwinton) (:☕️)]

Yup. :) PDol is tracking it, and might have a more detailed status update, but I suspect "in progress" mostly covers it.