305313 - Allow additional predicates in assertions for bookmarks datasource

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Todd Agulnick]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050818 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050818 Firefox/1.6a1

nsIBooksMarks service restricts predicates that can be set on the bookmarks
datasource via nsiBookmarksService::CanAccept(). The attached patch allows two
additional predicates (instanceOf and forwardProxy) to be used. These are
required to create bookmarks and folders via the RDF interface.

Reproducible: Always

Comment 1

[Todd Agulnick]

Attachment: Bookmarks patch for CanAccept().

Comment 2

[Alec Flett]

This is more of an RFE than a bug, so I'm confirming it.

Comment 3

[Alec Flett]

ok, no obvious owner for this, so I'm assigning to todd and will help him get
this patch submitted

Comment 4

[Mike Connor [:mconnor]]

So, we're changing the whole bookmarks component and moving to a sqlite-backed
storage solution, does this matter in the new world?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No; this all goes away in the new world.

Comment 6

[Alec Flett]

So in the new world, we'll be able to make arbitrary RDF assertions in the
bookmarks datasource? or at least will the bookmarks datasource allow folder
creation through nsIRDFDataSource?

Comment 7

[Benjamin Smedberg]

In the new world there will not be a bookmarks RDF datasource.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(There may be a read-only RDF datasource for compatability, but you most likely
won't be able to create new assertions this way)

Comment 9

[Alec Flett]

Comment on attachment 193283 [details] [diff] [review]
Bookmarks patch for CanAccept().

wow, that's really pretty disappointing... but a whole other issue that myself
or todd can complain about later :)

In the meantime, since this won't be rewritten for 1.5, I'd like to help todd
get this into the trunk. He's got an interesting extension that allows some
neat RDF-based datasource merging that depends on this patch.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Are you ok with the kForwardProxy assertion automatically changing if the URL of
the bookmark is ever changed by the user?  Also, are you ok with your custom
forwardProxy and instanceOf bits never getting saved to disk?

Comment 11

[Todd Agulnick]

(In reply to comment #10)
> Are you ok with the kForwardProxy assertion automatically changing if the URL of
> the bookmark is ever changed by the user?

Yes, that's fine.

>  Also, are you ok with your custom
> forwardProxy and instanceOf bits never getting saved to disk?

That would be a problem. Would instanceOf be treated any differently from, say,
nextVal (which presumably is saved to disk)? They're both container-related, so
I assumed they're both written to disk. What did I overlook?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #11)
> >  Also, are you ok with your custom
> > forwardProxy and instanceOf bits never getting saved to disk?
> 
> That would be a problem. Would instanceOf be treated any differently from, say,
> nextVal (which presumably is saved to disk)? They're both container-related, so
> I assumed they're both written to disk. What did I overlook?

nextVal is never saved to disk as nextval; Bookmarks are not serialized using
RDF.  It's purely an in-memory store that's populated from bookmarks.html and
written to it using a custom pseudo-HTML goop writer.

Comment 13

[Todd Agulnick]

(In reply to comment #12)
> 
> nextVal is never saved to disk as nextval; Bookmarks are not serialized using
> RDF.  It's purely an in-memory store that's populated from bookmarks.html and
> written to it using a custom pseudo-HTML goop writer.

Thanks for pointing that out -- I had overlooked how bookmarks get written to disk.

Fortunately, I don't think it affects what I'm trying to do. The goal is to make
the same sequence of assertions that the bookmark service itself generates (on
mInner) when it creates, modifies, or deletes bookmarks, folders, separators,
etc. As it stands now, the only obstacle is CanAccept(), which refuses from
clients a few types of assertions (like instanceOf) that it uses itself
internally. (Actually, bmsvc calls container utils::MakeSeq() which does an
instanceOf assertion. But I digress.)

So long as the datasource's state is the same after my series of manual
assertions as it would be had I used one of bmsvc's functions (e.g.,
CreateFolder()) directly, everything else should work out. I'm not trying to
create any new types of assertions, but merely echo manually the ones that bmsvc
uses itself.

Comment 14

[Todd Agulnick]

Attachment: Revised patch.

Revised patch for CanAccept(). Now also allows NC:ID assertions.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 193283 [details] [diff] [review]
Bookmarks patch for CanAccept().

I'm still a little scared about this, because I'm not sure what the behaviour
will be of things if instanceOf/etc. are set to things that are not expected...
the entire bookmarks system is rather fragile.	However, the risk here is only
if an extension actually sets these RDF assertions itself, and shouldn't affect
normal usage, so that's probably safe enough.

r=vladimir

Comment 16

[Mike Schroepfer]

Todd,

Please land this on the trunk and mark the bug as resolved->fixed.   Then ask
for 1.8b5 approval in the patch.

Comment 17

[Todd Agulnick]

(In reply to comment #16)
> Todd,
> 
> Please land this on the trunk and mark the bug as resolved->fixed.   Then ask
> for 1.8b5 approval in the patch.

Mike,

Can you please provide some direction on what I need to do to land this patch on
the trunk? {Insert clueless newbie apologies here.}

Thanks,

-Todd

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> Can you please provide some direction on what I need to do to land this patch on
> the trunk? {Insert clueless newbie apologies here.}

You need to find someone with CVS access to check it in. You can try asking
#developers on irc.mozilla.org.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Todd, I can get it landed for you on the trunk; wasn't sure if you had cvs
access or not.  I'll get it in in a bit today.

Comment 20

[Todd Agulnick]

(In reply to comment #19)
> Todd, I can get it landed for you on the trunk; wasn't sure if you had cvs
> access or not.  I'll get it in in a bit today.
> 

Vlad,

No, I don't have cvs access. Thanks for your help.

-Todd

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 195194 [details] [diff] [review]
Revised patch.

r=vladimir on revised patch

checked in on trunk.

Comment 22

[Mike Schroepfer]

Vlad - can you also land on 1.8 branch and mark fixed1.8?

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in on branch.