bookmarkProperties cleanup

RESOLVED FIXED in Firefox 3 alpha3

Status

()

P2
normal
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Trunk
Firefox 3 alpha3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Blocks: 357316
Summary: bookmarkProperties code cleanup → bookmarkProperties cleanup
Comment on attachment 256563 [details] [diff] [review]
patch

>   /**
>    * This method returns the correct title for the current variant
>    * of this dialog.
>    */
>   _getDialogTitle: function BPP__getDialogTitle() {
>-    switch(this._variant) {
>-    case this.ADD_BOOKMARK_VARIANT:
>-      return this._strings.getString("dialogTitleAdd");
>-    case this.EDIT_FOLDER_VARIANT:
>+    if (this._action == ACTION_ADD) {
>+      if (this._itemType == BOOKMARK_ITEM)
>+        return this._strings.getString("dialogTitleAdd");
>+      // Not yet supported, but the string exsits

nit: "exists"

>   /**
>    * This method can be run on a URI parameter to ensure that it didn't
>    * receive a string instead of an nsIURI object.
>    */
>   _assertURINotString: function BPP__assertURINotString(value) {
>     NS_ASSERT((typeof(value) == "object") && !(value instanceof String),
>     "This method should be passed a URI as a nsIURI object, not as a string.");
>   },

Should this move to PlacesUtils, since it is duplicated here and in the controller?

>+   * @param aIdentifier
>+   *        the URI or folder ID to display the properties for
>+   * @param aAction
>+   *        "add" if this is being triggered from an "add bookmark"
>+   *        UI action; or "editfolder" or "edititem" if this is being
>+   *        triggered from a "properties" UI action; or "addmulti" if
>+   *        we're trying to create multiple bookmarks.
>+   *
>+   */
>+  _determineVariant: function BPP__determineVariant(aIdentifier, aAction) {
>+    if (aAction == "add") {
>+      this._assertURINotString(aIdentifier);
>+      this._action = ACTION_ADD;
>+      this._itemType = BOOKMARK_ITEM;
>+    }
>+    else if (aAction == "addmulti") {
>+      this._action = ACTION_ADD_WITH_ITEMS
>+      this._itemType = BOOKMARK_FOLDER;
>+    }
>+    if (typeof(aIdentifier) == "number") {

Should this be an |else if|? Assuming bad input, we'd hit this codepath.

>   //XXXDietrich - bug 370215 - update to use bookmark id once 360133 is fixed.
>   _rebuildMicrosummaryPicker: function BPP__rebuildMicrosummaryPicker() {
>-    var microsummaryMenuList = document.getElementById("microsummaryMenuList");
>-    var microsummaryMenuPopup = document.getElementById("microsummaryMenuPopup");
>+    var microsummaryMenuList = this._element("microsummaryMenuList");
>+    var microsummaryMenuPopup = this._element("microsummaryMenuPopup");
> 
>     // Remove old items from the menu, except the first item, which represents
>     // "don't show a microsummary; show the page title instead".
>-    while (microsummaryMenuPopup.childNodes.length > 1)
>+    while (microsummaryMenuPopup.hasChildNodes())
>       microsummaryMenuPopup.removeChild(microsummaryMenuPopup.lastChild);

wouldn't this change remove the item that the comment is referring to?

r=me with these comments addressed or fixed
Attachment #256563 - Flags: review?(dietrich) → review+
(In reply to comment #2)

> >   /**
> >    * This method can be run on a URI parameter to ensure that it didn't
> >    * receive a string instead of an nsIURI object.
> >    */
> >   _assertURINotString: function BPP__assertURINotString(value) {
> >     NS_ASSERT((typeof(value) == "object") && !(value instanceof String),
> >     "This method should be passed a URI as a nsIURI object, not as a string.");
> >   },
> 
> Should this move to PlacesUtils, since it is duplicated here and in the
> controller?

I'm planning to remove it from both places at some point.

> >+  _determineVariant: function BPP__determineVariant(aIdentifier, aAction) {
> >+    if (aAction == "add") {
> >+      this._assertURINotString(aIdentifier);
> >+      this._action = ACTION_ADD;
> >+      this._itemType = BOOKMARK_ITEM;
> >+    }
> >+    else if (aAction == "addmulti") {
> >+      this._action = ACTION_ADD_WITH_ITEMS
> >+      this._itemType = BOOKMARK_FOLDER;
> >+    }
> >+    if (typeof(aIdentifier) == "number") {
> 
> Should this be an |else if|? Assuming bad input, we'd hit this codepath.
> 
hrm, yeah, thanks.

> > 
> >     // Remove old items from the menu, except the first item, which represents
> >     // "don't show a microsummary; show the page title instead".
> >-    while (microsummaryMenuPopup.childNodes.length > 1)
> >+    while (microsummaryMenuPopup.hasChildNodes())
> >       microsummaryMenuPopup.removeChild(microsummaryMenuPopup.lastChild);
> 
> wouldn't this change remove the item that the comment is referring to?
> 
and thanks for catching that too.
Created attachment 256808 [details] [diff] [review]
comments addressed
Attachment #256563 - Attachment is obsolete: true
Attachment #256563 - Flags: review?(sspitzer)
mozilla/browser/components/places/content/bookmarkProperties.js 1.31
mozilla/browser/components/places/content/bookmarkProperties.xul 1.16
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.