Closed Bug 1385083 Opened 7 years ago Closed 7 years ago

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

Categories

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

56 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- verified

People

(Reporter: smartfon.reddit, Assigned: Gijs)

References

Details

(Keywords: regression, Whiteboard: [photon-structure])

Attachments

(3 files)

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.
Component: Untriaged → Bookmarks & History
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?
Blocks: 1354533
Flags: needinfo?(mdeboer)
Flags: needinfo?(mak77)
Keywords: regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
Please stop needinfo-ing a bunch of people to ask about automated tests, it's quite annoying.
Flags: needinfo?(mak77)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.4 - Aug 1
Flags: needinfo?(mdeboer)
Priority: -- → P1
Summary: History button doesn't show the Recent History. Available on Sidebar and Ctrl+Shift+H. → History button doesn't show the Recent History (but does when opened from the library)
Whiteboard: [photon-structure]
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
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?
Flags: needinfo?(mak77)
how do I reproduce this? both the history button and Library/History are WFM...
(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).
You also need the photon structure pref set to the nightly default (ie photon structure enabled).
OK, I needed to open the new window. I'll into this tomorrow morning.
(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.
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.
Flags: needinfo?(mak77)
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Blocks: 1387512
Summary: History button doesn't show the Recent History (but does when opened from the library) → History button (and history panel in library, after bug 1354117) doesn't show the Recent History
Assignee: gijskruitbosch+bugs → mdeboer
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.
Attachment #8894520 - Flags: review?(gijskruitbosch+bugs)
My precious!
Assignee: mdeboer → gijskruitbosch+bugs
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?
Attachment #8894519 - Flags: review?(mdeboer) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87152019960e
history panel should actually list history, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/87152019960e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
QA Whiteboard: [bugday-20170809]
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]
Depends on: 1388753
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.