Last Comment Bug 801874 - Opening a minimized chat is broken
: Opening a minimized chat is broken
Status: VERIFIED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Firefox 19
Assigned To: :Felipe Gomes (needinfo me!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-15 14:41 PDT by :Felipe Gomes (needinfo me!)
Modified: 2012-12-04 13:52 PST (History)
3 users (show)
felipc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
Patch (1.12 KB, patch)
2012-10-15 14:41 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (986 bytes, patch)
2012-10-15 15:09 PDT, :Felipe Gomes (needinfo me!)
gavin.sharp: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2012-10-15 14:41:56 PDT
Created attachment 671609 [details] [diff] [review]
Patch

When opening a chat window with the minimized option set to true, we set chatbox.minimized = true here: http://hg.mozilla.org/mozilla-central/annotate/cd3270dc35cc/browser/base/content/socialchat.xml#l328

However, this happens before we insert the element to the DOM, so the minimized getter/setter from the binding is not triggered, and this ends up as a simple property assignment. Also, since the property is defined, the binding doesn't overwrite it, so the toggle() function that changes cb.minimized doesn't work in this case.

By setting it afterwards the problem is fixed, and also the isActive property of that chat gets properly set. Note though that the socialFrameHide event will also be dispatched, so there's a change in behavior with this patch (which could be avoided if done in a different manner, but that sounds like the right behavior, so I'll let others chime in)
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-15 14:47:59 PDT
Couldn't you just set the attribute instead? That way the panel opens as minimized, rather than opening normally and then being minimized.
Comment 2 :Felipe Gomes (needinfo me!) 2012-10-15 15:06:02 PDT
Yeah, that's what I was thinking as the alternate approach. Do you think it's better that way? The only problem of not using the setter is getting it out of sync with this opening code if we add more things to setter later. But that is likely not a problem as whoever writes the code will search for "minimized" and find this in the same file.
Comment 3 :Felipe Gomes (needinfo me!) 2012-10-15 15:09:33 PDT
Created attachment 671616 [details] [diff] [review]
Patch v2

approach suggested by gavin. Turns out setting isActive and dispatching socialFrameHide will happen anyways due to the DOMContentLoaded handler
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-15 15:11:12 PDT
Comment on attachment 671616 [details] [diff] [review]
Patch v2

Yeah, I think it's unlikely that we'll make the "minimized" setter any more complicated, and if we do we'll likely take this into account since it's hard to miss.
Comment 5 :Felipe Gomes (needinfo me!) 2012-10-15 15:13:42 PDT
ok cool, i'm adding a test in a moment and another one might need to be changed
Comment 6 :Felipe Gomes (needinfo me!) 2012-10-15 16:19:10 PDT
added a check for this in the existing chatwindow test. Other parts of the test did not need to be updated

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc55531de631
Comment 7 Ed Morley [:emorley] 2012-10-16 01:20:22 PDT
https://hg.mozilla.org/mozilla-central/rev/cc55531de631
Comment 8 :Felipe Gomes (needinfo me!) 2012-10-18 16:17:11 PDT
Comment on attachment 671616 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature, part of Social API
User impact if declined: chats automatically opened by the worker will open full size and can't be minimized
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): very small on Firefox's side, but the provider needs to fix an issue that their chat layout is broken when open as minimized.
(If we do not take this patch we should take at least an alternate patch on the branches that allows chats to still be opened as full size but fix the fact that they can't be minimized)

String or UUID changes made by this patch: none
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-18 16:41:41 PDT
Comment on attachment 671616 [details] [diff] [review]
Patch v2

a=me
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 13:52:27 PST
Marking verified fixed since this landed with tests. Please add verifyme keyword if there's something you need QA to test.

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