Closed Bug 1175306 Opened 9 years ago Closed 9 years ago

Rely on documentURI as primary url in PageMetaData

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(firefox39 wontfix, firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch fix metadata url (obsolete) — Splinter Review
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.
Attachment #8623314 - Flags: review?(markh)
Attachment #8623314 - Flags: review?(markh) → review+
This is a better approach, it allows the existing behavior in most sites, fixes problems in both share and hello on sites using pushState.
Assignee: nobody → mixedpuppy
Attachment #8623314 - Attachment is obsolete: true
Attachment #8623426 - Flags: review?(markh)
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.
Attachment #8623426 - Flags: review?(markh) → review+
Attached patch pushstateSplinter Review
comments addressed + fix for existing test

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dd3c4dba16c
Attachment #8623426 - Attachment is obsolete: true
Attachment #8623736 - Flags: review+
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.
Attachment #8623736 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/da35fc16b6cf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Attachment #8623736 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.