Closed Bug 855803 Opened 7 years ago Closed 6 years ago

Create the History widget with subview

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: jaws)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch Rough WIP patch (obsolete) — Splinter Review
This patch is rough, but it adds a History subview. It currently uses a <tree> for showing the history, but I think we'll want to change it to use richlistitems.

Also, both views (History and Bookmarks) are appearing when one of them is chosen. I looked at the panelUI.js code but didn't see anything out of the ordinary (selectedPanel is being set on the deck properly).
I looked into the misbehaving deck - it looks like the position: absolute; on #PanelUI-subViews is causing this. Removing that rule fixed the problem, and I don't believe position: absolute is actually necessary. Probably a leftover fragment from my experiments making the subview thing actually work.
Attached patch Patch v.1 (obsolete) — Splinter Review
This patch is pretty close, but it has a couple issues:
* The panel size is incorrect the first time the subview is shown (due to the async calls within onViewShowing)
* The favicon is not clickable
* Might switch from richlistbox/richlistitem to toolbarbuttons, if that is simpler and works better.
Attachment #731208 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
This handles all the previously mentioned issues.

One remaining issue is when the widget is dragged to the navbar, clicking on it is currently a no-op. Fixing this wasn't part of this patch because it involves much more work and might not be want we want to do anyways.
Attachment #735352 - Attachment is obsolete: true
Attachment #736065 - Flags: review?(mconley)
Comment on attachment 736065 [details] [diff] [review]
Patch

Review of attachment 736065 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following changes.

::: browser/base/content/panelUI.inc
@@ +69,5 @@
>              <toolbarbutton label="&viewBookmarksToolbar.label;"/>
>              <toolbarbutton label="&showAllBookmarks2.label;"/>
>              <spacer flex="1"/>
>            </vbox>
> +          <vbox class="PanelUI-tall-maker"/>

The tall-maker stuff is probably safe to revert. It's bogus anyway, and will likely go away - but let's just leave it the way it was for now.

::: browser/modules/CustomizableUI.jsm
@@ +47,5 @@
>        // Right now, I just jam in a tall vbox to simulate making the view
>        // tall before switching to it.
>        LOG("Bookmark view is being shown!");
>        let doc = aEvent.detail.ownerDocument;
> +      let vbox = aEvent.target.getElementsByClassName("PanelUI-tall-maker")[0];

Please undo this change as well.

@@ +116,5 @@
>    }, {
> +    id: "history-panelmenu",
> +    type: "view",
> +    viewId: "PanelUI-history",
> +    name: "History...",

At some point, I guess we're going to have to worry about translating these labels. But not right now.

@@ +169,5 @@
> +                  doc.defaultView.PanelUI.hide();
> +                }
> +              });
> +              item.setAttribute("image", "moz-anno:favicon:" + icon);
> +            } catch (e) {}

I think at the very least this should Cu.reportError, instead of failing silently.

::: browser/themes/shared/panelUIOverlay.inc.css
@@ +223,5 @@
> +  -moz-appearance: none;
> +  background-color: transparent;
> +}
> +
> +.panelHistoryItem[selected="true"] {

This rule can probably be removed.

@@ +237,5 @@
> +.panelHistoryItemIcon[src=""] {
> +  list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");
> +}
> +
> +.panelHistoryItemLabel {

This rule as well should be removed.
Attachment #736065 - Flags: review?(mconley) → review+
Comment on attachment 736065 [details] [diff] [review]
Patch

Review of attachment 736065 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/CustomizableUI.jsm
@@ +10,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");

This should be lazy-loaded.

@@ +125,5 @@
>        "16": "chrome://branding/content/icon16.png",
>        "32": "chrome://branding/content/icon48.png",
>        "48": "chrome://branding/content/icon48.png"
> +    },
> +    onViewShowing: function(aEvent) {

This function convinces me we need built-in widgets to be defined in a separate file (even if its just preprocessed in). Filed bug 860642.

@@ +149,5 @@
> +      // Clear previous history items.
> +      while (items.firstChild) {
> +        items.removeChild(items.firstChild);
> +      }
> +      items.appendChild(fragment);

Hm, think this exposed a bug in our panel resize code. For me, if I don't have many widgets in the panel, the panel doesn't resize to fit all the history items. But it does for the bookmarks subview, and then going back into the history subview the panel resizes to the size needed for the bookmarks subview. Filed bug 860646.

@@ +154,5 @@
> +
> +      PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +                         .asyncExecuteLegacyQueries([query], 1, options, {
> +        handleResult: function (aResultSet) {
> +          for (let row, i = 0; (row = aResultSet.getNextRow()); i++) {

Nit: What's with the extra () ?

::: browser/themes/shared/panelUIOverlay.inc.css
@@ +226,5 @@
> +
> +.panelHistoryItem[selected="true"] {
> +}
> +
> +.panelHistoryItemIcon {

This isn't used anymore... but it needs to be (to enforce the size, and the default favicon - the rest probably isn't needed). Selector can just be changed to something like #PanelUI-historyItems > toolbarbutton > .toolbarbutton-icon
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #6)
> > +      PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> > +                         .asyncExecuteLegacyQueries([query], 1, options, {
> > +        handleResult: function (aResultSet) {
> > +          for (let row, i = 0; (row = aResultSet.getNextRow()); i++) {
> 
> Nit: What's with the extra () ?

It takes care of a strict warning for a potentially unintended assignment.
Attached patch Patch v2 (obsolete) — Splinter Review
Fixes some of the issues and builds on top of the patch in bug 860646.
Attachment #736065 - Attachment is obsolete: true
Attachment #736591 - Flags: review?(mconley)
Comment on attachment 736591 [details] [diff] [review]
Patch v2

Review of attachment 736591 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these two nits fixed.

::: browser/modules/CustomizableUI.jsm
@@ +140,5 @@
> +      options.includeHidden = false;
> +      options.resultType = options.RESULTS_AS_URI;
> +      options.queryType = options.QUERY_TYPE_HISTORY;
> +      options.sortingMode = options.SORT_BY_DATE_DESCENDING;
> +      options.maxResults = 10;

Let's move this magic number up to a const at the top of this function.

@@ +143,5 @@
> +      options.sortingMode = options.SORT_BY_DATE_DESCENDING;
> +      options.maxResults = 10;
> +      let query = PlacesUtils.history.getNewQuery();
> +
> +      let fragment = doc.createDocumentFragment();

Let's move this fragment assignment into the handleResult function - I don't think it's needed out here.
Attachment #736591 - Flags: review?(mconley) → review+
The first draft of this widget just landed in jamun: https://hg.mozilla.org/projects/jamun/rev/8b5a8000425e
Whiteboard: [fixed in jamun]
Landed in UX as https://hg.mozilla.org/projects/ux/rev/8b5a8000425e
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed in ux]
Depends on: 878102
https://hg.mozilla.org/mozilla-central/rev/f08e68d08484
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed in ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.