Incorrect behavior for "Save Page to Demo Social Service" icon (flag icon) for HTTPS pages

VERIFIED FIXED in Firefox 32

Status

()

Firefox
SocialAPI
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: cbadau, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 34
Points:
---
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox31 wontfix, firefox32+ verified, firefox33+ verified, firefox34+ verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Reproducible on Firefox 32 beta 3
Mozilla/5.0 (Windows NT 5.1; rv:32.0) Gecko/20100101 Firefox/32.0
Reproducible on latest Nightly (2014-07-31)
Mozilla/5.0 (Windows NT 5.1; rv:34.0) Gecko/20100101 Firefox/34.0
Reproducible on latest Aurora (2014-07-31)
Mozilla/5.0 (Windows NT 5.1; rv:33.0) Gecko/20100101 Firefox/33.0


Steps to reproduce: 
1. Start Firefox. 
2. Navigate to http://mixedpuppy.github.io/socialapi-demo/ and activate the social service.
3. Click the "Sign in" button to automatically sign in.
4. Open a new tab, go to https://www.youtube.com/, and click the flag icon at the top right ("Save Page to Demo Social Service" icon). 
6. Open a new tab or a new window and go to https://www.youtube.com/. 


Expected results: 
After step 4, the flag icon become red and the website is social marked. 
After step 5, the website is social marked indicated through red flag icon. 

Actual resuls: 
After step 4, the flag icon become red and the website is social marked (as expected). 
After step 5, the website is no longer social marked (the flag icon is no longer red). This is applicable only for https page. If you enter www.youtube.com, it works as expected: the flag is red and the website is social marked. 

Notes: 
1. This issue is reproducible also on Mac OSX 10.8.5. 
2. This issue seems to also reproduce in Firefox 29.
[Tracking Requested - why for this release]:
status-firefox31: --- → wontfix
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
tracking-firefox32: --- → ?
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
Flags: firefox-backlog?
(Assignee)

Comment 2

4 years ago
Created attachment 8471017 [details] [diff] [review]
use browser url, not cannonical url

This is caused because youtube sets the cannonical url, which we use to annotate, to http regardless.  It would be more correct for us to use the url in the browser rather than the cannonical url from their html (and that fixes this issue).
Assignee: nobody → mixedpuppy
Attachment #8471017 - Flags: review?(jaws)
Comment on attachment 8471017 [details] [diff] [review]
use browser url, not cannonical url

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

::: browser/base/content/socialmarks.xml
@@ +161,5 @@
> +        let markURI = gBrowser.currentURI.spec;
> +        if (!pageData) {
> +          pageData = OpenGraphBuilder.getData(gBrowser);
> +        } else {
> +          markURI = pageData.url;

I think this will read better if it is changed to:

> let markURI;
> if (pageData) {
>   markURI = pageData.url;
> } else {
>   pageData = OpenGraphBuilder.getData(gBrowser);
>   markURI = gBrowser.currentURI.spec;
> }

I was quite confused reading it as it is currently written.

@@ +202,2 @@
>              } else {
> +              Social.unmarkURI(provider.origin, gBrowser.currentURI, () => {

Can this just be:
> Social.unmarkURI(provider.origin, gBrowser.currentURI, this.update);
?
Attachment #8471017 - Flags: review?(jaws) → review+
Summary: Incorect behaviour for "Save Page to Demo Social Service" icon (flag icon) for https pages → Incorrect behavior for "Save Page to Demo Social Service" icon (flag icon) for HTTPS pages
(Assignee)

Comment 4

4 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Comment on attachment 8471017 [details] [diff] [review]
> use browser url, not cannonical url
> 
> Review of attachment 8471017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/socialmarks.xml
> @@ +161,5 @@
> > +        let markURI = gBrowser.currentURI.spec;
> > +        if (!pageData) {
> > +          pageData = OpenGraphBuilder.getData(gBrowser);
> > +        } else {
> > +          markURI = pageData.url;
> 
> I think this will read better if it is changed to:

sigh.  I forgot to hg qrefresh before uploading the patch, the first change section of this patch is unecessary, I'll update the patch here.

> @@ +202,2 @@
> >              } else {
> > +              Social.unmarkURI(provider.origin, gBrowser.currentURI, () => {
> 
> Can this just be:
> > Social.unmarkURI(provider.origin, gBrowser.currentURI, this.update);
> ?

In that case I'd have to: this.update.bind(this).
(Assignee)

Comment 5

4 years ago
Created attachment 8471213 [details] [diff] [review]
use browser url, not cannonical url

remove first part of patch (forgot qrefresh before prior upload)
Attachment #8471017 - Attachment is obsolete: true
Attachment #8471213 - Flags: review+
tracking-firefox32: ? → +
tracking-firefox33: ? → +
tracking-firefox34: ? → +
Shane - Final beta 32 goes to build on Thu. Last comment from you was a week ago? Any progress?
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 9

4 years ago
Comment on attachment 8471213 [details] [diff] [review]
use browser url, not cannonical url

Approval Request Comment
[Feature/regressing bug #]: social bookmarking
[User impact if declined]: some annotations will be incorrect since we're annotating the canonical url rather than the url the user is visiting.  This bug is not severe, but is also very minimal with little if any risk in uplifting through beta.  If we only uplift through aurora, it's not a big deal.
[Describe test coverage new/current, TBPL]: no new tests written
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8471213 - Flags: approval-mozilla-beta?
Attachment #8471213 - Flags: approval-mozilla-aurora?
Flags: firefox-backlog? → firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/92c62c837623
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox34: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Attachment #8471213 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8471213 [details] [diff] [review]
use browser url, not cannonical url

This fix looks pretty safe. Given the work that we're doing with social in 32, I'm going to take this for beta9.
Attachment #8471213 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
(Reporter)

Comment 14

4 years ago
Verified fixed on: 
- Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.4 using latest Nightly 34.0a1 (buildID: 20140821030201) and latest Aurora 33.0a2 (buildID: 20140821004002)
- Ubuntu 13.10 32bit and Mac OSX 10.9.4 using Firefox 32 Beta 9 (buildID: 20140822024446)
(Reporter)

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
status-firefox34: fixed → verified
Verified on Win7 with Fx32b9(build1) using the steps in comment #0. I get the expected results.
status-firefox32: fixed → verified

Updated

3 years ago
Iteration: --- → 34.3

Updated

3 years ago
QA Contact: camelia.badau
You need to log in before you can comment on or make changes to this bug.