1175306 - Rely on documentURI as primary url in PageMetaData

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: fix metadata url

This will fix sharing urls via hello and/or share on sites that use pushState, my intention is to land and uplift to 40.

history.pushState will update documentURI (with bug 1173215 fixed), but it is up to the website to update other meta tags.  Because of this, cannonical urls, opengraph tags, etc. may or may not be correct after pushState.  e.g. youtube does not update meta tags, but microdata is updated/correct.

PageMetaData currently overwrites its primary url with a canonical url (from meta tags or opengraph).  This change will separate those leaving url == documentURI.  

This fix does not address whether the state of additional meta data is correct or not, it only ensures a correct url is provided after pushState.  A later, more aggressive change will try to address stale metadata (bug 1155014) but in the meantime this should address the problem for code that does not rely on the extra metadata.

Comment 1

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cc865dca499

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: if history pushstate used, dont get metadata

This is a better approach, it allows the existing behavior in most sites, fixes problems in both share and hello on sites using pushState.

Comment 3

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf7d57d13d43

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8623426 [details] [diff] [review]
if history pushstate used, dont get metadata

Review of attachment 8623426 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser.ini
@@ +350,5 @@
>  [browser_private_browsing_window.js]
>  skip-if = buildapp == 'mulet'
>  [browser_private_no_prompt.js]
>  skip-if = buildapp == 'mulet'
> +[browser_pushstate.js]

I think it would be better if this test had PageMetadata in the name (ie, browser_PageMetadata_pushState.js)

::: browser/base/content/test/general/browser_pushstate.js
@@ +9,5 @@
> +  is(result.url, rooturi + "metadata_simple.html", "metadata url is correct");
> +  is(result.title, "Test Title", "metadata title is correct");
> +  is(result.description, "A very simple test page", "description is correct");
> +
> +  

nit - trailing space, and public domain header should be added to the file.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: pushstate

comments addressed + fix for existing test

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dd3c4dba16c

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/da35fc16b6cf

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8623736 [details] [diff] [review]
pushstate

pushed https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=da35fc16b6cf

preemptively requesting uplift since I'm OoO tomorrow.

Approval Request Comment
[Feature/regressing bug #]: hello & share
[User impact if declined]: sites that use pushState to change the document url are not properly shareable
[Describe test coverage new/current, TreeHerder]: new mochitest
[Risks and why]: low
[String/UUID change made/needed]: none

Can be uplifted separately, but also requires bug 1173215 uplift to fully fix the problem.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da35fc16b6cf

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fe497b4c40c