Places protocol handler

RESOLVED FIXED in Firefox 3.7a1

Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 years ago
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

9 years ago
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/
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
(Assignee)

Comment 4

9 years ago
Created attachment 415543 [details] [diff] [review]
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.
(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.
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.
(Assignee)

Comment 7

9 years ago
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:.
(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

9 years ago
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

9 years ago
what I mean is that I assume the UI that reflects the place URI will be nice like faaborg's mockups.
the breadcrumb will hide uris, if that's what you mean
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?
(Assignee)

Comment 13

9 years ago
(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

9 years ago
(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.
(Assignee)

Comment 15

9 years ago
Created attachment 415787 [details] [diff] [review]
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.
Attachment #415543 - Attachment is obsolete: true
(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

9 years ago
(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.
(Assignee)

Comment 18

9 years ago
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.
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

9 years ago
Now we just need mconner to bless jQuery:) HA HA.
(Assignee)

Comment 21

9 years ago
Created attachment 416141 [details] [diff] [review]
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.
Attachment #415787 - Attachment is obsolete: true
Attachment #416141 - Flags: review?(dietrich)
(Assignee)

Updated

9 years ago
Attachment #416141 - Flags: review?(dietrich) → review?(mak77)
(Assignee)

Comment 22

9 years ago
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.
(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 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)
(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)
(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 on attachment 416141 [details] [diff] [review]
for review v1

waiting new patch but looks good overall.
Attachment #416141 - Flags: review?(mak77)
(Assignee)

Comment 28

9 years ago
Created attachment 418473 [details] [diff] [review]
for review v2

Addresses comments, except NetUtil.newChannel was backed out recently.
Attachment #416141 - Attachment is obsolete: true
Attachment #418473 - Flags: review?(mak77)
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 <adw@mozilla.com>

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
Attachment #418473 - Flags: review?(mak77) → review+
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.
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)
(Assignee)

Comment 32

9 years ago
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.
Attachment #418473 - Flags: review?(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?
(Assignee)

Comment 34

9 years ago
Yes, controller.xhtml builds itself dynamically onload by examining its URL.  Comment 18 explains the design choice.
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.
(Assignee)

Comment 36

9 years ago
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?
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.
(Assignee)

Comment 38

9 years ago
(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.
(Assignee)

Comment 39

9 years ago
(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...
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.
(Assignee)

Comment 41

9 years ago
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.
> 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.
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.
(Assignee)

Comment 44

9 years ago
(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.
(Assignee)

Comment 45

9 years ago
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.
(Assignee)

Comment 46

9 years ago
Actually, to save the page we could just serialize the DOM.
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.
> 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.
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.
(Assignee)

Comment 50

9 years ago
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?
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.
Yeah, I don't see any security problems here.
(Assignee)

Comment 53

9 years ago
Created attachment 419308 [details] [diff] [review]
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.
Attachment #418473 - Attachment is obsolete: true
Attachment #419308 - Flags: review?(bzbarsky)
Attachment #418473 - Flags: review?(bzbarsky)
Comment on attachment 419308 [details] [diff] [review]
for review v2.1

Looks ok.
Attachment #419308 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 55

9 years ago
Created attachment 419321 [details] [diff] [review]
for check-in

With updated commit message to help whoever checks this in.
Attachment #419308 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/06e4ea15db72
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 3.7 → Firefox 3.7a1
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.
backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
we are probably expecting that opening places: url is considered a bad url in that test, that is not true anymore. thanks for backout.
(Assignee)

Comment 60

9 years ago
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.
oh, hrm, yeah i agree it's testing the opposite of what we are implementing. You have my approval to remove it.
(Assignee)

Comment 62

9 years ago
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?
> 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?
(Assignee)

Comment 64

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

Comment 65

9 years ago
Created attachment 419534 [details] [diff] [review]
for check-in 2

Removes old test, uses NetUtil.newChannel.  Pushed to tryserver, all oranges are known oranges, requesting check-in again.
Attachment #419321 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/6453f3ca39fc
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 737046
You need to log in before you can comment on or make changes to this bug.