Closed Bug 1289549 Opened 8 years ago Closed 8 years ago

SocialAPI deprecation

Categories

(Firefox Graveyard :: SocialAPI, defect)

Product:
Firefox Graveyard
Component:
SocialAPI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 affected, firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(8 files, 15 obsolete files)

Bug 1289549 fix test driver child counting,
58 bytes, text/x-review-board-request
Details
Bug 1289549 P1 socialapi remove status and marks,
58 bytes, text/x-review-board-request
Details
Bug 1289549 P2 socialapi remove sidebar and chat,
58 bytes, text/x-review-board-request
Details
Bug 1289549 P3 socialapi move toolkit component to browser,
58 bytes, text/x-review-board-request
Details
Bug 1289549 P4 socialapi remove unused css,
58 bytes, text/x-review-board-request
Details
Bug 1289549 P5 socialapi fix path in pocket bootstrap,
58 bytes, text/x-review-board-request
Details
Bug 1289549 P6 socialapi remove non-socialapi chat code,
58 bytes, text/x-review-board-request
Details
Bug 1289549 bump slop in jpg color test,
58 bytes, text/x-review-board-request
Details 
Following up on the removal of Hello, much of socialapi will also be removed.

- remove chat window support (only used by hello)
- remove socialmarks and socialstatus (unused at this time)
- remove socialsidebar (a few providers, insignificant userbase)
  We are in process of contacting the providers who have sidebars.
- share will remain, it has a good set of users and providers
- removal will happen on nightly after hello is removed
- these changes will ride the train
All sidebar-supporting sites have been notified of the deprecation for 51.
Attached patch Remove status panel and marks (obsolete) — — Splinter Review 
Part 1 removes code for status and marks
Attachment #8775361 - Flags: feedback?(florian)
Attached patch part 2 sidebar, chatwindow, etc (obsolete) — — Splinter Review 
part 2 is the large patch, removing chat, sidebar, flyout, etc
Attachment #8775362 - Flags: feedback?(florian)
part 3 moves toolkit code into browser, no reason to have this in toolkit, and it's only one module now (mozsocial.jsm was removed)
Attachment #8775363 - Flags: feedback?(florian)
part 4 picks out the css and other stuff
Attachment #8775364 - Flags: feedback?(florian)
Florian, I'm sure there will be more to remove once the scope of hello removal is a little more clear.  For example, some chat tests that are not under the social directory.  

There is probably some additional work that could be done later, such as looking at combining/reducing Social.jsm and SocialService.jsm.  Social.jsm was meant in part as the browser interface to SocialService.

Social tests pass but I haven't run the full test suite since I know that prior to hello removal a lot of tests will break.
Comment on attachment 8775362 [details] [diff] [review]
part 2 sidebar, chatwindow, etc

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

::: browser/base/content/browser-social.js
@@ +282,5 @@
>        case "PageVisibility:Hide":
>          SocialShare._dynamicResizer.stop();
>          break;
> +      case "Social:DOMWindowClose":
> +        this.panel.hidePopup();

Why is new code needed?

::: browser/base/content/browser.js
@@ +2297,2 @@
>        // In the case of popups, we need to find a non-popup browser window.
>        if (!tabBrowser || !window.toolbar.visible) {

Does the comment you are removing mean we can remove the !tabBrowser check?

::: browser/base/content/content.js
@@ -710,5 @@
>  PageMetadataMessenger.init();
>  
>  addEventListener("ActivateSocialFeature", function (aEvent) {
>    let document = content.document;
> -  if (PrivateBrowsingUtils.isContentWindowPrivate(content)) {

Why is this check no longer relevant but the whole function still needed? Is this code path used for the Share feature? If so, was Share broken before for private browsing?

::: browser/base/content/nsContextMenu.js
@@ +1057,2 @@
>        // In the case of popups, we need to find a non-popup browser window.
>        if (!tabBrowser || !window.toolbar.visible) {

Does the comment you are removing mean that the !tabBrowser check is no longer needed?

::: browser/base/content/test/social/browser_social_activation.js
@@ +111,5 @@
>    });
>  }
>  
>  function activateOneProvider(manifest, finishActivation, aCallback) {
> +  info("activating provider "+manifest.name);

nit: spaces around '+'
Attachment #8775362 - Flags: feedback?(florian) → feedback+
Comment on attachment 8775361 [details] [diff] [review]
Remove status panel and marks

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

The only comment I had was that this should remove the associated strings from browser.dtd, but I see you did it in part2, so that's fine.
Attachment #8775361 - Flags: feedback?(florian) → feedback+
Attachment #8775363 - Flags: feedback?(florian) → feedback+
Attachment #8775364 - Flags: feedback?(florian) → feedback+
This looks good, thanks for preparing the patches!

The above reviews were by looking at the patches only, I haven't tried to apply them and test things; I assume it would not be very useful before the hello removal lands.
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 8775362 [details] [diff] [review]
> part 2 sidebar, chatwindow, etc
> 
> Review of attachment 8775362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-social.js
> @@ +282,5 @@
> >        case "PageVisibility:Hide":
> >          SocialShare._dynamicResizer.stop();
> >          break;
> > +      case "Social:DOMWindowClose":
> > +        this.panel.hidePopup();
> 
> Why is new code needed?

This was originally handed in toolkit/components/social/MozSocial.jsm which is removed in these patches.  This is a simplified form since it only has to handle the share panel.  This is the handler for window.close in a share panel.

> 
> ::: browser/base/content/browser.js
> @@ +2297,2 @@
> >        // In the case of popups, we need to find a non-popup browser window.
> >        if (!tabBrowser || !window.toolbar.visible) {
> 
> Does the comment you are removing mean we can remove the !tabBrowser check?

I don't think so, the comment left is about popups which may also not have tabBrowser.

> 
> ::: browser/base/content/content.js
> @@ -710,5 @@
> >  PageMetadataMessenger.init();
> >  
> >  addEventListener("ActivateSocialFeature", function (aEvent) {
> >    let document = content.document;
> > -  if (PrivateBrowsingUtils.isContentWindowPrivate(content)) {
> 
> Why is this check no longer relevant but the whole function still needed? Is
> this code path used for the Share feature? If so, was Share broken before
> for private browsing?

When I removed the frameWorker a while back, I removed all code preventing social running in a private window, but aparently missed this one.  FrameWorker was the sole reason for avoiding private windows since it could cross between the two.  So basically, you could use social (share) in private windows, but you couldn't add a social provider.  There's no reason for that limitation.

> ::: browser/base/content/nsContextMenu.js
> @@ +1057,2 @@
> >        // In the case of popups, we need to find a non-popup browser window.
> >        if (!tabBrowser || !window.toolbar.visible) {
> 
> Does the comment you are removing mean that the !tabBrowser check is no
> longer needed?

As per above similar code comment.
(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> This looks good, thanks for preparing the patches!
> 
> The above reviews were by looking at the patches only, I haven't tried to
> apply them and test things; I assume it would not be very useful before the
> hello removal lands.

Correct.  You could apply and only run the social tests, they pass.  But it's best to just wait and we'll do a try run after hello is removed.
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)

> > ::: browser/base/content/browser.js
> > @@ +2297,2 @@
> > >        // In the case of popups, we need to find a non-popup browser window.
> > >        if (!tabBrowser || !window.toolbar.visible) {
> > 
> > Does the comment you are removing mean we can remove the !tabBrowser check?
> 
> I don't think so, the comment left is about popups which may also not have
> tabBrowser.

I think popup windows are browser.xul windows, so they should have gBrowser.

Looking at https://hg.mozilla.org/mozilla-central/rev/524ba07b73eb seems to confirm that the !tabBrowser check wasn't related to popups.
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> 
> > > ::: browser/base/content/browser.js
> > > @@ +2297,2 @@
> > > >        // In the case of popups, we need to find a non-popup browser window.
> > > >        if (!tabBrowser || !window.toolbar.visible) {
> > > 
> > > Does the comment you are removing mean we can remove the !tabBrowser check?
> > 
> > I don't think so, the comment left is about popups which may also not have
> > tabBrowser.
> 
> I think popup windows are browser.xul windows, so they should have gBrowser.
> 
> Looking at https://hg.mozilla.org/mozilla-central/rev/524ba07b73eb seems to
> confirm that the !tabBrowser check wasn't related to popups.

Well, we could remove it then, I'm not sure that really matters though since tabBrowser is set in that block.
I've already archived the documentation; however, we need to add a mention of this to the Firefox X for developers page once it's known for certain which release this will take effect in.
Keywords: dev-doc-needed
(In reply to Eric Shepherd [:sheppy] from comment #14)
> I've already archived the documentation; however, we need to add a mention
> of this to the Firefox X for developers page once it's known for certain
> which release this will take effect in.

Share is not deprecated, so a chunk of this documentation needs to remain active.
Flags: needinfo?(eshepherd)
Group: mozilla-employee-confidential
Review commit: https://reviewboard.mozilla.org/r/68986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68986/
Attachment #8777441 - Flags: review?(florian)
Attachment #8777442 - Flags: review?(florian)
Attachment #8777443 - Flags: review?(florian)
Attachment #8777444 - Flags: review?(florian)
Attachment #8777445 - Flags: review?(florian)
Attachment #8777446 - Flags: review?(florian)
Attachment #8775361 - Attachment is obsolete: true
Attachment #8775362 - Attachment is obsolete: true
Attachment #8775363 - Attachment is obsolete: true
Attachment #8775364 - Attachment is obsolete: true
Comment on attachment 8777441 [details]
Bug 1289549 fix test driver child counting,

https://reviewboard.mozilla.org/r/68986/#review66352
Attachment #8777441 - Flags: review?(florian) → review+
Comment on attachment 8777442 [details]
Bug 1289549 P2 socialapi remove sidebar and chat,

https://reviewboard.mozilla.org/r/68988/#review66370
Attachment #8777442 - Flags: review?(florian) → review+
Attachment #8777443 - Flags: review?(florian) → review+
Comment on attachment 8777444 [details]
Bug 1289549 P4 socialapi remove unused css,

https://reviewboard.mozilla.org/r/68992/#review66378
Attachment #8777444 - Flags: review?(florian) → review+
Comment on attachment 8777445 [details]
Bug 1289549 P5 socialapi fix path in pocket bootstrap,

https://reviewboard.mozilla.org/r/68994/#review66380

Looks like this should be folded into P3, but I don't mind if you prefer to keep it separate.
Attachment #8777445 - Flags: review?(florian) → review+
Comment on attachment 8777446 [details]
Bug 1289549 P6 socialapi remove non-socialapi chat code,

https://reviewboard.mozilla.org/r/68996/#review66382
Attachment #8777446 - Flags: review?(florian) → review+
It's taken some effort, but I've figured out that the tests are failing if the sidebar browser (id="social-sidebar-browser") is removed from browser.xul.  Even though any code using it is removed, that has no affect so long as the browser element exists.  Once removed, MR and WR tests fail.  The failure is a popup window that is opened to run tests in (rather than in a tab in the main window).  I've yet to figure out why the sidebar browser is important to these tests.
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> (In reply to Eric Shepherd [:sheppy] from comment #14)
> > I've already archived the documentation; however, we need to add a mention
> > of this to the Firefox X for developers page once it's known for certain
> > which release this will take effect in.
> 
> Share is not deprecated, so a chunk of this documentation needs to remain
> active.

Okay, someone had originally told me the entire thing was deprecated, before I knew about this bug, so I'll go fix it.
Flags: needinfo?(eshepherd)
James, I was wondering if you might have some input on wpt failure caused by the removal of the social sidebar browser (see comment 34 above).
Flags: needinfo?(james)
OK, the following changes have been made:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API has "obsolete since 51" badges next to the bookmarks, MozSocial, and status APIs, and next to the "Adding bookmark support" and "Supporting chat" articles.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Widgets now lists all of the widgets as obsolete starting in Firefox 51, except for the share panel.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Guide/FirstSteps has a warning box that the page uses features no longer supported; it should be properly updated but honestly I'm not sure what we can leave here in place of the simple sidebar created there. Ideas?

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Guide/Share has been tidied up slightly.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Manifest has been updated to note which things are obsolete and when and to do some formatting cleanup.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Glossary says to look to the standard service worker API, and lists which terms refer to obsolete technologies.

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/mozSocial and https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/MozSocial both updated to mention the deprecation (along with the subpages of the latter).

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Bookmarks now says it's obsolete.
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Social_API/Status lists itself as obsolete.

Firefox 51 for developers updated: https://developer.mozilla.org/en-US/Firefox/Releases/51
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: dev-doc-needed → dev-doc-complete
Resolution: --- → FIXED
It looks to me like you have broken window.open(), possibly with width and height settings, when running under marionette. That would explain why Wr is broken (because it opens some fixed-size windows) and also seems to be the failures that you are seeing with the Mn tests.
Flags: needinfo?(james)
(In reply to James Graham [:jgraham] from comment #38)
> It looks to me like you have broken window.open(), possibly with width and
> height settings, when running under marionette. That would explain why Wr is
> broken (because it opens some fixed-size windows) and also seems to be the
> failures that you are seeing with the Mn tests.

Interesting.  I'm not clear how removing a browser element unrelated to anything in Wr/Mn would break window.open.  I also don't have any code in Wr/Mn related to socialapi.  When running Wr/Mn the windows they open *do* open, but the tests inside don't run.  Put the social sidebar browser element back, and they run.  Is there anything in Wr/Mn that is selecting a browser element to run in via some query/etc., or could they be injecting some additional elements into the window that could be broken by some structure change?
Flags: needinfo?(james)
James, are you thinking that the window size changed by removing the browser element and that is breaking the tests?  The element is hidden by default so I wouldn't think it does.
So looking more closely, it seems that opening the window probably succeeds, but subsequent calls to switch_to_window fail to switch to the newly opened window. So it seems entirely plausible that this is related to the DOM structure changing, although I am not an expert on the marionette server so I don't know exactly what's going on here. However I think the relevant code is [1] if you want to investigate more closely. Otherwise I suggest asking ato (but he's away for a few days) or perhaps AutomatedTester.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#1280
Flags: needinfo?(james) → needinfo?(ato)
I've further narrowed this today.  

The problem seems to stem from GeckoDriver.prototype.whenBrowserStarted where it sets this.curBrowser.frameRegsPending = mm.childCount;

Regardless of whether the social sidebar browser is in the window or not, mm.childCount == 2.  Two messages for Marionette:register are expected due to that.  If the social sidebar browser is not in the window, only one is received.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8777442 - Attachment is obsolete: true
Attachment #8777443 - Attachment is obsolete: true
Attachment #8777444 - Attachment is obsolete: true
Attachment #8777445 - Attachment is obsolete: true
Attachment #8777446 - Attachment is obsolete: true
I'm not sure why my other patches were obsoleted when adding the new patch :(
The previous patches are already reviewed by florian, just getting review on the change to the marrionette driver to fix Mn/Wr failures that caused the backout.

A try with the test change only is at (had pushed to try with wrong bug#)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df333b9a1cb8

A try with all the patches including test fix is at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df1fb7f596e8
Keywords: site-compat
Attachment #8777441 - Flags: review?(dburns)
Comment on attachment 8777441 [details]
Bug 1289549 fix test driver child counting,

https://reviewboard.mozilla.org/r/68986/#review69546
Attachment #8777441 - Flags: review?(dburns) → review+
Attachment #8777441 - Flags: review?(ato)
whimboo: apply patches in order:

0 U testing: Bug 1289549 fix test driver child counting, r=automatedtester
1 U social: Bug 1289549 P1 socialapi remove status and marks, r=florian
2 U sidebar: Bug 1289549 P2 socialapi remove sidebar and chat, r=florian
3 U movetoolkit: Bug 1289549 P3 socialapi move toolkit component to browser, r=florian
4 U socialcss: Bug 1289549 P4 socialapi remove unused css, r=florian
5 U pocketsocial: Bug 1289549 P5 socialapi fix path in pocket bootstrap, r=florian
6 U removechat: Bug 1289549 P6 socialapi remove non-socialapi chat code, r=florian

The run ./mach firefox-ui-functional you should get a failure pretty quickly with a new window opened.
Flags: needinfo?(mixedpuppy) → needinfo?(hskupin)
whimboo: I pulled, built (not artifact) and verified that fx-ui tests work without my patches, hangs with patches on TestBaseWindow.test_open_close

./mach firefox-ui-functional testing/firefox-ui/tests/puppeteer/test_windows.py
It seems my prior marionette fix was not entirely correct, above try has a modification to that patch which, at least locally, fixes fx-ui tests as well as the wp tests that were broke in the first place.

I've no idea why the failure in comment 57 happened, I cannot repro locally, we'll see if it is otherwise fixed via try.
All the fx-ui-tests were running on desktop-test workers before which is a single core instance at AWS. With the change to desktop-test-large last Friday we make use of multi-core cpu now. I assume your last try push includes the latest changeset from fxteam, which has this instance change included. Also because you will have a multi-core machine locally, it would have been hard to reproduce for you.
Flags: needinfo?(hskupin)
whimboo: I don't think the desktop-test changes have anything to do with my test failures.  The change here:

https://hg.mozilla.org/try/rev/5b3409139518b1ba5c90f8f8d6f602a348310d9b

Fixes most of them, including the fx-ui problem we chatted about yesterday.  I still have one other bug that I don't think is marionette related (the failure in comment 57).
Attachment #8781285 - Attachment is obsolete: true
Attachment #8781285 - Flags: review?(florian)
Attachment #8781286 - Attachment is obsolete: true
Attachment #8781286 - Flags: review?(florian)
Attachment #8781287 - Attachment is obsolete: true
Attachment #8781287 - Flags: review?(florian)
Attachment #8781288 - Attachment is obsolete: true
Attachment #8781288 - Flags: review?(florian)
Attachment #8781289 - Attachment is obsolete: true
Attachment #8781289 - Flags: review?(florian)
Attachment #8781290 - Attachment is obsolete: true
Attachment #8781290 - Flags: review?(florian)
Flags: needinfo?(ato)
As a note, this patch had a performance improvement on tpaint test in talos, thanks!
https://treeherder.mozilla.org/perf.html#/alerts?id=2709
Attachment #8784138 - Flags: review?(florian)
Attachment #8784139 - Flags: review?(florian)
Attachment #8784140 - Flags: review?(florian)
Attachment #8784141 - Flags: review?(florian)
Attachment #8784142 - Flags: review?(florian)
Attachment #8784143 - Flags: review?(florian)
Depends on: 1373065
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: