329586 - Places Browser Shim Cleanup

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

cleanup + move view to new window

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: partial work

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more work

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

comments shortly.

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: merged patch

comments in a sec

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214386 [details] [diff] [review]
merged patch

>Index: browser/base/content/browser-context.inc
>       <menuitem id="context-bookmarkpage"
>                 label="&bookmarkPageCmd.label;"
>                 accesskey="&bookmarkPageCmd.accesskey;"
>+#ifdef MOZ_PLACES
>+                command="Browser:AddBookmarkAs"/>
>+#else
>                 oncommand="addBookmarkAs(document.getElementById('content'));"/>
>+#endif

No need to have an oncomand handler if the attached <command> works...

>Index: browser/base/content/browser-menubar.inc
>       <menuitem label="&addCurPageAsCmd.label;" 
>                 command="Browser:AddBookmarkAs" key="addBookmarkAsKb"/>
>-      <menuseparator builder="start"/>
>-      <menuseparator builder="end"/>
>+      <menuitem label="&addCurPagesCmd.label;" 
>+                command="Browser:BookmarkAllTabs" key="bookmarkAllTabsKb"/>
>+      <menuitem label="&searchBookmarksCmd.label;" 
>+                command="Browser:ShowBookmarks" key="searchBookmarksKb"/>
>       <menuitem label="&manBookmarksCmd.label;" 
>                 command="Browser:ShowBookmarks" key="manBookmarkKb"/>
>+      <menuseparator builder="start"/>

Make static menu items at the top match the spec (wiki). 

>Index: browser/base/content/browser-sets.inc
>+#ifdef MOZ_PLACES
>+    <command id="Browser:AddBookmarkAs" 
>+             oncommand="PlacesCommandHook.bookmarkCurrentPage();"/>
>+    <command id="Browser:BookmarkAllTabs" 
>+             oncommand="PlacesCommandHook.bookmarkCurrentPages();"/>
>+#else
>     <command id="Browser:AddBookmarkAs" oncommand="addBookmarkAs(document.getElementById('content'), false);"/>
>     <command id="Browser:BookmarkAllTabs" oncommand="addBookmarkAs(document.getElementById('content'), true);"/>
>+#endif

Call the right functions for bookmark commands. 

>-    <command id="Browser:ShowBookmarks" oncommand="PlacesBrowserShim.showBookmarks();"/>
>-    <command id="Browser:ShowHistory" oncommand="PlacesBrowserShim.showHistory();"/>
>+    <command id="Browser:ShowBookmarks" oncommand="PlacesCommandHook.showPlacesOrganizer('bookmarks');"/>
>+    <command id="Browser:ShowHistory" oncommand="PlacesCommandHook.showPlacesOrganizer('history');"/>

Use the right function...

>+#ifdef MOZ_PLACES
>+    <key id="searchBookmarksKb" key="&searchBookmarksCmd.commandkey;" command="Browser:ShowBookmarks" modifiers="accel"/>
>+#else
>     <key id="viewBookmarksSidebarKb" key="&bookmarksSidebarCmd.commandkey;" command="viewBookmarksSidebar" modifiers="accel"/>
>+#endif

Add a keybinding for showing the search popup (even if it only shows the bookmarks organizer for now). 

>Index: browser/base/content/browser.css
>-#bookmarksBarContent {
>-  -moz-binding: url("chrome://browser/content/places/toolbar.xml#places-bar");
>-  overflow: hidden;
>-}
>-
>-menupopup[type="places"] {
>-  -moz-binding: url("chrome://browser/content/places/menu.xml#places-menupopup");
>-}
>-

Move these places specific things into chrome://browser/content/places/places.css for API consistency (the tree hookup is there). 

