users will be able to add and place this as they see fit.

This is placeholder CSS until we get new images, but I want to get this in ASAP.

Alex/Stephen: basically, we're moving the statusbar UI to an optional toolbar button. I've used the existing sync/sync-throbber icons, but those don't fit especially well, so I assume you'll want to do something better for this. Separate bug, I assume?

(In reply to comment #3) > Alex/Stephen: basically, we're moving the statusbar UI to an optional toolbar > button. I've used the existing sync/sync-throbber icons, but those don't fit > especially well, so I assume you'll want to do something better for this. > Separate bug, I assume? Yeah we will want to match the other glyphs. So another bug would be just fine. Thanks!

Will we just use a tooltip for showing the time since last sync? Another option is a split menu button, and in the menu have the same sub-menu that we have under tools.

Follow up bug 594663 to cover the secondary UI command in the Firefox menu.

Yeah, I think the tooltip is sufficient, tbh.

Comment on attachment 473391 [details] [diff] [review] v1 >diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js >- if (!this._isLoggedIn()) { >- //XXXzpao When we move the string bundle, we can add more and make this >- // say "needs setup" or something similar. (bug 583381) >- button.removeAttribute("tooltiptext"); >- } >+ document.getElementById("sync-button").removeAttribute("status"); Do you also need to remove tooltiptext here? >diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul >+ <toolbarbutton id="sync-button" Need to add this to browser/themes/browserShared.inc. >diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd >+<!ENTITY syncToolbarButton.label "Sync"> >\ No newline at end of file fix plz >diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css >+toolbar:not([iconsize="small"]) .toolbarbutton-1#sync-button > .toolbarbutton-icon { Why ".toolbarbutton-1" here? Seems redundant. I don't even understand why these rules are needed, though. Shouldn't the styling match other buttons? Dao would be a better reviewer for this part. >diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css There's a #sync-status-button reference in here that needs removing.

(In reply to comment #8) > >- if (!this._isLoggedIn()) { > >- //XXXzpao When we move the string bundle, we can add more and make this > >- // say "needs setup" or something similar. (bug 583381) > >- button.removeAttribute("tooltiptext"); > >- } > >+ document.getElementById("sync-button").removeAttribute("status"); > > Do you also need to remove tooltiptext here? The only tooltip is the last sync time, so I don't think we need to remove that. > >diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css > > >+toolbar:not([iconsize="small"]) .toolbarbutton-1#sync-button > .toolbarbutton-icon { > > Why ".toolbarbutton-1" here? Seems redundant. I don't even understand why these > rules are needed, though. Shouldn't the styling match other buttons? Dao would > be a better reviewer for this part. That's bug 594657 > >diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css > > There's a #sync-status-button reference in here that needs removing. will do, though this CSS is all placeholder until bug 594657 is fixed.

Comments addressed, keep in mind that the theme stuff is all placeholdery until shorlander has new icons.

Comment on attachment 474095 [details] [diff] [review] v2 >diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js > if (gBrowser) { >+ document.getElementById("sync-button").removeAttribute("status"); >+ } >+ if (needsSetup) { >+ document.getElementById("sync-button").removeAttribute("tooltiptext"); > } Shouldn't this be under the if (gBrowser) check? (Is it actually possible to return to the "needsSetup" state after a sync has been completed?)

the previous attachment brought to you by e, the text editor, and a silly disrespect for existing file encodings!

>#zoom-out-button { > list-style-image: url("moz-icon://stock/gtk-zoom-out?size=toolbar"); >-} The new patch removes the closing bracket on the gnomestripe #zoom-out-button.

http://hg.mozilla.org/mozilla-central/rev/363c8bae0fb2 merge-fail sucks. shorlander is adding fix in the patch I'll push for bug 594657

Comment on attachment 475521 [details] [diff] [review] v2 unbitrotted >diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js >+ document.getElementById("sync-button").removeAttribute("status"); >+ if (needsSetup) { >+ document.getElementById("sync-button").removeAttribute("tooltiptext"); Oops, missed this - these need null-checks, given that the button is optional an all.

Gah, and the entry in browserShared.inc is wrong, and comment 11 wasn't addressed :(

(In reply to comment #16) > Oops, missed this - these need null-checks, given that the button is optional > an all. Filed followup bug 596799, feel free to review ;). For some reason I thought dolske had reviewed this bug so I asked him...

(In reply to comment #18) > (In reply to comment #16) > > Oops, missed this - these need null-checks, given that the button is optional > > an all. > > Filed followup bug 596799, feel free to review ;). For some reason I thought > dolske had reviewed this bug so I asked him... Actually, I just realised you meant something else. Moving over to bug 596799...

Verified fix on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre