Status

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

Trunk
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(8 attachments, 15 obsolete attachments)

58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 2

3 years ago
Posted patch Remove status panel and marks (obsolete) — Splinter Review
Part 1 removes code for status and marks
Attachment #8775361 - Flags: feedback?(florian)
(Assignee)

Comment 3

3 years ago
part 2 is the large patch, removing chat, sidebar, flyout, etc
Attachment #8775362 - Flags: feedback?(florian)
(Assignee)

Comment 4

3 years ago
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)
(Assignee)

Comment 5

3 years ago
part 4 picks out the css and other stuff
Attachment #8775364 - Flags: feedback?(florian)
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
(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.
(Assignee)

Comment 13

3 years ago
(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
(Assignee)

Comment 15

3 years ago
(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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(eshepherd)
(Assignee)

Updated

3 years ago
Group: mozilla-employee-confidential
(Assignee)

Comment 16

3 years ago
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)
(Assignee)

Updated

3 years ago
Attachment #8775361 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8775362 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8775363 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 34

3 years ago
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)
(Assignee)

Comment 36

3 years ago
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
Last Resolved: 3 years ago
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)
(Assignee)

Comment 39

3 years ago
(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)
(Assignee)

Comment 40

3 years ago
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)
(Assignee)

Comment 42

3 years ago
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.
(Assignee)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Attachment #8777442 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8777443 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8777444 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8777445 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8777446 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 52

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8777441 - Flags: review?(dburns)

Comment 54

3 years ago
mozreview-review
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+
(Assignee)

Updated

3 years ago
Attachment #8777441 - Flags: review?(ato)
(Assignee)

Comment 58

3 years ago
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)
(Assignee)

Comment 59

3 years ago
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
(Assignee)

Comment 61

3 years ago
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)
(Assignee)

Comment 63

3 years ago
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).
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Attachment #8781285 - Attachment is obsolete: true
Attachment #8781285 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8781286 - Attachment is obsolete: true
Attachment #8781286 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8781287 - Attachment is obsolete: true
Attachment #8781287 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8781288 - Attachment is obsolete: true
Attachment #8781288 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8781289 - Attachment is obsolete: true
Attachment #8781289 - Flags: review?(florian)
(Assignee)

Updated

3 years ago
Attachment #8781290 - Attachment is obsolete: true
Attachment #8781290 - Flags: review?(florian)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Flags: needinfo?(ato)

Comment 77

3 years ago
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)

Updated

2 years ago
Depends on: 1373065

Updated

2 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.