482235 - instead of exposing bookmarklets attribute, expose scheme in uri nodes

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

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(":"))

Comment 1

[Marco Bonardo [:mak]]

Pardal do you want to take care of this since you originally worked on bookmarklets patch?

Comment 2

[Pardal Freudenthal (:ShareBird)]

Attachment: patch

proposed patch

Comment 3

[Marco Bonardo [:mak]]

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
}

Comment 4

[Pardal Freudenthal (:ShareBird)]

(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 ;-))

Comment 5

[Nochum Sossonko [:Natch]]

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.

Comment 6

[Pardal Freudenthal (:ShareBird)]

(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.

Comment 7

[Pardal Freudenthal (:ShareBird)]

Attachment: patch v1.1

new patch

Comment 8

[Marco Bonardo [:mak]]

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)

Comment 9

[Pardal Freudenthal (:ShareBird)]

Attachment: patch v1.2

new patch

Comment 10

[Marco Bonardo [:mak]]

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!

Comment 11

[Marco Bonardo [:mak]]

Attachment: as pushed

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/297c00e7245b

Comment 13

[Marco Bonardo [:mak]]

patch was missing semicolons :\
pushed an additional changeset
http://hg.mozilla.org/mozilla-central/rev/709e55b2747d

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 367033 [details] [diff] [review]
as pushed

asking approval because this would be good for theme developers

Comment 15

[Marco Bonardo [:mak]]

and very low risk

Comment 16

[Pardal Freudenthal (:ShareBird)]

(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 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367033 [details] [diff] [review]
as pushed

a191=beltzner

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3aa09f6dad1d

Comment 19

[Marco Bonardo [:mak]]

pushed some missing bit of this patch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7a27f2cb898a

Comment 20

[Gervase Markham [:gerv]]

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