instead of exposing bookmarklets attribute, expose scheme in uri nodes

RESOLVED FIXED in Firefox 3.6a1

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: mak, Assigned: ShareBird)

Tracking

(Blocks 1 bug, {fixed1.9.1})

Trunk
Firefox 3.6a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Gavin thinks would be far more useful to expose the scheme as an attribute for uri nodes.
The main issue is we don't have a nsIURI object, and building one has a certain overhead, so we could provide an util (i suggest PlacesUIUtils::GuessURISchemeForUI() to extract the scheme for UI purposes, using only string methods.

from a first perf comparison the fastest way to do that looks like:
var scheme = url.substr(0, url.indexOf(":"))
Pardal do you want to take care of this since you originally worked on bookmarklets patch?
Assignee: nobody → pardal
Posted patch patch (obsolete) — Splinter Review
proposed patch
Attachment #366459 - Flags: review?(mak77)
Blocks: 482166
Comment on attachment 366459 [details] [diff] [review]
patch

notice this patch should apply upon the previous one (bookmarklets), so please update your sources.


>diff --git a/source/browser/components/places/content/toolbar.xml b/source/browser/components/places/content/toolbar.xml

you are diffing a level too low, your diff should be done inside source, so it should be for 
/browser/components/places/content/toolbar.xml
Btw if you want to start hacking on mozilla sources i really suggest you to five a try at mozilla-build and start learning Mercurial.

>diff --git a/source/browser/components/places/content/utils.js b/source/browser/components/places/content/utils.js
>--- a/source/browser/components/places/content/utils.js
>+++ b/source/browser/components/places/content/utils.js
>@@ -976,15 +976,22 @@
>             return;
>           }
>         }
>       }
>       openUILinkIn(aNode.uri, aWhere);
>     }
>   },
>-
>+  

nit: trailing spaces here

>+  /**
>+    * Helper for exposing URI Scheme
>+    */

fix the indentation here, see other comments, also please use javadoc style, so

/**
 * Helper for guessing scheme from an url string.
 * Used to avoid nsIURI overhead in frequently called UI functions.
 *
 * @param aUrlString the url to guess the scheme from.
 * 
 * @return guessed scheme for this url string.
 *
 * @note: this is not supposed be perfect, so use it only for UI purposes.
 */
>+  guessURISchemeForUI: function PU_guessURISchemeForUI(aNode) {

make this guessUrlSchemeForUI and make it receive directly an url string, there's no need
to pass the full node.
The name should be PUU_guess... since this is PlacesUIUtils

>+    return aNode.uri.substr(0, aNode.uri.indexOf(":"));
>+  },
>+  

nit: trailing spaces here

>diff --git a/source/browser/components/places/content/treeView.js b/source/browser/components/places/content/treeView.js
>--- a/source/browser/components/places/content/treeView.js
>+++ b/source/browser/components/places/content/treeView.js
>@@ -924,14 +924,17 @@
>         }
>       }
>       else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR)
>         properties.push(this._getAtomFor("separator"));
>       else if (itemId != -1) { // bookmark nodes
>         if (PlacesUtils.nodeIsLivemarkContainer(node.parent))
>           properties.push(this._getAtomFor("livemarkItem"));
>+        else if (PlacesUtils.nodeIsURI(node)) {
>+            properties.push(this._getAtomFor(PlacesUIUtils.guessURISchemeForUI(node)));
>+        }
>       }

this should be inverted with (itemId != -1) since could apply to history items too.
so you should have

else if (nodeIsURI) {
  // apply scheme
  if (itemId != -1)
    // apply livemark
}
Attachment #366459 - Flags: review?(mak77) → review-
(In reply to comment #3)
> (From update of attachment 366459 [details] [diff] [review])
> notice this patch should apply upon the previous one (bookmarklets), so please
> update your sources.
> 
O_O I swear I've downloaded the files yesterday from http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/ and the patch was not applied. So I erroneously thought you had backed out the patch... My fault. Anyway, should I download the source directly from hg? Unfortunately http://hg.mozilla.org/mozilla-central/raw-file/71507aa08a27/browser/components/places/content/menu.xml gives a XML Parsing Error.

> you are diffing a level too low, your diff should be done inside source, so it
> should be for 
> /browser/components/places/content/toolbar.xml
> Btw if you want to start hacking on mozilla sources i really suggest you to
> five a try at mozilla-build and start learning Mercurial.
> 
I'm using Subversion a couple years now and I have already a lot of projects under Subversion. I didn't have the time to consider migrating to Mercurial yet.

I'm working on a new patch now. Thank you for your points (and also for your patience ;-))
See https://developer.mozilla.org/en/Mozilla_Source_Code_(Mercurial) for help with Mercurial, there are tons of docs on MDC about Mercurial, so feel free to rummage through.
(In reply to comment #5)
> See https://developer.mozilla.org/en/Mozilla_Source_Code_(Mercurial) for help
> with Mercurial, there are tons of docs on MDC about Mercurial, so feel free to
> rummage through.

Thanks for the link. I will for sure have a look on it.
Posted patch patch v1.1 (obsolete) — Splinter Review
new patch
Attachment #366459 - Attachment is obsolete: true
Attachment #366713 - Flags: review?(mak77)
Attachment #366713 - Flags: review?(mak77) → review-
Comment on attachment 366713 [details] [diff] [review]
patch v1.1

sorry, but you didn't fix all comments in comment 3 (helper name and docs)
Posted patch patch v1.2 (obsolete) — Splinter Review
new patch
Attachment #366713 - Attachment is obsolete: true
Attachment #366745 - Flags: review?(mak77)
Comment on attachment 366745 [details] [diff] [review]
patch v1.2

>diff --git a/browser/components/places/content/utils.js b/browser/components/places/content/utils.js
>   /**
>+   * Helper for guessing scheme from an url string.
>+   * Used to avoid nsIURI overhead in frequently called UI functions.
>+   *
>+   * @param aUrlString the url to guess the scheme from.
>+   * 
>+   * @return guessed scheme for this url string.
>+   *
>+   * @note: this is not supposed be perfect, so use it only for UI purposes.

my fault, note should not have a trailing ":", but i'll fix this on push.

looks good, thanks!
Attachment #366745 - Flags: review?(mak77) → review+
Posted patch as pushedSplinter Review
Attachment #366745 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/297c00e7245b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
patch was missing semicolons :\
pushed an additional changeset
http://hg.mozilla.org/mozilla-central/rev/709e55b2747d
Target Milestone: Firefox 3.2a1 → ---
Target Milestone: --- → Firefox 3.2a1
Comment on attachment 367033 [details] [diff] [review]
as pushed

asking approval because this would be good for theme developers
Attachment #367033 - Flags: approval1.9.1?
and very low risk
(In reply to comment #13)
> patch was missing semicolons :\
> pushed an additional changeset
> http://hg.mozilla.org/mozilla-central/rev/709e55b2747d

Uups... Sorry for that.
Comment on attachment 367033 [details] [diff] [review]
as pushed

a191=beltzner
Attachment #367033 - Flags: approval1.9.1? → approval1.9.1+
pushed some missing bit of this patch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7a27f2cb898a
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.