Fennec should not create bookmarks without a name.

RESOLVED FIXED

Status

Fennec Graveyard
Bookmarks
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Chwiej Pawel, Assigned: mfinkle)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090223 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090317 Fennec/1.0b1




Reproducible: Always

Steps to Reproduce:
1. Navigate to any page.
2. Open right control panel.
3. Invoke add bookmark dialog.
4. Clear name field and click 'Done'.
Actual Results:  
Bookmark is added.

Expected Results:  
Adding bookmarks without a name shouldn't be possible.
(Assignee)

Updated

9 years ago
tracking-fennec: --- → ?

Updated

9 years ago
Status: UNCONFIRMED → NEW
tracking-fennec: ? → 1.0b2+
Ever confirmed: true

Updated

9 years ago
Assignee: nobody → mark.finkle
Created attachment 373352 [details] [diff] [review]
patch

This patch handles name and URI validation in the bookmark editor panel and the bookmark list:
* if name is blank, we make it the URI of the bookmark
* if URI is blank, we make it the previous URI

Also has a small syntax fix I found.
Attachment #373352 - Flags: review?(gavin.sharp)
Created attachment 373366 [details] [diff] [review]
patch 2

This patch tries to touch the DOM less during a save, but I had to actually touch the DOM more, in browser-ui.js, because of some bookmarklist/bookmarkeditor sync issues
Attachment #373352 - Attachment is obsolete: true
Attachment #373366 - Flags: review?(gavin.sharp)
Attachment #373352 - Flags: review?(gavin.sharp)
Comment on attachment 373366 [details] [diff] [review]
patch 2

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+              // If the URI was updated change it in the bookmark, but don't
>+              // allow a blank URI. Revert to previous URI if blank.
>+              let spec = this.spec || this._uri.spec
>+              if (this._uri.spec != spec) {
>+                this._uri = this._ioService.newURI(spec, null, null);
>                 PlacesUtils.bookmarks.changeBookmarkURI(this._itemId, this._uri);
>               }
>+              this.spec = this._uri.spec;

This can just be:

let spec = this.spec;
if (spec && this._uri.spec != spec) {
  this._uri = this._ioService.newURI(spec, null, null);
  this.spec = this._uri.spec;
  PlacesUtils.bookmarks.changeBookmarkURI(this._itemId, this._uri);
}

right? r=me with that.
Attachment #373366 - Flags: review?(gavin.sharp) → review+
Created attachment 373840 [details] [diff] [review]
patch 3 - create/destroy placeitem and fix tags

The previous code kept the <placeitem> around and reused it, but this caused messy code that needed to "reset" the placeitem into it's XBL fresh state. This patch creates and destroys the <placeitem>, just like the bookmark list. We might get a Ts gain from this too, depending on how XBL binds to hidden tags.

This patch also fixes tags, which weren't passed into the editor panel.
Attachment #373366 - Attachment is obsolete: true
Attachment #373840 - Flags: review?(gavin.sharp)
Created attachment 373847 [details] [diff] [review]
patch 4 - with gavin's suggestion

This patch is the same as the last, but uses Gavin's suggestion to minimize the xpconnect roundtrips (we skip some .getAttribute calls)
Attachment #373840 - Attachment is obsolete: true
Attachment #373847 - Flags: review?(gavin.sharp)
Attachment #373840 - Flags: review?(gavin.sharp)
Attachment #373847 - Flags: review?(gavin.sharp) → review+
Comment on attachment 373847 [details] [diff] [review]
patch 4 - with gavin's suggestion

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+              if (spec && this._uri.spec != spec) {
>+                this._uri = this._ioService.newURI(spec, null, null);

Maybe we should actually catch failures here if the user enters an invalid URI? Not a new issue...

>+              if (spec != this._uri.spec)
>+                this.spec = this._uri.spec;

Could set spec = this.spec = this._uri.spec here, and use the local below:

>+            // Update the name and use the URI if name is blank
>+            this.name = this.name || this._uri.spec

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+    document.getElementById("bookmark-form").appendChild(this._editor);

Worth caching bookmark-form?

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>-        <placeitem id="bookmark-edit" type="bookmark" flex="1" ui="manage"
>-                   onmove="FolderPicker.show(this);"
>-                   onclose="BookmarkHelper.close()"
>-                   onchange="this.save()"/>

The onchange handler wasn't added to edit() like onmove and onclose were, is it not needed?
(In reply to comment #6)
> (From update of attachment 373847 [details] [diff] [review])
> >diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
> 
> >+              if (spec && this._uri.spec != spec) {
> >+                this._uri = this._ioService.newURI(spec, null, null);
> 
> Maybe we should actually catch failures here if the user enters an invalid URI?
> Not a new issue...

Added a try/catch that falls back to previous valid URL

> >+              if (spec != this._uri.spec)
> >+                this.spec = this._uri.spec;
> 
> Could set spec = this.spec = this._uri.spec here, and use the local below:
> 
> >+            // Update the name and use the URI if name is blank
> >+            this.name = this.name || this._uri.spec

Done.

> 
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >+    document.getElementById("bookmark-form").appendChild(this._editor);
> 
> Worth caching bookmark-form?

There are more than that one that could be cached. I'll file a new bug for the general case.

> >-        <placeitem id="bookmark-edit" type="bookmark" flex="1" ui="manage"
> >-                   onmove="FolderPicker.show(this);"
> >-                   onclose="BookmarkHelper.close()"
> >-                   onchange="this.save()"/>
> 
> The onchange handler wasn't added to edit() like onmove and onclose were, is it
> not needed?

Not needed and bug 486082 will need to address the close/save aspects too.


http://hg.mozilla.org/mobile-browser/rev/fba0af7a87ce
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.