This patch also fixes an existing bug where we used to call RewriteEntriesSchema at the end of each individual migration, which is wrong since after the first call, sqlite will think that it's dealing with the final schema. So far we were lucky that we only changed the actual schema once.

Comment on attachment 8724449 [details] [diff] [review] Part 2: Store the Request referrerPolicy in the DOM Cache Review of attachment 8724449 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/DBSchema.cpp @@ +104,5 @@ > "response_principal_info TEXT NOT NULL, " > "cache_id INTEGER NOT NULL REFERENCES caches(id) ON DELETE CASCADE, " > > // New columns must be added at the end of table to migrate and > // validate properly. Can you move this comment to the end? So its clear to people adding further columns to the table? @@ +2511,5 @@ > > + // Now overwrite the master SQL for the entries table to remove the column > + // default value. This is also necessary for our Validate() method to > + // pass on this database. > + rv = RewriteEntriesSchema(aConn); Why do we want to rewrite the entries schema for all migrations? This is only needed for the few that need to remove a column default.

(In reply to Ben Kelly [PTO, back Feb 29][:bkelly] from comment #3) > @@ +2511,5 @@ > > > > + // Now overwrite the master SQL for the entries table to remove the column > > + // default value. This is also necessary for our Validate() method to > > + // pass on this database. > > + rv = RewriteEntriesSchema(aConn); > > Why do we want to rewrite the entries schema for all migrations? This is > only needed for the few that need to remove a column default. Sure! Should I set a boolean in the migration function when it needs to rewrite the schema and look at that here to determine whether to call RewriteEntriesSchema?

Is there a reason we can't just call it in the specific migrations that need it like the code currently does?

(In reply to Ben Kelly [PTO, back Feb 29][:bkelly] from comment #5) > Is there a reason we can't just call it in the specific migrations that need > it like the code currently does? Yes. If we do what we do today, once you upgrade from a really old version to a newer one so that RewriteEntriesSchema() gets called twice, the first call will update the schema to the *latest* version, not to the version that was the latest at the time that code was written, and in the second place you try to update the schema things will go south since sqlite's notion of the on-disk schema will be different than the actual on-disk schema. I noticed this when running the test_migration.js js. That test tries to update a DB from version 10 to the latest. With my patch applied, the upgrade to version 16 updates the schema to include the request_referrer_policy field in the entries table, and then the ALTER TABLE command in the upgrade to version 20 fails, since it thinks the request_referrer_policy field already exists (even though it doesn't exist in the on-disk contents of the table!) Sorry if this wasn't obvious. I tried to explain this in the commit message, but I'm happy to change that description to something more obvious if you have suggestions...

I see. Ok, maybe just set a boolean somehow that indicates "default column setting needs to be removed" and then do it once at the end. Thanks for the explanation.

Comment on attachment 8724448 [details] [diff] [review] Part 1: Implement Request.referrerPolicy Review of attachment 8724448 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentUtils.h @@ +2531,5 @@ > */ > static nsresult SetFetchReferrerURIWithPolicy(nsIPrincipal* aPrincipal, > nsIDocument* aDoc, > + nsIHttpChannel* aChannel, > + mozilla::net::ReferrerPolicy aReferrerPolicy = mozilla::net::RP_Default); Does this need to be a default argument? ::: dom/fetch/FetchDriver.cpp @@ +308,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > rv = > httpChan->SetReferrerWithPolicy(referrerURI, > + referrerPolicy == ReferrerPolicy::_empty ? This really belongs in separate variables for clarity; I have a really hard time deciphering nested ternary conditionals.

(In reply to Josh Matthews [:jdm] from comment #8) > ::: dom/base/nsContentUtils.h > @@ +2531,5 @@ > > */ > > static nsresult SetFetchReferrerURIWithPolicy(nsIPrincipal* aPrincipal, > > nsIDocument* aDoc, > > + nsIHttpChannel* aChannel, > > + mozilla::net::ReferrerPolicy aReferrerPolicy = mozilla::net::RP_Default); > > Does this need to be a default argument? I did that in order to avoid updating the other call sites, but there is only a few of them. Would you prefer me to make it non-default?

(In reply to :Ehsan Akhgari from comment #9) > (In reply to Josh Matthews [:jdm] from comment #8) > > ::: dom/base/nsContentUtils.h > > @@ +2531,5 @@ > > > */ > > > static nsresult SetFetchReferrerURIWithPolicy(nsIPrincipal* aPrincipal, > > > nsIDocument* aDoc, > > > + nsIHttpChannel* aChannel, > > > + mozilla::net::ReferrerPolicy aReferrerPolicy = mozilla::net::RP_Default); > > > > Does this need to be a default argument? > > I did that in order to avoid updating the other call sites, but there is > only a few of them. Would you prefer me to make it non-default? I'll just do this in order to be able to land this today...

https://hg.mozilla.org/integration/mozilla-inbound/rev/a87e26a12a98 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0d6bb9abee

https://hg.mozilla.org/mozilla-central/rev/a87e26a12a98 https://hg.mozilla.org/mozilla-central/rev/4d0d6bb9abee

I've added a page for this property: https://developer.mozilla.org/en-US/docs/Web/API/Request#Properties https://developer.mozilla.org/en-US/docs/Web/API/Request/referrerPolicy And mentioned it in the 47 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/47#Service_Worker_and_related_APIs Does that look ok?