529597 - Places protocol handler

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Drew Willcoxon :adw]

Bookmarks and history are browsable first-class objects in 3.7, so we need a protocol handler.  One possibility is to simply turn the existing place: URIs into browsable URLs, but we should investigate.  Are all place: URIs browsable?  Are there things that are browsable but don't have place: URIs?

Comment 1

[David Dahl :ddahl]

I think this matches up to the controller in MVC. Perhaps the protocol handler is mainly a dispatcher for several controllers that are invoked depending on the task at hand: BookMarksController, HistoryController, AwesomeResultsController, etc...

Comment 2

[David Dahl :ddahl]

what I mean to say is that I would write this like the Rails' dispatcher or Django's mechanics behind urls.py: http://docs.djangoproject.com/en/dev/topics/http/urls/

Comment 3

[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

Comment 4

[Drew Willcoxon :adw]

Attachment: WIP 1

Saving my work, an empty stub that handles "place:" URLs.  Navigating to place:foo displays the URL in the tab.

(In reply to comment #1)
> I think this matches up to the controller in MVC. Perhaps the protocol handler
> is mainly a dispatcher for several controllers that are invoked depending on
> the task at hand: BookMarksController, HistoryController,
> AwesomeResultsController, etc...

Exactly right I think.  It's easy to imagine a dispatcher and controllers in this patch.

I think the next task is to decide whether we want to use the existing place: URIs or need to come up with something else because they aren't sufficient.  Using the existing URIs, even though they're ugly, would be great:  We'd need only a single controller that just passes them to nsINavHistoryService.queryStringToQueries() and executes the queries.  The views would take it from there.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
Note: the flags you'll want are:
URI_NORELATIVE
URI_DANGEROUS_TO_LOAD
URI_IS_LOCAL_RESOURCE

And of course, you'll want to make sure your channel is asynchronous when it is opened.

Comment 6

[Marco Bonardo [:mak]]

ideally we won't load any content that is not in our db (about security), so security analysis should take that in count.

about place: uris being enough, i think we should have a way to make them work with both old and new syntax... something like a versioning?
place:option1=aaa&option2=bbb, place:1:new_syntaxV1, place:2:new_syntaxV2

I think it would be really cool to have a single protocol handler able to forward traffico toward the correct querying api.

Comment 7

[Drew Willcoxon :adw]

After thinking a bit, I don't see any reason not to use the existing URI scheme.  We need the following to be browsable:

* folders
* tags
* history ranges

All can be represented with queries already.  They're just a subset of all queries of course, but I don't think it's a big deal if others -- searchTerms, maxVisits, tagsAreNot, etc. -- are also browsable.  (In fact it's kind of cool and will give Lifehacker something to write about.)

The only downside is that the URIs aren't pretty, since they weren't designed to be exposed to users.

One thing I noticed with this patch is that when I added place:foo to my bookmarks toolbar, on restart it turned into a container with junk inside.  Either we'll have to rework the UI bits to recognize that place:s are now browsable, or we'll have to pick a different scheme that simply translates to place:.

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #7)
> One thing I noticed with this patch is that when I added place:foo to my
> bookmarks toolbar, on restart it turned into a container with junk inside. 

junk? i'd expect it to contain default query (history ordered by something like id i suppose).

Comment 9

[David Dahl :ddahl]

I would really like it if the uris were not ugly, but that is just a developer ergonomics thing, as we will not neccesarily be displaying them to endusers.

Comment 10

[David Dahl :ddahl]

what I mean is that I assume the UI that reflects the place URI will be nice like faaborg's mockups.

Comment 11

[Marco Bonardo [:mak]]

the breadcrumb will hide uris, if that's what you mean

Comment 12

[Dietrich Ayala (:dietrich)]

seems like there's two inputs:

1. raw place URIs
2. the natural language queries and breadcrumbs

maybe the question is whether we push understanding of the natural language queries and breadcrumbs into the backend, or if it's a front-end component that converts them into place URIs?

Comment 13

[Drew Willcoxon :adw]

(In reply to comment #9)
> I would really like it if the uris were not ugly, but that is just a developer
> ergonomics thing, as we will not neccesarily be displaying them to endusers.

This ties in with your query API redesign.  Do you know yet how it will affect place: URIs?

(In reply to comment #11)
> the breadcrumb will hide uris, if that's what you mean

I forgot about that.  Although it would be a bonus if the URLs were simple and memorable enough for the user to type in, if she wanted.

(In reply to comment #12)
> 1. raw place URIs
> 2. the natural language queries and breadcrumbs

I was thinking we'll do 2 in awesomebar code, since it's similar to the search strings it handles now, and then build appropriate place: URIs.

Comment 14

[David Dahl :ddahl]

(In reply to comment #13)
> (In reply to comment #9)
> > I would really like it if the uris were not ugly, but that is just a developer
> > ergonomics thing, as we will not neccesarily be displaying them to endusers.
> 
> This ties in with your query API redesign.  Do you know yet how it will affect
> place: URIs?

No, not yet.

Comment 15

[Drew Willcoxon :adw]

Attachment: WIP 2

WIP 1 stuffed some HTML into a data URI into a channel.  Problem is, that HTML doesn't have chrome privs.  The chrome: protocol handler grants chrome privs by setting the channel's owner to the system security principal.  I don't know a way of doing the same for my handler here, so I just route it through chrome.  This patch adds browser/components/places/content/placesContent.html, which I load into a channel from its chrome: URI.

I was thinking that the HTML would be built up in the "back-end" before the channel is opened, but given the above it looks like it'll be done entirely by JS running in placesContent.html.  The plan is, onload, placesContent.html examines window.location.href, which will be something like "place:folder=1", turns it into queries, executes them, and builds the view.  Since it delegates to this HTML file now, the protocol handler itself is really simple.

Does that make sense?

Just as a proof of concept, this patch has a quick and dirty view:  You can load up places:folder=1 and actually browse around your bookmarks by clicking the links.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> WIP 1 stuffed some HTML into a data URI into a channel.  Problem is, that HTML
> doesn't have chrome privs.  The chrome: protocol handler grants chrome privs by
> setting the channel's owner to the system security principal.  I don't know a
> way of doing the same for my handler here, so I just route it through chrome. 
> This patch adds browser/components/places/content/placesContent.html, which I
> load into a channel from its chrome: URI.
You'll want to do it just like the about: protocol here:
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsAbout.cpp#64

All the methods should be scriptable, so you just need to convert the C++ to JS.

Comment 17

[David Dahl :ddahl]

(In reply to comment #15)

> Does that make sense?
> 
Yes. So this is like the "main" controller or dispatcher. Perhaps you should throw these files in a directory like browser/components/places/content/mvc/controller.html ?

Or give the directory a snazzy name just to distinguish it a bit.
Just a thought.

Comment 18

[Drew Willcoxon :adw]

Thanks, Shawn and David.

Collecting my thoughts, there are two ways to do this:

1. Generate all the HTML in the back-end and stream it through the channel.
2. Piggyback on an existing stub HTML file delivered through a chrome: channel, and generate most of the content dynamically in content JS once that stub is loaded.  (Fortunately it appears JS still works in chrome-privileged content even if you turn off JS.)

The simpler built-in about: handlers -- about:blank, about:cache, etc. -- do 1.  Others -- the one linked in comment 16 at least -- do 2.  I looked at Jetpack and about:me, and they do 2 too.  (From what I could tell, Chrome does basically the same also.)

I think 2 is simpler and makes more sense here.

Comment 19

[Marco Bonardo [:mak]]

since the basic functionalities of the page won't change much between our data, i think 2 makes sense and creates better code separation, and this stuff is really exciting. I hope hooking up editing capabilities in future won't be an hell.

Comment 20

[David Dahl :ddahl]

Now we just need mconner to bless jQuery:) HA HA.

Comment 21

[Drew Willcoxon :adw]

Attachment: for review v1

Really small patch, loads browser/components/places/content/content-ui/controller.xhtml through a chrome: channel.  (I think ddahl's suggestion to create a new directory for this is good.)  I'll ask for security review once the Places review is OK.

(In reply to comment #19)
> I hope hooking up editing capabilities in future won't be an
> hell.

(In reply to comment #20)
> Now we just need mconner to bless jQuery:) HA HA.

I take this opportunity to reiterate my comment that if we had spare webdev people, we should turn them loose all up in here.

Comment 22

[Drew Willcoxon :adw]

Something I forgot from comment 7.  Folders will become browsable in 3.7.  However, a folder on your bookmarks toolbar is a container.  It expands.  What should happen in 3.7 if you put a folder on your toolbar?

Beyond UX, this is important for the protocol handler, because if we want to change the behavior, we should invent a new "browsable" protocol similar to place: but that specifically does not expand.

Comment 23

[Marco Bonardo [:mak]]

(In reply to comment #22)
> Beyond UX, this is important for the protocol handler, because if we want to
> change the behavior, we should invent a new "browsable" protocol similar to
> place: but that specifically does not expand.

I think there are 2 possibilities:
- folder will just open in content
- folder will have an expand button to support both beaviors

in both cases though the view should be able to avoid expansion without the need for 2 handlers

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 416141 [details] [diff] [review]
for review v1

just a couple drive by comments for now, we started talking to bz about our target and eventual issues, we should define what is carved in stones in next few days, while we are here.

>diff --git a/browser/components/places/src/Makefile.in b/browser/components/places/src/Makefile.in

>-EXTRA_COMPONENTS = nsPlacesTransactionsService.js
>+EXTRA_COMPONENTS = \
>+                   nsPlacesTransactionsService.js \
>+                   protocolHandler.js \
>+                   $(NULL)

since going nl, let's use just 2 spaces
EXTRA_COMPONENTS = \
  component.js \
  ...

>diff --git a/browser/components/places/src/protocolHandler.js b/browser/components/places/src/protocolHandler.js

i think the filename should start uppercase, what's the style of other components?
actually all Places components are called nsPlacesComponent, we thought about removing
 ns prefix but looks like we allowed to add external files to the component dir, so maybe it's better to continue like that, so that all places components have the same prefix and are near each other.

you should add this file to /browser/installer/package-manifest.in too
 
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

this should always be Mozilla Foundation regardless anything done till now

>+
>+ProtocolHandler.prototype = {
>+
>+  scheme: SCHEME,

the empty new line above does not look like a common style convention

>+  defaultPort: -1,
>+  protocolFlags: Ci.nsIProtocolHandler.URI_DANGEROUS_TO_LOAD |
>+                 Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE |
>+                 Ci.nsIProtocolHandler.URI_NORELATIVE,

we probably also want URI_NO_AUTH

>+  newURI: function PPH_newURI(aSpec, aOriginCharset, aBaseUri) {
>+    let uri = Cc["@mozilla.org/network/simple-uri;1"].createInstance(Ci.nsIURI);
>+    uri.spec = aSpec;
>+    return uri;
>+  },

could make sense to have a lazyGetter for ioService and use newURI here

>+  newChannel: function PPH_newChannel(aUri) {
>+    let url = "chrome://browser/content/places/content-ui/controller.xhtml";

this sounds like being a const

>+    let chan = Cc["@mozilla.org/network/io-service;1"].
>+                 getService(Ci.nsIIOService).
>+                 newChannel(url, null, null);

ditto for the lazy getter (btw the formatting is wrong here, getSvc should be aligned with Cc)

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> i think the filename should start uppercase, what's the style of other
> components?
> actually all Places components are called nsPlacesComponent, we thought about
> removing
>  ns prefix but looks like we allowed to add external files to the component
> dir, so maybe it's better to continue like that, so that all places components
> have the same prefix and are near each other.
Just using |Places| as a prefix moving forward is probably fine.

> you should add this file to /browser/installer/package-manifest.in too
AFAIK, we don't need to do this with components anymore.

> could make sense to have a lazyGetter for ioService and use newURI here
No - the IO service would be calling us in the first place.  This would be a bad thing to do :)

> >+    let chan = Cc["@mozilla.org/network/io-service;1"].
> >+                 getService(Ci.nsIIOService).
> >+                 newChannel(url, null, null);
> 
> ditto for the lazy getter (btw the formatting is wrong here, getSvc should be
> aligned with Cc)
NetUtil.newChannel (via bug 532146)

Comment 26

[Marco Bonardo [:mak]]

(In reply to comment #25)
> > you should add this file to /browser/installer/package-manifest.in too
> AFAIK, we don't need to do this with components anymore.

i had to do it for expiration otherwise the component was not installed in components folders

> > could make sense to have a lazyGetter for ioService and use newURI here
> No - the IO service would be calling us in the first place.  This would be a
> bad thing to do :)

ugh, right

> NetUtil.newChannel (via bug 532146)

a new helper, yay!

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 416141 [details] [diff] [review]
for review v1

waiting new patch but looks good overall.

Comment 28

[Drew Willcoxon :adw]

Attachment: for review v2

Addresses comments, except NetUtil.newChannel was backed out recently.

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 418473 [details] [diff] [review]
for review v2

>diff --git a/browser/components/places/src/PlacesProtocolHandler.js b/browser/components/places/src/PlacesProtocolHandler.js

>+ * Contributor(s):
>+ *   Drew Willcoxon <[e-mail]>

please tail (original author)

>+function ProtocolHandler() {}

i'd say to call it like the filename, PlacesProtocolHandler

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIProtocolHandler]),

nit: to avoid polluting blame in future:
QueryInterface: XPCOMUtils.generateQI([
  Ci.nsIProtocolHandler
]),

looks good otherwise

Comment 30

[Marco Bonardo [:mak]]

if we are going to push this without the NetUtil (bug 532146 ), i'd say to file a followup to use it once pushed. if instead that will make trunk before this, we should use it here.

Comment 31

[Shawn Wilsher :sdwilsh]

I just need to fix the test for that bug.  I can probably do this today, but somebody else will have to push it for me.  You can apply that patch locally, as the actually code is fine (removing a file on windows is flaky, and therefore problematic.  I know this from storage tests, but forgot when I wrote that test in NetUtil)

Comment 32

[Drew Willcoxon :adw]

Comment on attachment 418473 [details] [diff] [review]
for review v2

Requesting bz's review as suggested by Marco and Shawn, since this patch creates a new protocol handler and has security and netwerk implications.

I'll address NetUtil and the nits of comment 29 in the final patch, after bz has looked at this one.

Comment 33

[Boris Zbarsky [:bzbarsky]]

So I'm a little confused about this protocol handler.  This protocol handler returns the same data no matter what URI is actually used, as long as it has this scheme?

Comment 34

[Drew Willcoxon :adw]

Yes, controller.xhtml builds itself dynamically onload by examining its URL.  Comment 18 explains the design choice.

Comment 35

[Boris Zbarsky [:bzbarsky]]

What sorts of URIs are users going to load via these channels?  Are they going to want to save them?  View the "real" source?

Comment 16 links to a protocol impl that only returns data for one url, so returning the same data every time is ok.

Fundamentally, using #1 and delivering "the same" data for different URIs is an abuse of the whole idea of URIs....  We can do it if it's really warranted, but it's not obvious to me that it is in this case.

Comment 36

[Drew Willcoxon :adw]

I see your point.

One concern I have, looking to the future, is that eventually we will query bookmarks and history asynchronously.  It's easy to imagine retrofitting the current design to support that.  (Natural, even.  Async Places queries become analogous to XHRs.)  In fact, when we do move completely to async, what will be the meaning of the content synchronously returned by this protocol handler?

If I return an async channel from newChannel(), will it be opened asynchronously?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Why would the protocol handler need to return content synchronously?

You have no control over what your channel consumer will do, but if they call open() instead of asyncOpen() you're allowed to spin the event loop as desired.  That's what HTTP does, say.

Comment 38

[Drew Willcoxon :adw]

(In reply to comment #37)
> Why would the protocol handler need to return content synchronously?

Well, this is what I was asking, if I return an async channel, will my consumer open it asynchronously?  I figured I had no control over that, as you say, and in that case how can I not return content synchronously?  (I'm not trying to be stubborn, I just really don't know.)  Spinning the even loop for this sounds crazy.

What about simply returning stub content that is nearly identical for every URI except that it contains the URI within the document?  Instead of examining the URL onload, the JS examines the URI within the document and then builds the doc accordingly.  The entire doc, JS and all, could be self-contained.

Comment 39

[Drew Willcoxon :adw]

(In reply to comment #38)
> accordingly.  The entire doc, JS and all, could be self-contained.

Except that doesn't help if the user saves the file for later, file: URLs not having chrome privs...

Comment 40

[Boris Zbarsky [:bzbarsky]]

You return a channel.  It needs to implement the nsIChannel API.  How it does this is up to it.  The consumer will then either asyncOpen() or open() the channel.  In the latter case, the channel has to return a stream, and attempts to read the stream have to look synchronous to the consumer, but both open() and attempts to read the stream are allowed to spin the event loop.

Returning broken stub content (since it needs chrome privileges to work right) is not much better than returning the same stub for everything.  If you're not expecting anyone to ever be able to sensibly work with the byte stream from this channel, then you can just go with what you already have.  The question I had was whether this was a conscious limitation (and general violation of the way URIs are supposed to work) or just an accident.

Comment 41

[Drew Willcoxon :adw]

An accident.  I hadn't thought deeply enough about it or I would have noted it earlier.  (I can't speak for others following this bug, Marco and Shawn, maybe they did?)  I think it's up to Dietrich to make the conscious decision here.  If we decide to ignore your concerns, would that block your r+?

Thanks, Boris.  I'll let you clear the review request or r-, whichever makes more sense.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> If we decide to ignore your concerns, would that block your r+?

No.  As long as you're ok with the byte-stream being useless, the patch looks fine.

Comment 43

[Marco Bonardo [:mak]]

This is an interesting question.
The main purpose of the protocol handler is to show pages with links (and thumbs and infos to the user) and view/search functionality, for this functionality the current patch is fine.
But i could see some user willing to save the page contents as a way to save a single bookmarks folder or a specific search result.
On the other side if i understand this correctly, it is not much different than trying to save a js dynamically built web page, so we could provide a "save current view/search as bookmarks.html" option.
That said, Boris knows implications of the two approaches largely better than me, personally i think that if the problem is just saving the page and implementing the stream makes the code much more complex, then we should not block on it and provide a later save option (Actually i don't know other use-cases).
If otherwise the code changes are not so complex we would obtain both effects at once.

Comment 44

[Drew Willcoxon :adw]

(In reply to comment #43)
> On the other side if i understand this correctly, it is not much different than
> trying to save a js dynamically built web page, so we could provide a "save
> current view/search as bookmarks.html" option.

It's a little different since we need chrome privileges.  Extending your analogy, it's more like saving an XHR-heavy page and then trying to use it offline.  We can provide a separate option, but if we want that we should just write the protocol handler to return all the content in the first place, since that's what it would amount to.

The two main practical issues with the current impl might be:

1. You can view the pages only in Firefox in your profile through the protocol
   handler.  You can't really get them "out" of Firefox.
2. They're only useful to human eyes.  The byte streams delivered through the
   handler are identical for all URIs.

I don't think we care about 2, since they're intended to be presentation only.  We might care about 1, and I can picture future bugs like "I saved my bookmarks and now they're gone!"  If we don't care, we should at least disable the browser's save commands.

> If otherwise the code changes are not so complex we would obtain both effects
> at once.

Doing it with the current sync Places API is OK, but I'm not clear on how to do it when we have async.  Boris says we can spin the event loop.  I don't have any experience doing that, and it sounds hairy.

Comment 45

[Drew Willcoxon :adw]

A related issue is live update.  Eventually these pages should support it, but a live update shouldn't cause the entire page to reload.  In other words, a live update causes a change that isn't reflected in the page's source.

Comment 46

[Drew Willcoxon :adw]

Actually, to save the page we could just serialize the DOM.

Comment 47

[Marco Bonardo [:mak]]

I don t think saving the page blocks us there Are many possible alternatve solutions. And no, the page is not intended for non-humans.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> Boris says we can spin the event loop. 

You only have to do that if someone calls open() on the channel.  And it's trivial to do, fwiw.

Comment 49

[Marco Bonardo [:mak]]

So, as drew said we will support liveupdate, in such a case the user will want to save current status of the page, we would still want to handle save differently in both cases.

Comment 50

[Drew Willcoxon :adw]

Marco, it sounds like we both think this patch is OK.  Dietrich wanted to land it soon; do you think it's OK, since he's on vacation?

Comment 51

[Marco Bonardo [:mak]]

i'm ok with it, if Boris thinks it does not involve any security issue and  does not block us on anything other use-case that was not figured out till now.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Yeah, I don't see any security problems here.

Comment 53

[Drew Willcoxon :adw]

Attachment: for review v2.1

Great, can I get your r+ on this final patch?  The only differences from the last are the three nits in comment 29.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419308 [details] [diff] [review]
for review v2.1

Looks ok.

Comment 55

[Drew Willcoxon :adw]

Attachment: for check-in

With updated commit message to help whoever checks this in.

Comment 56

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/06e4ea15db72

Comment 57

[Robert Longson [:longsonr]]

Could this have caused?

s: moz2-darwin9-slave095591 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/places/tests/test_bug_405924.html | Alert Dialog was shown
5592 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/places/tests/test_bug_405924.html | Test timed out.

Comment 58

[Dão Gottwald [:dao]]

backed out

Comment 59

[Marco Bonardo [:mak]]

we are probably expecting that opening places: url is considered a bad url in that test, that is not true anymore. thanks for backout.

Comment 60

[Drew Willcoxon :adw]

Marco, can we just remove that test?  All it's doing is checking that there's no place: protocol handler.  It's kind of dumb.

Comment 61

[Marco Bonardo [:mak]]

oh, hrm, yeah i agree it's testing the opposite of what we are implementing. You have my approval to remove it.

Comment 62

[Drew Willcoxon :adw]

NetUtil.newChannel re-landed in the meantime.  I'll update the patch to use it unless I hear otherwise.  It doesn't set the originalURI of the channel, though, and my patch here did.  Is originalURI really necessary?

Comment 63

[Boris Zbarsky [:bzbarsky]]

> Is originalURI really necessary?

If you don't set originalURI then the url bar will show the channel's URI (in your case, chrome://browser/content/places/content-ui/controller.xhtml).  If you want it to show the place:whatever URI, then you need to set it as originalURI.

I see no reason you couldn't set originalURI on the channel NetUtil.newChannel returns.  Do you?

Comment 64

[Drew Willcoxon :adw]

No, after double checking the originalURI comment in nsIChannel.idl I suspected that that's the answer but wanted to make sure.  Thanks.

Comment 65

[Drew Willcoxon :adw]

Attachment: for check-in 2

Removes old test, uses NetUtil.newChannel.  Pushed to tryserver, all oranges are known oranges, requesting check-in again.

Comment 66

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/6453f3ca39fc