Last Comment Bug 715565 - two throbbers in main window
: two throbbers in main window
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)
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:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1 (1.64 KB, patch)
2012-01-11 09:25 PST, Mike Conley (:mconley)
bwinton: review+
bwinton: ui‑review-
Details | Diff | Splinter Review
Patch v2 (5.98 KB, patch)
2012-01-16 13:05 PST, Mike Conley (:mconley)
sid.bugzilla: review+
Details | Diff | Splinter Review
Patch v3 (12.89 KB, patch)
2012-01-19 14:10 PST, Mike Conley (:mconley)
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)
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)
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) 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) 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) 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) 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) 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.

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) 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.

Comment 11 Mike Conley (:mconley) 2012-01-23 07:21:07 PST

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?

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) 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.
Comment 16 Mike Conley (:mconley) 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) 2012-01-25 07:39:58 PST
Committed to comm-central as
Comment 18 Mike Conley (:mconley) 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) 2012-01-31 10:31:51 PST
Committed to comm-aurora as

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