Closed
Bug 591344
Opened 14 years ago
Closed 14 years ago
Star button fixes from SeaMonkey review
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: kairo, Assigned: kairo)
References
Details
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
mak
:
review+
benjamin
:
approval2.0+
|
Details | Diff | Splinter Review |
As pretty much always, Neil has review comments on the SeaMonkey bookmarking button in bug 589601 that should be backported to the star button in FF.
|
Assignee | |
Comment 1•14 years ago
|
||
Here's the patch fixing those nits - see the original bug for explanantions.
Attachment #469886 -
Flags: review?(mak77)
|
||
Comment 2•14 years ago
|
||
Comment on attachment 469886 [details] [diff] [review]
patch: fix a few nits
>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
> onItemRemoved: function PSB_onItemRemoved(aItemId, aFolder, aIndex,
> aItemType) {
>- if (!this._batching)
>+ if (!this._batching && this._starred)
> this.updateState();
> },
this looks wrong for now, livemarks children for example are bookmarks and can get onItemRemoved, but they are not marked as starred.
Add a comment about this instead, to clarify code.
r=me with this fixed.
|
|
||
Updated•14 years ago
|
Attachment #469886 -
Flags: review?(mak77) → review+
|
|
Assignee | |
Comment 3•14 years ago
|
||
Requesting approval - this is very low risk nitfixing.
Attachment #469886 -
Attachment is obsolete: true
Attachment #471593 -
Flags: review+
Attachment #471593 -
Flags: approval2.0?
|
||
Comment 4•14 years ago
|
||
(In reply to comment #2)
> (From update of attachment 469886 [details] [diff] [review])
> >diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
> > onItemRemoved: function PSB_onItemRemoved(aItemId, aFolder, aIndex,
> > aItemType) {
> >- if (!this._batching)
> >+ if (!this._batching && this._starred)
> > this.updateState();
> > },
> this looks wrong for now, livemarks children for example are bookmarks and can
> get onItemRemoved, but they are not marked as starred.
Sure, but who cares? We only care if the current URL is removed.
|
|
||
Comment 5•14 years ago
|
||
(In reply to comment #4)
> Sure, but who cares? We only care if the current URL is removed.
after a brief talk with Neil, since the new check skips updateState that is expensive, the first version will be fine, we indeed don't care of unstarring unstarred items.
|
|
||
Updated•14 years ago
|
Attachment #469886 -
Attachment is obsolete: false
|
|
||
Updated•14 years ago
|
Attachment #469886 -
Flags: approval2.0?
|
|
||
Updated•14 years ago
|
Attachment #471593 -
Attachment is obsolete: true
Attachment #471593 -
Flags: approval2.0?
|
||
Updated•14 years ago
|
Attachment #469886 -
Flags: approval2.0? → approval2.0+
|
|
||
Comment 6•14 years ago
|
||
this is not worth taking anymore since bug 613477 is going to change most of this code.
But I'll go through these changes and add them back to bug 591344 where they are still applicable, in next iteration of that patch.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
|
|
||
Comment 7•14 years ago
|
||
sorry, I meant "...add them back to bug 613477 where..."
You need to log in
before you can comment on or make changes to this bug.
Description
•