898192 - Social API does unnecessary work in onLocationChange when it's disabled

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Matthew N. [:MattN]]

Attachment: wip.1 Skip social function calls when !SocialUI.enabled

onLocationChange calls the following functions even when SocialAPI is disabled:
          SocialMark.updateMarkState();
          SocialShare.update();

These functions do more work than necessary for this hot code path.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=414a9b7337bb

Comment 1

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

Comment on attachment 781298 [details] [diff] [review]
wip.1 Skip social function calls when !SocialUI.enabled

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

that looks right to me!

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 781298 [details] [diff] [review]
wip.1 Skip social function calls when !SocialUI.enabled

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

This looks good to me, anything else needed here? Only thing that I wonder is if we can make Social.enabled faster so it can cache the state until something makes it dirty.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] from comment #2)
> Only thing that I wonder
> is if we can make Social.enabled faster so it can cache the state until
> something makes it dirty.

I take this back. Social.enabled only checks the Social._provider private variable. Somehow I had thought that this in turn checked all of the providers to see if any of them were enabled, but I was wrong.

So this looks good to land from my POV as long as all tests pass.

Comment 4

[Matthew N. [:MattN]]

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

Comment 5

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

This made it to central - https://hg.mozilla.org/mozilla-central/rev/91c8657b0fa9

Comment 6

[u279076]

Matt, is there anything QA needs to do to verify this is fixed?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Verification for this shouldn't be necessary and it would be hard to verify anyways.

Comment 8

[u279076]

(In reply to Jared Wein [:jaws] from comment #7)
> Verification for this shouldn't be necessary and it would be hard to verify
> anyways.

Thanks Jared.