(no longer active) Assignee
	Description • 8 years ago

      No description provided.

	(no longer active) Assignee
	Comment 2 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 3 • 8 years ago

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.

	(no longer active) Assignee
	Comment 4 • 8 years ago

(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?

	Ben Kelly [:bkelly, not reviewing]
	Comment 5 • 8 years ago

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

	(no longer active) Assignee
	Comment 6 • 8 years ago

(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...

	Ben Kelly [:bkelly, not reviewing]
	Comment 7 • 8 years ago

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.

	Josh Matthews [:jdm]
	Comment 8 • 8 years ago

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.

	(no longer active) Assignee
	Comment 9 • 8 years ago

(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?

	(no longer active) Assignee
	Comment 10 • 8 years ago

(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...

	Pulsebot
	Comment 11 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 12 • 8 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 13 • 8 years ago

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?