Last Comment Bug 788100 - Remove social.initialize-response message
: Remove social.initialize-response message
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Mark Hammond [:markh]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-03 23:31 PDT by Mark Hammond [:markh]
Modified: 2012-10-16 16:04 PDT (History)
5 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Remove the message (4.56 KB, patch)
2012-09-04 19:35 PDT, Mark Hammond [:markh]
jaws: review+
mixedpuppy: feedback+
Details | Diff | Review
updated tests (6.25 KB, patch)
2012-09-11 19:52 PDT, Mark Hammond [:markh]
markh: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Review

Description Mark Hammond [:markh] 2012-09-03 23:31:42 PDT
WorkerAPI.jsm has support for a 'social.initialize-response' message, which sets 'this.initialized' to true.  However, this message and attribute are only used by the test suite and can almost certainly be removed without losing any test coverage or functionality.  The way it is setup implies it is a "real" message which must be sent by providers (and no current providers do that).  Thus it should be removed (or worst-case, replaced with something which makes it obvious it is purely for internal tests and is not expected to be implemented by real providers).
Comment 1 Mark Hammond [:markh] 2012-09-04 19:35:18 PDT
Created attachment 658339 [details] [diff] [review]
Remove the message

Turns out to be relatively simple to remove.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=28fc9d15e3b8
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-04 19:44:59 PDT
I expected this to be implemented by real providers, which is why I added it. I didn't do a very good job of advertising that expectation, though, which explains why no one does :)

Some social functionality depends on the livelihood of the worker (i.e. whether the worker has actually responded, rather than just whether we had attempted to create it and were queuing messages for it). I envisioned this as the mechanism for client code to detect that - for example, to only show the "share" button when we knew that the message it sends will be received.
Comment 3 Mark Hammond [:markh] 2012-09-04 21:15:20 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Some social functionality depends on the livelihood of the worker (i.e.
> whether the worker has actually responded, rather than just whether we had
> attempted to create it and were queuing messages for it). I envisioned this
> as the mechanism for client code to detect that - for example, to only show
> the "share" button when we knew that the message it sends will be received.

I'm not sure that would make much sense for the "share" button, as we need to get an image from the provider.  I guess I don't really see what this tells us - eg, just because it responds with this message doesn't seem to imply it will certainly respond to a social.user-recommend-prompt message.

I'm happy if you want this closed, but I'd think that means we should open new bugs for places where workerapi.initialized should be checked but currently is not - and TBH I can't think of where that would be (it is currently not used *anywhere* by the runtime code)
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-05 15:11:33 PDT
The addition of this message pre-dates the user-recommend-prompt stuff (bug 780987). I'm not objecting to the removal, I just wanted to make it clear why this exists. Given that there's a suitable alternative now, I think you're right that removing it makes sense.
Comment 5 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-09-07 12:07:56 PDT
Comment on attachment 658339 [details] [diff] [review]
Remove the message

I think this is fine to remove until we know we have a use for it.
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-09-10 02:31:20 PDT
Comment on attachment 658339 [details] [diff] [review]
Remove the message

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

::: toolkit/components/social/test/browser/browser_workerAPI.js
@@ -30,1 @@
>    

nit: can you also remove this extra whitespace?
Comment 7 Mark Hammond [:markh] 2012-09-11 19:52:03 PDT
Created attachment 660290 [details] [diff] [review]
updated tests

New patch with trivial change in the tests, carrying r+ forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df2ddcab7143
Comment 8 Phil Ringnalda (:philor) 2012-09-11 21:01:31 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f496fceb64ad for xpcshell test_SocialService.js bustage.
Comment 10 Ed Morley [:emorley] 2012-09-13 13:08:11 PDT
https://hg.mozilla.org/mozilla-central/rev/4a1b8c9f1184
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-25 11:41:28 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe8907cef50e

Note You need to log in before you can comment on or make changes to this bug.