In Bug 1163254 we will update the nsIPrincipal.origin for new security model. So right now in RequestSyncService.jsm it still uses appId/isBrowser/origin as key, we should use origin for this.

depends on Bug 1168300 as RequestSyncService.jsm will listen to webapps-clear-data event and QI mozIApplicationClearPrivateDataParams to get appId and browserOnly.

Comment on attachment 8615154 [details] [diff] [review] Patch. Hi Ehsan, could you help to review this patch as you reviewed the original patch from Bug 1918320. Also feedback? on Bobby for the usage of the nsIPricipal. There are some other places will use origin and isInBrowserElement, but I think they are more related to revise RequestSyncApp in WebIDL and deserve another bug for this. Feel free to comment if I should also do it in this bug. Thanks

Comment on attachment 8615154 [details] [diff] [review] Patch. Review of attachment 8615154 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/requestsync/RequestSyncService.jsm @@ +198,5 @@ > }, > > // This method generates the key for the indexedDB object storage. > principalToKey: function(aPrincipal) { > + return aPrincipal.cookieJar + '|' + aPrincipal.originNoSuffix; I think you should be using aPrincipal.origin here.

Comment on attachment 8615154 [details] [diff] [review] Patch. Review of attachment 8615154 [details] [diff] [review]: ----------------------------------------------------------------- Yes, we should be using .origin here. The .originNoSuffix stuff was there so that the previous change I made didn't change the format for any persistent data. Now that we're changing it we should switch to .origin, and we also need to couple it with a migration scheme for any persistent data, or an explanation of why no migration is needed.

Yeah, actually my first attemp is to use 'orgin' as key, but when we receive "webapps-clear-cookiejar-data", it's 'cookieJar' contained in the notification, so I need to do some mapping from the cookieJar({digit}:(t|f)) to the origin (!appId={digit}&inBrowser=1), I'll check what's the easy way to do this. Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #6) > Yeah, actually my first attemp is to use 'orgin' as key, but when we receive > "webapps-clear-cookiejar-data", it's 'cookieJar' contained in the > notification, so I need to do some mapping from the cookieJar({digit}:(t|f)) > to the origin (!appId={digit}&inBrowser=1), I'll check what's the easy way > to do this. > > Thanks We don't want to try to map from cookieJar to OriginAttributes - conceptually, it's a one-way mapping. Why do we need to do that? The notification contains the cookieJar, which is the first part of the name. Ehsan and I were suggesting that full origin for the second part of the name, since we have the principal at that point.

Comment on attachment 8615154 [details] [diff] [review] Patch. Review of attachment 8615154 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/requestsync/RequestSyncService.jsm @@ +198,5 @@ > }, > > // This method generates the key for the indexedDB object storage. > principalToKey: function(aPrincipal) { > + return aPrincipal.cookieJar + '|' + aPrincipal.originNoSuffix; Oh, so you're saying this should be return aPrincipal.cookieJar + '|' + aPrincipal.origin; I thought you're saying return aPrincipal.origin; Sorry for my misunderstanding :P

(In reply to Yoshi Huang[:allstars.chh] from comment #8) > Oh, so you're saying this should be > return aPrincipal.cookieJar + '|' + aPrincipal.origin; Yes. The first part is the tag so that we can find it when we get a webapps-clear-cookiejar-data notification, and the second part is the actual origin. You can think of it as two imaginary columns in an SQL database.

Comment on attachment 8616553 [details] [diff] [review] Patch v2. use principal.origin in principalToKey also change to listen to "clear-clearjar-data".

Comment on attachment 8616553 [details] [diff] [review] Patch v2. Review of attachment 8616553 [details] [diff] [review]: ----------------------------------------------------------------- Per comment 5, we still need a migration plan or an analysis of why we don't need to do any migrating, right?

(In reply to Bobby Holley (:bholley) from comment #5) > Now that we're changing it we should switch to .origin, and we also need to > couple it with a migration scheme for any persistent data, or an explanation > of why no migration is needed. Hi Bobby Sorry I didn't notice last part. I've noticed that the RequestSyncApp should be changed as well, but this could be done as seperate bug as it requires more change. So in Comment 3 I've asked if we could do this as follow-up. Does this answer your concern? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #13) > (In reply to Bobby Holley (:bholley) from comment #5) > > Now that we're changing it we should switch to .origin, and we also need to > > couple it with a migration scheme for any persistent data, or an explanation > > of why no migration is needed. > > Hi Bobby > Sorry I didn't notice last part. > I've noticed that the RequestSyncApp should be changed as well, but this > could be done as seperate bug as it requires more change. So in Comment 3 > I've asked if we could do this as follow-up. > > Does this answer your concern? > > Thanks I'm not sure I understand - my concern is that we're using indexedDB here to store things, and the format of those things is changing in this patch. So we need to understand what happens to a user's data if they upgrade (or downgrade) versions. Is there a reason that doesn't apply here?

Comment on attachment 8616553 [details] [diff] [review] Patch v2. Review of attachment 8616553 [details] [diff] [review]: ----------------------------------------------------------------- Asked baku on irc, he said I should send a mail to dev-b2g/dev-gaia to see if anyone is already using syncManager, if no we could get rid of the older one.

I've sent mail to dev-b2g/gaia, if no further responses until next Monday (6/15) I'll ask feedback from bholley and baku (original requestSync author).

Comment on attachment 8616553 [details] [diff] [review] Patch v2. Hi Bobby and baku I've sent mail to dev-b2g/dev-gaia to ask the usage of syncManager API last week, and it seems no one is using it yet. So I'll just update the dbkey directly without bumping the DB Version. feedback? to bobby to the usage of nsIPrincipal and feedback? to baku to check if I've missed anything, Thanks

Comment on attachment 8616553 [details] [diff] [review] Patch v2. Review of attachment 8616553 [details] [diff] [review]: ----------------------------------------------------------------- Ok sounds good then.