This button opens a subview when clicked, which contains: * A button called 'View History Sidebar' * A button called 'Clear Recent History' * A button called 'Restore Previous Session' (disabled if there's no session) * A separator * A button called 'Recently Closed Tabs', which references another subview * A button called 'Recently Closed Windows', which references another subview * A separator * A list of items visited today * A separator * A list of items visited yesterday It is recommended to implement the complete subview in a separate bug, per-spec. The title of the subview reads 'History', the same as the button label.

Comment on attachment 8888274 [details] Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. https://reviewboard.mozilla.org/r/159222/#review164608 ::: browser/themes/shared/menupanel.inc.css:261 (Diff revision 2) > #appMenu-library-history-button { > list-style-image: url(chrome://browser/skin/history.svg); > } > > +#appMenuRecentlyClosedTabs, > +#appMenuRecentlyClosedWindows, I have to ask Aaron for a better icon here. The one in the spec also isn't very, well, good. ::: browser/themes/shared/menupanel.inc.css:276 (Diff revision 2) > list-style-image: url("chrome://browser/skin/device-mobile.svg"); > } > > +%ifdef MOZ_PHOTON_THEME > #appMenuViewHistorySidebar, > +#appMenuRestoreLastSession, I don't really get why the sidebar icon is used for this button in the spec. I'll ask Aaron for a new icon here too.

Aaron, when you look at https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229252061, you'll see what I mention in comment 3: the icons used for 'Restore Previous Session' and 'Recently Closed Windows' are rather odd. Are you able to provide ones that are more related to what the action describes?

Perhaps new-window.svg would be a good fit for 'Recently Closed Windows'?

Comment on attachment 8888274 [details] Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. https://reviewboard.mozilla.org/r/159222/#review164906 r=me assuming Marco OKs the marker stuff, though if you want me to look at the ViewShowing stuff and/or the footer extraction if you decide to implement that, that is also OK. ::: browser/components/customizableui/CustomizableWidgets.jsm:198 (Diff revision 4) > // Populate our list of history > const kMaxResults = 15; > let doc = aEvent.target.ownerDocument; > let win = doc.defaultView; > > + if (win.gPhotonStructure) { Nit: here and elsewhere, use AppConstants.MOZ_PHOTON_THEME, because the markup is wrong otherwise. Or maybe even && those. I don't know. We have 2 things, it is not going to work right one way or another if you try to flip this pref to true on a non-photon-theme thing, or if you flip the pref to false on a photon theme thing (because we'll use a non-photon panelmultiview for the history panel when it's opened standalone). Not sure if there's much we can do about this. ::: browser/components/customizableui/CustomizableWidgets.jsm:314 (Diff revision 4) > + // When either of these sub-subviews show, populate them with recently closed > + // objects data. > + document.getElementById(this.recentlyClosedTabsPanel).addEventListener("ViewShowing", this); > + document.getElementById(this.recentlyClosedWindowsPanel).addEventListener("ViewShowing", this); TIL, you can't add the same listener (JS object identity) more than once. I thought we'd have to remove the listener in viewhiding as well, but I guess panelmultiviewhidden alone is enough, then. ::: browser/components/customizableui/CustomizableWidgets.jsm:328 (Diff revision 4) > // Middle clicking recently closed items won't close the panel - cope: > let onRecentlyClosedClick = function(aEvent) { FWIW, this seems relevant to https://bugzilla.mozilla.org/show_bug.cgi?id=1377967 , maybe. Does this fix the history part of that bug? ::: browser/components/customizableui/CustomizableWidgets.jsm:364 (Diff revision 4) > + let utils = RecentlyClosedTabsAndWindowsMenuUtils; > + let fragment = utils[`get${viewType}Fragment`](window, "toolbarbutton", > + true, `menuRestoreAll${viewType}Subview.label`); Nit: split the method name and the string id into temp vars for readability's sake, then line up the args with the starting ( ? :-) ::: browser/components/customizableui/CustomizableWidgets.jsm:373 (Diff revision 4) > + this._panelMenuView._setEmptyPopupStatus(panelview, !elementCount); > + while (--elementCount >= 0) { > + let element = fragment.children[elementCount]; > + CustomizableUI.addShortcut(element); > + element.classList.add("subviewbutton"); > + if (!element.classList.contains("restoreallitem")) The invision spec has the 'restore all' items as footers. Could we split them out of the fragment here? Note that their strings (in the mockup) don't really match. Probably good to check with UX what we should be using. ::: browser/components/customizableui/PanelMultiView.jsm:687 (Diff revision 4) > + let custWidget = CustomizableWidgets.find(widget => widget.viewId == viewNode.id); > + if (custWidget) > + custWidget["on" + eventName]({ target: viewNode }); It's a little ugly that we're then not using this for ViewShowing. Is that hard to do? Also, the onFoo version can't cancel anything. Given it's for ViewHiding, I guess it doesn't matter... but it will matter for ViewShowing if we try to use it for that too. ::: browser/components/places/content/browserPlacesViews.js:2118 (Diff revision 4) > - if (!panelview._startMarker.previousSibling && > - !panelview._endMarker.nextSibling) > + if (!panelview._startMarker || > + (!panelview._startMarker.previousSibling && !panelview._endMarker.nextSibling)) { This makes me nervous because I'm not sure off-hand what this does. Can Marco maybe sanity-check that this won't break the assumptions of other places views? ::: browser/components/places/content/browserPlacesViews.js:2174 (Diff revision 4) > if (!this.controllers.getControllerCount() && this._controller) > this.controllers.appendController(this._controller); > } > } > + > +this.PlacesPanelview = PlacesPanelview; Why do we need this now?

Comment on attachment 8888274 [details] Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. https://reviewboard.mozilla.org/r/159222/#review164906 > TIL, you can't add the same listener (JS object identity) more than once. I thought we'd have to remove the listener in viewhiding as well, but I guess panelmultiviewhidden alone is enough, then. Nice, eh! > FWIW, this seems relevant to https://bugzilla.mozilla.org/show_bug.cgi?id=1377967 , maybe. Does this fix the history part of that bug? Ermz, maybe? I don't have a middle click handy atm. > The invision spec has the 'restore all' items as footers. Could we split them out of the fragment here? > > Note that their strings (in the mockup) don't really match. Probably good to check with UX what we should be using. Oooh, I hadn't even checked the specs for those :/ Thanks for the heads-up! > It's a little ugly that we're then not using this for ViewShowing. Is that hard to do? > > Also, the onFoo version can't cancel anything. Given it's for ViewHiding, I guess it doesn't matter... but it will matter for ViewShowing if we try to use it for that too. Good point; it resulted in a very nice cleanup as well! > Why do we need this now? TYL: standalone class expressions are block-scoped. I did, however, adjust the code somewhat to use `var PlacesPanelview = ` to make the scoping effort a bit clearer.

Comment on attachment 8888274 [details] Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. https://reviewboard.mozilla.org/r/159222/#review165112 This looks OK besides the below, but also note that on try, the webextension popup tests are unhappy. I expect it's the messing with the event listeners and how they relate to the timing of the panelmultiview's `_viewShowing` prop. :-( ::: browser/components/customizableui/CustomizableWidgets.jsm:370 (Diff revisions 4 - 5) > - let fragment = utils[`get${viewType}Fragment`](window, "toolbarbutton", > - true, `menuRestoreAll${viewType}Subview.label`); > + let method = `get${viewType}Fragment`; > + let fragment = utils[method](window, "toolbarbutton"); > let elementCount = fragment.childElementCount; > this._panelMenuView._setEmptyPopupStatus(panelview, !elementCount); > + if (elementCount) { > + let body = fragment.insertBefore(document.createElement("vbox"), fragment.children[0]); Wouldn't it be easier to just create the vbox with the class, and append the fragment to the vbox, and then take out the restoreallitem individually, finally appending the footer and the vbox? That's fewer DOM manipulations from JS. :-) ::: browser/components/customizableui/PanelMultiView.jsm:690 (Diff revisions 4 - 5) > + } > + } > + > + let evt = new this.window.CustomEvent(eventName, { > + bubbles: true, > + cancelable: eventName != "ViewShown" I don't think viewhiding is cancelable... should this be eventName == "ViewShowing" ?

Comment on attachment 8888274 [details] Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. https://reviewboard.mozilla.org/r/159222/#review166138 I trust Gijs on the general review, I'm just leaving a comment about the markers code. ::: browser/components/customizableui/CustomizableWidgets.jsm:362 (Diff revision 6) > + let viewType = panelview.id == this.recentlyClosedTabsPanel ? "Tabs" : "Windows"; > + > + while (panelview.firstChild) { > + panelview.firstChild.remove(); > + } > + panelview._emptyMenuitem = panelview._startMarker = panelview._endMarker = null; I'm not extremely happy about this pollution, where Customizeable widgets needs to play with the code internals of the view... Can we provide a cleaner abstraction for it, a method on the view to clear it completely, includind the markers? Something like resetContents or clearAllContents that would include both the removal and resetting the internal details.

(In reply to Marco Bonardo [::mak] from comment #13) > Comment on attachment 8888274 [details] > Bug 1354533 - Update the History panelview when it's shown inside the new > Library panel. > > https://reviewboard.mozilla.org/r/159222/#review166138 > > I trust Gijs on the general review, I'm just leaving a comment about the > markers code. > > ::: browser/components/customizableui/CustomizableWidgets.jsm:362 > (Diff revision 6) > > + let viewType = panelview.id == this.recentlyClosedTabsPanel ? "Tabs" : "Windows"; > > + > > + while (panelview.firstChild) { > > + panelview.firstChild.remove(); > > + } > > + panelview._emptyMenuitem = panelview._startMarker = panelview._endMarker = null; > > I'm not extremely happy about this pollution, where Customizeable widgets > needs to play with the code internals of the view... Can we provide a > cleaner abstraction for it, a method on the view to clear it completely, > includind the markers? > Something like resetContents or clearAllContents that would include both the > removal and resetting the internal details. Because Mike's out, I'm going to be pushing this over the line. Where do you want this method to live? On some kind of places view object?

I'd expect it to exist at the view level (same level as ._startMarker and friends)

Comment on attachment 8889897 [details] Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. https://reviewboard.mozilla.org/r/160964/#review166276

Comment on attachment 8889897 [details] Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. https://reviewboard.mozilla.org/r/160964/#review166278 yes, it's a little bit more abstracted away now. Note I didn't do a full review, just checked the previous issue. If it works, it will be fine :)

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/58b73242f3ef Update the History panelview when it's shown inside the new Library panel. r=mak

(The earlier trypush was green bar some webextension issues, which I fixed locally by always passing the `detail` parameter. So I pushed with that updated.)

Verified on Windows, Mac and Ubuntu with the specs in Comment 1.

^ Meant to be Comment 0.