Closed
Bug 407468
Opened 17 years ago
Closed 16 years ago
Livemark loading and failed to load messages should be static menuitems from menu.xml
Categories
(Firefox :: Bookmarks & History, defect, P4)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3.1b1
People
(Reporter: philor, Assigned: mak)
References
Details
Attachments
(1 file, 8 obsolete files)
|
27.15 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
Currently, we create the "Live bookmark loading..." and "Live bookmark failed to load" messages by creating bookmark items with URLs like about:livemark-loading, which requires that everything (including extensions) that deals with bookmarks remembers to exclude them when they aren't appropriate, and results in gymnastics like bug 392984 to manage them, and causes bug 333416 and bug 278247 and the others that I remember being around in the dusty corners, because we don't exclude them when it comes to loading, so we try to load "about:livemark-failed" and throw a dialog at the poor user who wondered why his feed was broken.
If instead they were just static items created by the menu binding, managing them would just be a matter of setting and unsetting an annotation on the feed, and doing either something useful or nothing at all when they are clicked would be simple.
Flags: blocking-firefox3?
|
||
Updated•17 years ago
|
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Priority: -- → P4
Target Milestone: --- → Firefox 3 M11
|
||
Comment 1•17 years ago
|
||
Attachment #298979 -
Flags: review?(mano)
|
||
Comment 2•17 years ago
|
||
Comment on attachment 298979 [details] [diff] [review]
patch v1
>Index: browser/components/places/content/menu.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/menu.xml,v
>retrieving revision 1.95
>diff -u -p -8 -r1.95 menu.xml
>--- browser/components/places/content/menu.xml 9 Jan 2008 06:21:09 -0000 1.95
>+++ browser/components/places/content/menu.xml 24 Jan 2008 18:15:08 -0000
>@@ -108,16 +108,23 @@
> this._ensureInitialized();
> return this._resultNode;
> ]]></body>
> </method>
>
> <method name="_cleanMenu">
> <parameter name="aPopup"/>
> <body><![CDATA[
>+ // Remove the special and static 'loading' & 'loading failed'
>+ // menuitems manually
>+ if (aPopup._loadingItem)
>+ aPopup.removeChild(aPopup._loadingItem);
>+ if (aPopup._failedItem)
>+ aPopup.removeChild(aPopup._failedItem);
>+
Thinking about this more, It would be better to do this in _rebuild, there you should also avoid removing and re-creating these elements if they're still needed.
Attachment #298979 -
Flags: review?(mano) → review-
|
|
||
Comment 3•17 years ago
|
||
Attachment #298979 -
Attachment is obsolete: true
Attachment #300325 -
Flags: review?(mano)
|
|
||
Comment 4•17 years ago
|
||
Comment on attachment 300325 [details] [diff] [review]
v1.1
Index: browser/components/places/content/menu.xml
===================================================================
RCS file: /cvsroot/mozilla/browser/components/places/content/menu.xml,v
retrieving revision 1.97
diff -u -p -8 -r1.97 menu.xml
--- browser/components/places/content/menu.xml 27 Jan 2008 05:36:44 -0000 1.97
+++ browser/components/places/content/menu.xml 30 Jan 2008 08:56:50 -0000
@@ -217,16 +217,58 @@
]]></body>
</method>
<method name="_rebuild">
<parameter name="aPopup"/>
<body><![CDATA[
this._cleanMenu(aPopup);
+ // The following will determine whether the 'Loading...'
All of this should be moved to a helper method in PlacesUtils, which would take
a folder popup and create/remove the menuitems.
+ // or 'Loading Failed' menuitems need to be added and/or removed,
+ // by checking the relevant annotations.
+ // Since the startMarker is reset to -1 in the _cleanMenu function,
+ // decreasing it's value will not be needed when removing a menuitem.
+ if (PlacesUtils.nodeIsLivemarkContainer(aPopup._resultNode)) {
+ var itemId = aPopup._resultNode.itemId;
nit: indent.
+ if (!aPopup._failedItem &&
+ PlacesUtils.annotations.itemHasAnnotation(itemId,
+ "livemark/loadfailed")) {
+ aPopup._failedItem = document.createElement("menuitem");
+ aPopup._failedItem.setAttribute("label",
+ PlacesUtils.getString("bookmarksLivemarkFailed"));
+ aPopup._failedItem.setAttribute("disabled", true);
+ aPopup.appendChild(aPopup._failedItem);
+ aPopup._startMarker++;
+ }
+ else if (aPopup._failedItem &&
+ !PlacesUtils.annotations.itemHasAnnotation(itemId,
+ "livemark/loadfailed")) {
+ aPopup.removeChild(aPopup._failedItem);
+ aPopup._failedItem = null;
+ }
+
+ if (!aPopup._loadingItem &&
+ PlacesUtils.annotations.itemHasAnnotation(itemId,
+ "livemark/loading")) {
you should avoid checking both annotations if possible (meaningfully, make this
an |else if| block and null out the loading item elsewhere)
+ aPopup._loadingItem = document.createElement("menuitem");
+ aPopup._loadingItem.setAttribute("label",
+ PlacesUtils.getString("bookmarksLivemarkLoading"));
+ aPopup._loadingItem.setAttribute("disabled", true);
+ aPopup.appendChild(aPopup._loadingItem);
+ aPopup._startMarker++;
+ }
+ else if (aPopup._loadingItem &&
+ !PlacesUtils.annotations.itemHasAnnotation(itemId,
+ "livemark/loading")) {
+ aPopup.removeChild(aPopup._loadingItem);
+ aPopup._loadingItem = null;
+ }
+ }
+
nit: trailing spaces.
var cc = aPopup._resultNode.childCount;
if (cc > 0) {
if (aPopup._emptyMenuItem)
aPopup._emptyMenuItem.hidden = true;
for (var i = 0; i < cc; ++i) {
var child = aPopup._resultNode.getChild(i);
this.insertNewItem(child, aPopup, null);
@@ -340,19 +382,62 @@
}
else
menuitem.removeAttribute("image");
var title = aNode.title;
if (menuitem.getAttribute("label") != title)
menuitem.setAttribute("label", title);
- if (!menuitem.hasAttribute("livemark") &&
- PlacesUtils.nodeIsLivemarkContainer(aNode))
- menuitem.setAttribute("livemark", "true");
+ // The following will add and remove the 'Loading Failed'
+ // and 'Loading ...' menuitems. Additionally, the startMarker
+ // is updated accordingly.
+ if (PlacesUtils.nodeIsLivemarkContainer(aNode)) {
+ if (!menuitem.hasAttribute("livemark"))
+ menuitem.setAttribute("livemark", "true");
+
+ var fChild = menuitem.firstChild;
s/fChild/popup/ maybe?
+ if (!fChild._loadingItem &&
+ PlacesUtils.annotations.itemHasAnnotation(aNode.itemId,
+ "livemark/loading")) {
+ fChild._loadingItem = document.createElement("menuitem");
+ fChild._loadingItem.setAttribute("label",
+ PlacesUtils.getString("bookmarksLivemarkLoading"));
+ fChild._loadingItem.setAttribute("disabled", true);
+ fChild.insertBefore(fChild._loadingItem,
+ fChild.childNodes[fChild._startMarker + 1]);
+ fChild._startMarker++;
+ }
+ else if (fChild._loadingItem &&
+ !PlacesUtils.annotations.itemHasAnnotation(aNode.itemId,
it would be good to read aNode.itemId just once.
Index: toolkit/components/places/src/nsLivemarkService.js
===================================================================
@@ -223,21 +220,19 @@ LivemarkService.prototype = {
this._updateLivemarkChildren(i, false);
}
},
deleteLivemarkChildren: function LS_deleteLivemarkChildren(folderId) {
this._bms.removeFolderChildren(folderId);
},
- insertLivemarkLoadingItem: function LS_insertLivemarkLoading(bms, livemark) {
- var loadingURI = gIoService.newURI("about:livemark-loading", null, null);
- if (!livemark.loadingId || livemark.loadingId == -1)
- livemark.loadingId = bms.insertBookmark(livemark.folderId, loadingURI,
- 0, this._loading);
+ insertLivemarkLoadingItem: function LS_insertLivemarkLoading(livemark) {
+ this._ans.setItemAnnotation(livemark.folderId,
+ LMANNO_LOADING, true, 0, this._ans.EXPIRE_NEVER);
This method should be rename as it no longer adds an item (maybe markLivemarkAsLoading?)
if (this._isAborted) {
- if (this._livemark.loadingId != -1) {
- this._bms.removeItem(this._livemark.loadingId);
- this._livemark.loadingId = -1;
- }
+ if (this._ans.itemHasAnnotation(this._livemark.folderId, LMANNO_LOADING))
+ this._ans.removeItemAnnotation(this._livemark.folderId, LMANNO_LOADING);
You can call removeItemAnnotation without checking that.
+ if (this._ans.itemHasAnnotation(this._livemark.folderId, LMANNO_LOADING))
+ this._ans.removeItemAnnotation(this._livemark.folderId, LMANNO_LOADING);
here too.
Attachment #300325 -
Flags: review?(mano) → review-
|
|
||
Comment 5•17 years ago
|
||
Attachment #300325 -
Attachment is obsolete: true
Attachment #300516 -
Flags: review?(mano)
|
|
||
Comment 6•17 years ago
|
||
Mano: I have made all the necessary adjustments to the code, except for one.
You mentioned the following:
"you should avoid checking both annotations if possible (meaningfully, make this
an |else if| block and null out the loading item elsewhere)"
But, in some instances - both the Loading & Loading Failed annotations exist, thus exists the need to create both menu items. For this reason, I must check both annotations.
|
|
||
Comment 7•17 years ago
|
||
oops... I forgot to include nsLivemarkService in the last diff. sorry :)
Attachment #300516 -
Attachment is obsolete: true
Attachment #300518 -
Flags: review?(mano)
Attachment #300516 -
Flags: review?(mano)
|
|
||
Comment 8•17 years ago
|
||
Same deal, less code redundancy..
Attachment #300518 -
Attachment is obsolete: true
Attachment #301584 -
Flags: review?(mano)
Attachment #300518 -
Flags: review?(mano)
|
|
||
Comment 9•17 years ago
|
||
+ ensureLivemarkStatusMenuItems: function(aPopup, aItemId, aIsItemChanged) {
+ // The following will determine whether the 'Loading...'
+ // or 'Loading Failed' menuitems need to be added and/or removed,
+ // by checking the relevant annotations.
+
+ if (this.nodeIsLivemarkContainer(aPopup._resultNode) || aIsItemChanged) {
I don't understand the || here. What do you actually want to do/not to do on-item-change? Why do you want to do so even if the popup isn't a livemark container?
|
|
||
Comment 10•17 years ago
|
||
The reason I checked the aIsItemChanged, is because it will only by true if it received a livemark container. During both (toolbar & menu) itemChanged, an if statement precedes the ensureLivemarkStatusMenuItems :
+ if (PlacesUtils.nodeIsLivemarkContainer(aNode)) {
+ if (!menuitem.hasAttribute("livemark"))
+ menuitem.setAttribute("livemark", "true");
+
+ PlacesUtils.ensureLivemarkStatusMenuItems(menuitem.firstChild,
+ aNode.itemId, true);
+ }
This means, that if the function was called during an itemChaned event, it is surely a livemark container.
|
|
||
Comment 11•17 years ago
|
||
Mano: Thank you so much for your reviews!
- Same deal, more comments!
Attachment #301584 -
Attachment is obsolete: true
Attachment #301714 -
Flags: review?(mano)
Attachment #301584 -
Flags: review?(mano)
|
|
||
Comment 12•17 years ago
|
||
Comment on attachment 301714 [details] [diff] [review]
v1.5
Reviewed over IM.
Attachment #301714 -
Flags: review?(mano) → review-
|
||
Updated•17 years ago
|
Target Milestone: Firefox 3 beta3 → ---
|
|
||
Updated•17 years ago
|
Status: NEW → ASSIGNED
|
|
||
Updated•17 years ago
|
Assignee: nobody → dev
Status: ASSIGNED → NEW
|
|
||
Comment 13•17 years ago
|
||
Since I moved the creation of the static menuitems to PlacesUtils (utils.js), the reterieval of both strings (loading & failed) is being made through PlacesUtils.getString. This is a problem for me, because bug 415117 had moved both strings to a different string bundle!
So, should I:
a. Move them back to the original string bundle?
b. Ignore PlacesUtils.getString function, and simply retrieve both strings my self from the appropriate string bundle?
Status: NEW → ASSIGNED
|
|
||
Comment 14•17 years ago
|
||
Updated patch.
Note: In some instances, both the "Loading" and "Loading Failed" menu items appear. This also happens prior to this patch, which leads me to believe that it is another bug all together.
Attachment #301714 -
Attachment is obsolete: true
Attachment #303729 -
Flags: review?(mano)
|
|
||
Comment 15•17 years ago
|
||
Michael: current utils.js uses the browser/ file, which is the one you've moved the strings to, haven't you?
|
|
||
Comment 16•17 years ago
|
||
Yes, I had moved the strings from the toolkit/ file to the browser/ file.
|
Assignee | |
Comment 17•17 years ago
|
||
Michael, sorry for that, but Bug 419832 wil unbitrot this :(
|
|
||
Comment 18•17 years ago
|
||
Marco: Don't worry about it.. Thats been happening a lot lately, and that is also why I have been holding off with this bug for a while...
|
|
Assignee | |
Comment 19•17 years ago
|
||
Michael, do you have time slot to update this? I could probably bring on your work if you need, i'd like to have this into final
|
|
||
Comment 20•17 years ago
|
||
Marco: I'm on vacation in Phoenix until next week. So either I'll get it done by next Thursday, or if it's too late and you've got the time and will, feel free to take this bug...
|
|
Assignee | |
Comment 21•17 years ago
|
||
Comment on attachment 303729 [details] [diff] [review]
v1.6
obsoleted (bitrotted)
Attachment #303729 -
Attachment is obsolete: true
Attachment #303729 -
Flags: review?(mano)
|
|
Assignee | |
Comment 23•16 years ago
|
||
for now i've unbitrotted the full thing, much code has changed.
this is based on the new markers code from bug 418671, that should ensure markers are correctly updated. I can tell this works however is not a finalized patch
Assignee: dev → mak77
|
|
Assignee | |
Comment 24•16 years ago
|
||
notice this requires a string move from toolkit to browser so we could try to put this in before the beta 1 string freeze on 26th
|
|
Assignee | |
Comment 25•16 years ago
|
||
i've sone some cleanup, we were setting annos in too many points and we need only one status menuitem (the feed should be in status failed or loading, not both).
Attachment #339465 -
Attachment is obsolete: true
Attachment #339570 -
Flags: review?(mano)
|
|
Assignee | |
Comment 26•16 years ago
|
||
PS: this fixed places transaction unit test that was not counting the "loading" item
|
|
||
Updated•16 years ago
|
Attachment #339570 -
Flags: review?(mano) → review+
|
|
||
Comment 27•16 years ago
|
||
Comment on attachment 339570 [details] [diff] [review]
v1.8
r=mano, thanks.
|
|
Assignee | |
Comment 28•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Updated•16 years ago
|
Target Milestone: --- → Firefox 3.1b1
|
||
Comment 30•15 years ago
|
||
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.
Description
•