Last Comment Bug 715565 - two throbbers in main window
: two throbbers in main window
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: 11 Branch
: x86_64 All
: -- normal (vote)
: Thunderbird 12.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on: 721378
Blocks: tb-tabsontop
  Show dependency treegraph
 
Reported: 2012-01-05 10:36 PST by Andreas Nilsson (:andreasn)
Modified: 2012-01-31 10:31 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1 (1.64 KB, patch)
2012-01-11 09:25 PST, Mike Conley (:mconley) - (Needinfo me!)
bwinton: review+
bwinton: ui‑review-
Details | Diff | Splinter Review
Patch v2 (5.98 KB, patch)
2012-01-16 13:05 PST, Mike Conley (:mconley) - (Needinfo me!)
sid.bugzilla: review+
Details | Diff | Splinter Review
Patch v3 (12.89 KB, patch)
2012-01-19 14:10 PST, Mike Conley (:mconley) - (Needinfo me!)
sid.bugzilla: review+
Details | Diff | Splinter Review
Patch v4 (r+'d by sid0) (13.52 KB, patch)
2012-01-24 07:22 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v5 (r+'d by sid0) (13.52 KB, patch)
2012-01-25 07:20 PST, Mike Conley (:mconley) - (Needinfo me!)
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Andreas Nilsson (:andreasn) 2012-01-05 10:36:45 PST
It seems we have two throbbers in the main window now. One on the tabs that is currently loading and one on the main toolbar. I suggest that the one on the tab is just enough by default.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-01-11 09:25:37 PST
Created attachment 587728 [details] [diff] [review]
Patch v1

This is my first run at it.  Removes the throbber in Windows 7 as expected.  Haven't seen what it looks like in OSX yet.
Comment 2 Andreas Nilsson (:andreasn) 2012-01-11 14:53:08 PST
Seems to work fine on Linux. I can test on OSX tomorrow.
Comment 3 Andreas Nilsson (:andreasn) 2012-01-12 06:18:55 PST
And works fine on OSX as expected.
Comment 4 Mike Conley (:mconley) - (Needinfo me!) 2012-01-12 06:22:03 PST
Comment on attachment 587728 [details] [diff] [review]
Patch v1

Awesome! Well, that was pretty easy.

What do you think, Blake?
Comment 5 Blake Winton (:bwinton) (:☕️) 2012-01-12 14:12:41 PST
Comment on attachment 587728 [details] [diff] [review]
Patch v1

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

The code seems fine, so r=me, but...

It didn't actually remove the throbber from my existing profile, which I think we want to do, so I'm going to have to say ui-r-.
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-01-12 14:21:08 PST
Hm. So how do we go about disqualifying the throbber in the current toolbars, while keeping it in the customize dialog?  I wonder if this is another case where the migrateUI code I wrote would come in handy.
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 13:05:10 PST
Created attachment 588979 [details] [diff] [review]
Patch v2

Ok, this patch uses our fancy new migrateUI function to remove the throbber from the menubar in Linux and Windows, and from the mail-bar3 in OSX.
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 13:06:36 PST
Comment on attachment 588979 [details] [diff] [review]
Patch v2

Hey Sid,

Remember migrateUI?  Looks like we've got a chance to use it again.  Or is there a better way to put old default toolbar items out to pasture?

Anyhow, I hope you've got some cycles to take a peek at this.

-Mike
Comment 9 Siddharth Agarwal [:sid0] (inactive) 2012-01-19 12:14:30 PST
Comment on attachment 588979 [details] [diff] [review]
Patch v2

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

::: mail/base/modules/mailMigrator.js
@@ +162,5 @@
>          this._unAssert(fpbResource, collapsedResource);
>        }
>      }
>  
> +    if (currentUIVersion < 2) {

Per our IRC discussion, let's not set the pref if we error out at some point. I'd also like you to do the same for _unAssert in the v1 bit and move it to v2.
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-01-19 14:10:50 PST
Created attachment 589986 [details] [diff] [review]
Patch v3

Thanks for the review, Sid!

Ok, I've moved the migration for V1 into V2 as suggested, and updated the tests.  I've changed enough, that I think it warrants a second go-over.  I hope that's alright.

-Mike
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-01-23 07:21:07 PST
Blake:

Sid has suggested that we also remove the throbber from the customize dialog, as a way of reducing our redundancy, and the number of knobs and dials that TB has.  Thoughts?

-Mike
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-01-23 10:09:40 PST
(As per IRC, I think we should leave it in the customization dialog, at least until we get stats on whether the number of people who use it is less than the amount of problems it causes us.)
Comment 13 Jim Porter (:squib) 2012-01-23 10:19:10 PST
Even Firefox leaves their throbber in the customize dialog, so I think we may as well follow suit.
Comment 14 Siddharth Agarwal [:sid0] (inactive) 2012-01-23 14:47:48 PST
Comment on attachment 589986 [details] [diff] [review]
Patch v3

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

::: mail/base/modules/mailMigrator.js
@@ +149,5 @@
>      this._dataSource = this._rdf.GetDataSource("rdf:local-store");
>      let dirty = false;
>  
> +    try {
> +      if (currentUIVersion < 2) {

I'd like to see a comment here explaining why we've effectively skipped v1.

@@ +185,4 @@
>        }
> +
> +      if (dirty)
> +        this._dataSource.QueryInterface(Ci.nsIRDFRemoteDataSource).Flush();

This should be in the finally block I believe.

@@ +199,1 @@
>      }

Well, technically you don't need the finally block since you're catching all errors, but I guess this is fine (as long as you move the flush call inside).
Comment 15 Mike Conley (:mconley) - (Needinfo me!) 2012-01-24 07:22:13 PST
Created attachment 591083 [details] [diff] [review]
Patch v4 (r+'d by sid0)

Thanks for the review, Sid! I've made the corrections you asked for, and I'm doing a try build for committing.

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=cb52e9146548
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-01-25 07:20:53 PST
Created attachment 591458 [details] [diff] [review]
Patch v5 (r+'d by sid0)

I'm glad I did that try build - the tests were failing on OSX because I was using the wrong ID in test-migrate-to-rdf-ui-2.js.

That's been fixed in this patch, and the tests pass locally on OSX.
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2012-01-25 07:39:58 PST
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/bc502832e7cd
Comment 18 Mike Conley (:mconley) - (Needinfo me!) 2012-01-25 07:40:59 PST
Comment on attachment 591458 [details] [diff] [review]
Patch v5 (r+'d by sid0)

I believe bwinton wanted this to co-exist with tabs-on-top.
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-01-31 10:31:51 PST
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/015e0052d398

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