1385083 - History button (and history panel in library, after bug 1354117) doesn't show the Recent History

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[smartfon.reddit]

Attachment: firefox nightly history.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170727162801

Steps to reproduce:

Visit sites. Click on the History button. The "Recent History" section is always empty. History is shown properly when it's accessed via Sidebar and Ctrl+Shift+H.

Tested with July 26 and July 27 (latest) builds, and a fresh profile.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2039119836eafbe19e47a9db193141e5f1da355d&tochange=58b73242f3ef211bdac77612e8cd797b49a22a35

Regressed by: 58b73242f3ef	Mike de Boer — Bug 1354533 - Update the History panelview when it's shown inside the new Library panel. r=mak


no automate test?

Comment 2

[Marco Bonardo [:mak]]

Please stop needinfo-ing a bunch of people to ask about automated tests, it's quite annoying.

Comment 3

[:Gijs (he/him)]

I tested, and this doesn't reproduce on cedar, so we'll be good for the 56 release, just need to fix for photon.

It seems we run the right places view initialization when this happens, so I don't understand off-hand why it doesn't work. Marco, would you know why the places view doesn't successfully render places nodes in this case? It seems like popup._placesNode is null in the single panel case, but not in the case where the popup is shown from the library. What's supposed to instantiate that?

Comment 4

[Marco Bonardo [:mak]]

how do I reproduce this? both the history button and Library/History are WFM...

Comment 5

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #4)
> how do I reproduce this? both the history button and Library/History are
> WFM...

0. move history button to the toolbar
1. open a new window
2. open history button (don't open the library entrypoint first).

Comment 6

[:Gijs (he/him)]

You also need the photon structure pref set to the nightly default (ie photon structure enabled).

Comment 7

[Marco Bonardo [:mak]]

OK, I needed to open the new window. I'll into this tomorrow morning.

Comment 8

[Marco Bonardo [:mak]]

Could it be because of this? http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/base/content/browser-places.js#1998

Comment 9

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #8)
> Could it be because of this?
> http://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/base/content/browser-places.
> js#1998

Maybe, though copying that to https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#312 doesn't seem to be fixing things.

Comment 10

[Marco Bonardo [:mak]]

the .place setter creates the result, and adds the view as an observer
that will set view.result to the result.
the result setter sets _rootElt._placesNode

_onPopupShowing does
let popup = aEvent.originalTarget;
and then uses popup._placesNode

BUT the PlacesPanelview constructor doe:
this._onPopupShowing({ originalTarget: this._viewElt });

Instead of passing _rootElt to _onPopupShowing, it's passing _viewElt.
_viewElt._placesNode is undefined, since it's set on _rootElt.

Just changing this._viewElt to this._rootElt seems to fix the problem for me.

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1385083 - history panel should actually list history,

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

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1385083 - Name and use the event argument appropriately by fixing a typo.

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

Comment 14

[:Gijs (he/him)]

Comment on attachment 8894520 [details]
Bug 1385083 - Name and use the event argument appropriately by fixing a typo.

https://reviewboard.mozilla.org/r/165692/#review170768

This isn't sufficient, please see the patch I raced with.

Comment 15

[:Gijs (he/him)]

My precious!

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8894519 [details]
Bug 1385083 - history panel should actually list history,

https://reviewboard.mozilla.org/r/165688/#review170770

This looks good to me! r=me with an explaination about the change in browserPlacesViews.js.

::: browser/components/customizableui/CustomizableWidgets.jsm:194
(Diff revision 1)
>          "&maxResults=42&excludeQueries=1";
> +
> +      // XPCOMUtils.defineLazyScriptGetter can't return class constructors, so
> +      // trigger the getter once without using the result before calling
> +      // PlacesPanelview as a constructor.
> +      window.PlacesPanelview;

I've run into this before and I think changing `var PlacesPanelview = class extends PlacesViewBase {` to `this.PlacesPanelview = class extends PlacesViewBase {` in browserPlacesViews.js fixed it for me.

::: browser/components/places/content/browserPlacesViews.js:2015
(Diff revision 1)
>      options.viewElt = panelview;
>      super(place, options);
>      this._viewElt._placesView = this;
>      // We're simulating a popup show, because a panelview may only be shown when
>      // its containing popup is already shown.
> -    this._onPopupShowing({ originalTarget: this._viewElt });
> +    this._onPopupShowing({ originalTarget: this._rootElt });

Why this change? I see the bookmarks panelview still works, so was this a bug I introduced?

Comment 17

[:Gijs (he/him)]

Comment on attachment 8894519 [details]
Bug 1385083 - history panel should actually list history,

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87152019960e
history panel should actually list history, r=mikedeboer

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87152019960e

Comment 20

[Azmina]

I have reproduced this bug with Nigtly 56.0a1 (2017-07-27) on Windows 8.1 (64 bit).

This bug's fix is verified on latest Nightly 57.0a1.

Build ID : 20170808114032
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 21

[Fahima Zulfath]

Managed to reproduce the issue on an affected build (Firefox 57.0a1), using the steps to reproduce from Comment 0, on Windows 10 X 64 bit. Bug 1385083 is fixed & verified.
build ID: 20170809100326
[bugday-20170809]

Comment 22

[u554753]

Verified on Windows, Mac, and Ubuntu.