>Index: browser/base/content/browser.js
> function pageShowEventHandlers(event)
> {
>   // Filter out events that are not about the document load we are interested in
>   if (event.originalTarget == content.document) {
>     checkForDirectoryListing();
>     charsetLoadListener(event);
>-#ifdef ALTSS_ICON
>-    updatePageStyles();
>-#endif
>-    FeedHandler.updateFeeds();
>+    
>+    XULBrowserWindow.asyncUpdateUI();

Call single unified function instead of several to init objects. 

>+#ifndef MOZ_PLACES
> function UpdateBookmarkAllTabsMenuitem()
> {
>   var tabbrowser = getBrowser();
>   var numTabs = 0;
>   if (tabbrowser)
>     numTabs = tabbrowser.tabContainer.childNodes.length;
> 
>   var bookmarkAllCommand = document.getElementById("Browser:BookmarkAllTabs");
>@@ -227,16 +226,17 @@ function addBookmarkMenuitems()
> function BookmarkThisTab()
> {
>   var tab = getBrowser().mContextTab;
>   if (tab.localName != "tab")
>     tab = getBrowser().mCurrentTab;
> 
>   addBookmarkAs(tab.linkedBrowser, false);
> }
>+#endif

A lot of stuff needs to die for non-places. 

>-#ifdef MOZ_PLACES
>-  PlacesBrowserShim.init();
>-#endif
>   setTimeout(delayedStartup, 0);
> }

No more need to init the shim in delayedStartup()

>-  // loads the services
>-#ifndef MOZ_PLACES
>-  initServices();
>-  initBMService();
>-#endif

Move around for code cleanliness.

>+#ifndef MOZ_PLACES
>   // add bookmark options to context menu for tabs
>   addBookmarkMenuitems();
>-#ifndef MOZ_PLACES
>+  initServices();
>+  initBMService();

See above. Comment out addBookmarkMenuitems for now too (intentional). 

> #else
>-  // XXXben - move this to the toolbar constructor if there is no performance penalty! (Ts/Txul)
>-  var bookmarksBar = document.getElementById("bookmarksBarContent");
>-  bookmarksBar.init();
>-  var bookmarksMenuPopup = document.getElementById("bookmarksMenuPopup");
>-  bookmarksMenuPopup.init();
>+  window.controllers.appendController(PlacesController);
> #endif

Initialization now done in toolbar/menupopup constructors... also set up the controller here (no more shim init function)

Skipping over lots of code ifdefing out for functions that are irrelevant to places... 

>@@ -3114,17 +3096,17 @@ function BrowserToolboxCustomizeDone(aTo
>     window.XULBrowserWindow.init();
>   }
> 
>   // Update the urlbar
>   var url = getWebNavigation().currentURI.spec;
>   if (gURLBar) {
>     gURLBar.value = url;
>     SetPageProxyState("valid");
>-    FeedHandler.updateFeeds();
>+    XULBrowserWindow.asyncUpdateUI();    
>   }

Consolidated UI object updating (see above). 

>@@ -3619,19 +3601,27 @@ nsBrowserStatusHandler.prototype =
>       // Close the Find toolbar if we're in old-style TAF mode
>       gFindBar.closeFindBar();
>     }
> 
>     //fix bug 253793 - turn off highlight when page changes
>     if (document.getElementById("highlight").checked)
>       document.getElementById("highlight").removeAttribute("checked");
> 
>-    setTimeout(function () { FeedHandler.updateFeeds(); }, 0);
>+    var self = this;
>+    setTimeout(function() { self.asyncUpdateUI(); }, 0);
>+  },
>+  
>+  asyncUpdateUI : function () {

The aforementioned consolidated UI Updating.

>+#ifdef MOZ_PLACES
>+      PlacesCommandHook.addLiveBookmark(feeds[0].href);
>+#else
>       this.addLiveBookmark(feeds[0].href);
>+#endif

Call the right function for adding live bookmarks. 

>+#ifndef MOZ_PLACES
>   /**
>    * Adds a Live Bookmark to a feed
>    * @param   url
>    *          The URL of the feed being bookmarked
>    */
>   addLiveBookmark: function(url) {
>     var doc = gBrowser.selectedBrowser.contentDocument;
>     var title = doc.title;
>-#ifndef MOZ_PLACES
>     var description = BookmarksUtils.getDescriptionFromDocument(doc);
>     BookmarksUtils.addLivemark(doc.baseURI, url, title, description);
>-#else
>-      dump("*** IMPLEMENT ME\n");
>-#endif

Remove functions not necessary for places. 

> #ifdef MOZ_PLACES
>-var PlacesBrowserShim = {
>-  _bms: null,  // Bookmark Service
>-  _lms: null,  // Livemark Service
>-  _hist: null, // History Service
...

whack a lot of unused code. 

>+var PlacesCommandHook = {

And rename the object...

>+  bookmarkCurrentPage: function PCH_bookmarkCurrentPage() {
>+    // TODO: add dialog for filing/confirmation
>+    var selectedBrowser = getBrowser().selectedBrowser;
>+    var uri = selectedBrowser.currentURI;
>+    var bms = PlacesController.bookmarks;
>+    bms.insertItem(bms.bookmarksRoot, uri, -1);
>+    bms.setItemTitle(uri, selectedBrowser.contentTitle);
>   },

Consolidate functions, give new symmetric names (bookmarkCurrentPage, bookmarkCurrentPages)

>+   * Get the description associated with a document, as specified in a <META> 
>+   * element.
>+   * @param   doc
>+   *          A DOM Document to get a description for
>+   * @returns A description string if a META element was discovered with a 
>+   *          "description" or "httpequiv" attribute, empty string otherwise.
>+  _getDescriptionFromDocument: function PCH_getDescriptionFromDocument(doc) {
>+    var metaElements = doc.getElementsByTagName("META");
>+    for (var i = 0; i < metaElements.length; ++i) {
>+      if (metaElements[i].localName.toLowerCase() == "description" || 
>+          metaElements[i].httpEquiv.toLowerCase() == "description") {
>+        return metaElements[i].content;
>+        break;

Future helper for determining descriptions, from old bookmarks.js

>+  addLiveBookmark: function PCH_addLiveBookmark(url) {

New add live bookmark function. 

>-  onPageShow: function PBS_onPageShow(e) {
>-  onTabSwitch: function PBS_onTabSwitch(e) {

Don't need to do this separately anymore - called by the browser code itself after pageshow/location change

>+  showPlacesOrganizer: function PCH_showPlacesOrganizer(mode) {
>+    // TODO: check for an existing one and focus it instead. 
>+    openDialog("chrome://browser/content/places/places.xul", 
>+               "", "resizable", mode);
>   },

Consolidated open function for new window. 

>+  updateTagButton: function PCH_updateTagButton() {

Rename the function that updates the tag button's state. Make rely less on shim state and other members. (Shim is static - does not have or should not rely on instance members)

>+  onBookmarkButtonClick: function PCH_onBookmarkButtonClick() {
>+    var currentURI = getBrowser().selectedBrowser.webNavigation.currentURI;
>+    var bms = PlacesController.bookmarks;
>+    if (bms.isBookmarked(currentURI)) {

Just renaming member variables. 

>-/**
>- * This is a custom implementation of nsITransactionManager. We do not chain

Not needed anymore. I have an idea for a simpler way to do this. 

> // Functions for the history menu.
> var HistoryMenu = {
> 
>-  /*
>+  /**
>    * Updates the history menu with the session history of the current tab.
>    * This function is called every time the history menu is shown.
>-   * @params menu XULNode for the history menu
>+   * @param menu 
>+   *        XULNode for the history menu

I am a formatting nazi. 

>Index: browser/base/content/browser.xul
>+<?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>

places.css: the one stop shop for places binding hookup. 

>               <menupopup position="after_end"
>                          onpopupshowing="return FeedHandler.buildFeedList(event);"
>+#ifdef MOZ_PLACES
>+                         oncommand="PlacesCommandHook.addLiveBookmark(event.target.getAttribute('feed'));" />
>+#else
>                          oncommand="FeedHandler.addLiveBookmark(event.target.getAttribute('feed'));" />
>+#endif

Call the right function from the feed icon. 

> #ifdef MOZ_PLACES
>       <toolbarbutton id="bookmarksBarShowPlaces"
>-                     oncommand="PlacesBrowserShim.showHistory()"/>
>+                     oncommand="PlacesCommandHook.showPlacesOrganizer('history');"/>
>       <toolbaritem flex="1" id="personal-bookmarks" title="&bookmarksItem.title;">
>-         <hbox id="bookmarksBarContent" flex="1"
>+         <hbox id="bookmarksBarContent" flex="1" type="places" 

Rename shim object, add type="places" to cause binding to be hooked up. 

>-                     oncommand="PlacesBrowserShim.onBookmarkButtonClick();"/>
>-      <toolbarbutton id="places-subscribe" 
>-                     label="&feed.subscribe.label;" 
>-                     tooltiptext="&feed.subscribe.tooltip;"/>
>+                     oncommand="PlacesCommandHook.onBookmarkButtonClick();"/>

Don't supply our own feed subscription button for now, just use the existing reliable one. 

> #ifdef MOZ_PLACES
>     <toolbar id="PersonalToolbar"
>              class="chromeclass-directories"
>              defaultset="bookmarksBarShowPlaces,personal-bookmarks"
>-             toolbarname="&places.toolbar.label;"
>+             toolbarname="&personalbarCmd.label;" accesskey="&personalbarCmd.accesskey;"

Need right label/accesskey for toolbar now we're not including places.dtd in the browser window anymore. 

>Index: browser/components/places/jar.mn
>+    content/browser/places/organizer.css                 (content/organizer.css)

Add non-API content stylesheet for the organizer window. This is for attaching bindings specific to the organizer window that aren't part of the places view api we expose to extension authors etc. 

>Index: browser/components/places/content/controller.js
>-  get _bms() {
>+  get bookmarks() {

Make this public to prevent lots of duplicate caching (wherever there is places  there is PlacesController anyway) and give it a nicer name. 

>-  get _hist() {
>+  get history() {
>+  get livemarks() {

Ditto.

>   /**
>-   * The top window
>+   * The Transaction Manager for this instance.
>    */
>-  topWindow: null,

This is no longer used since Places is never loaded as a tab. 

>-  
>-  /**
>-   * The Transaction Manager for this window.
>-   */
>-  tm: null,
>+  _tm: null,
>+  get tm() {
>+    if (!this._tm) {
>+      this._tm = 
>+        Cc["@mozilla.org/transactionmanager;1"].
>+        createInstance(Ci.nsITransactionManager);
>+    }
>+    return this._tm;
>+  },

Create our own instance of transaction manager per controller instance. Will document how undo/redo will work soon. 

>   /**
>    * Opens the bookmark properties for the selected URI Node.
>    */
>-
>   showBookmarkPropertiesForSelection: 
>   function PC_showBookmarkPropertiesForSelection() {
>     var node = this._activeView.selectedURINode;
>-    if (!node || !node.uri) return;
>+    if (!node || !node.uri) 
>+      return;

Formatting nazi. 

>Index: browser/components/places/content/menu.xml
>       <constructor><![CDATA[
>+        this.init();
>       ]]></constructor>

Initialize in constructor, rather than rely on someone else to call this. 

>Index: browser/components/places/content/places.css

API defined here. 

>Index: browser/components/places/content/places.js
>-var PlacesUIHook = {

>-    // Hook the browser UI
>-    PlacesUIHook.init(this._content);

>-    
>-    // We attach event listeners like this instead of using the more succinct
>-    // "onpageshow/hide" inline handlers because they don't work for XUL. See:
>-    //   https://bugzilla.mozilla.org/show_bug.cgi?id=326260
>-    function onPageShow() {
>-      PlacesUIHook.showPlacesUI();
>-    }
>-    function onPageHide() {
>-      PlacesUIHook.hidePlacesUI();
>-    }
>-    window.addEventListener("pageshow", onPageShow, false);
>-    window.addEventListener("pagehide", onPageHide, false);

No more browser integration crap!

>     // Now load the appropriate folder in the Content View, and select the 
>     // corresponding entry in the Places View. This is a little fragile. 
>-    var params = window.location.search;
>-    var index = params == "?history" ? INDEX_HISTORY : INDEX_BOOKMARKS;
>+    var params = window.arguments[0];
>+    var index = params == "history" ? INDEX_HISTORY : INDEX_BOOKMARKS;

window now opened with arg "history" or "bookmarks"...

>Index: browser/components/places/content/places.xul
>+        windowtype="Places" 

For locating duplicate windows

>         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-        onload="PlacesPage.init();">
>+        onload="PlacesOrganizer.init();"
>+        width="700" height="500" screenX="10" screenY="10"
>+        persist="width height screenX screenY sizeMode">

Sizing. 

>+  <toolbox>
>+    <menubar id="placesMenubar">
>+      <menu id="fileMenu" label="&file.label;" accesskey="&file.accesskey;">
>+        <menupopup>
>+          <menuitem label="&fileClose.label;" accesskey="&fileClose.accesskey;" 
>+                    oncommand="window.close();"/>
>+        </menupopup>
>+      </menu>
>+    </menubar>
>+  </toolbox>

Stub menu. 

>Index: browser/components/places/content/toolbar.xml
>       <constructor><![CDATA[
>+        this.init();
>       ]]></constructor>

See menu comment. 

>Index: browser/themes/pinstripe/browser/places/places.css
>===================================================================
>RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/places/places.css,v
>retrieving revision 1.1
>diff -p -u -8 -r1.1 places.css
>--- browser/themes/pinstripe/browser/places/places.css	24 Feb 2006 21:41:25 -0000	1.1
>+++ browser/themes/pinstripe/browser/places/places.css	8 Mar 2006 01:58:07 -0000
>@@ -1,12 +1,10 @@
> /* Root View */
> #placesView {
>-  -moz-appearance: tabpanels;
>-  margin: -10px;

Appearance.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Or more globally, this patch does several things on the path to browser happiness:

- tidies up some UI update notification in browser
- fixes up a few menu items here and there
- removes unnecessary initialization
- moves places to its own window
- removes all code to do with maintaining places as a tab
- renames some objects, member variables
- formatting nazi

Comment 7

[Annie Sullivan]

Comment on attachment 214386 [details] [diff] [review]
merged patch

>+#ifndef MOZ_PLACES
> function UpdateBookmarkAllTabsMenuitem()

This function sets the Browser:BookmarkAllTabs menuitem to enabled/disabled based on whether there are multiple tabs open.  The menu still uses that command; shouldn't this function be called onpopupshowing instead of getting #ifdef-ed out?  I don't see it moved/renamed anywhere.

>-  // XXXben - move this to the toolbar constructor if there is no performance penalty! (Ts/Txul)
>-  var bookmarksBar = document.getElementById("bookmarksBarContent");
>-  bookmarksBar.init();
>-  var bookmarksMenuPopup = document.getElementById("bookmarksMenuPopup");
>-  bookmarksMenuPopup.init();
>+  window.controllers.appendController(PlacesController);

So there's definitely no performance penalty for moving this code into the constructors?  Because I just realized that calling bookmarsBar.init() in BrowserToolboxCustomizeDone() should fix the problems with customization without being too hacky.

>+  addLiveBookmark: function PCH_addLiveBookmark(url) {
>+    var ios = 
>+        Cc["@mozilla.org/network/io-service;1"].
>+        getService(Ci.nsIIOService);
>+    var feedURI = ios.newURI(url, null, null);
>+    
>+    var browser = gBrowser.selectedBrowser;
>+    
>+    // TODO: implement description annotation
>+    //var description = this._getDescriptionFromDocument(doc);
>+    // TODO: add dialog for filing/confirmation
>+    var bms = PlacesController.bookmarks;
>+    var livemarks = PlacesController.livemarks;
>+    livemarks.createLivemark(bms.toolbarRoot, title, browser.currentURI, 
>+                             feedURI, -1);

The site URI here changed from gBrowser.selectedBrowser.contentDocument.baseURI to gBrowser.selectedBrowser.currentURI -- why?  Also, I don't see title defined anywhere; it used to be gBrowser.selectedBrowser.contentDocument.title.

>     if (!this._groupableView)
>-      return;
>-    var query = this._hist.getNewQuery();
>-    var options = this._hist.getNewQueryOptions();
>+      return null;
>+    var query = this.history.getNewQuery();
>+    var options = this.history.getNewQueryOptions();

This function doesn't return a value normally, so there is a JavaScript warning when you return null here.  What's the reasoning for putting the return null back in?

>   <binding id="places-menupopup" 
>            extends="chrome://global/content/bindings/popup.xml#popup">
>     <implementation>
>       <constructor><![CDATA[
>+        this.init();
>       ]]></constructor>

If we're going to keep the init() functions in the constructors, the menu init() function should bail out immediately if there is no places attribute set on the menupopup.  Otherwise, the submenus will break and/or run lots of extra queries.

>Index: browser/components/places/content/places.xul
> <?xml-stylesheet href="chrome://browser/content/places/places.css"?>
>+<?xml-stylesheet href="chrome://browser/content/places/organizer.css"?>

I don't see the organizer.css file in this patch.  I assume it just has the things you took out of browser/content/places.css?

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch, addresses issues

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

(In reply to comment #7)
> (From update of attachment 214386 [details] [diff] [review] [edit])
> >+#ifndef MOZ_PLACES
> > function UpdateBookmarkAllTabsMenuitem()
> 
> This function sets the Browser:BookmarkAllTabs menuitem to enabled/disabled
> based on whether there are multiple tabs open.  

So I fixed this in an interesting way (see the new patch). I created a BrowserController object that i'd eventually like to move more commands into. The way it does it using the popupshowing handler is potentially flaky. Using the controller system allows us to handle updating of commands in a centralized fashion. I will write more on this to the list later on. 

There is one small issue with the fix here, when you close the last tab the command remains enabled until the user selects text somewhere, because a select event doesn't fire in that case. As soon as mconnor lands the fix for tab events, I can change the select event to a TabChange event and this will be fixed. 
 
> >-  // XXXben - move this to the toolbar constructor if there is no performance penalty! (Ts/Txul)
> >-  var bookmarksBar = document.getElementById("bookmarksBarContent");
> >-  bookmarksBar.init();
> >-  var bookmarksMenuPopup = document.getElementById("bookmarksMenuPopup");
> >-  bookmarksMenuPopup.init();
> >+  window.controllers.appendController(PlacesController);
> 
> So there's definitely no performance penalty for moving this code into the
> constructors?  Because I just realized that calling bookmarsBar.init() in
> BrowserToolboxCustomizeDone() should fix the problems with customization
> without being too hacky.

If it regresses things, I'll back it out and revert the other behavior. I'm going to see if I can run timing here on windows as well. In general I'd like to reduce the number of places in browser.js where you need to add special reinitialization, simply because it's a pain and is hard to deal with/grok. Having elements that initialize themselves and a centralized controller system suggests a cleaner architecture that is easier to hack on. 

> The site URI here changed from gBrowser.selectedBrowser.contentDocument.baseURI
> to gBrowser.selectedBrowser.currentURI -- why?  Also, I don't see title defined
> anywhere; it used to be gBrowser.selectedBrowser.contentDocument.title.

Fixed title. .contentDocument.baseURI == .currentURI, except currentURI is a nsIURI, which is what createLivemark expects, so no extra translation step. 

> >+      return null;
> >+    var query = this.history.getNewQuery();
> >+    var options = this.history.getNewQueryOptions();
> 
> This function doesn't return a value normally, so there is a JavaScript warning
> when you return null here.  What's the reasoning for putting the return null
> back in?

No reason, good find. Merge conflict I guess. Fixed. 

All other issues fixed. new patch is -N, so you'll find organizer.css 

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

landed branch annd trunk. watching for ts adjustments

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

landed branch annd trunk. watching for ts adjustments

Comment 12

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv