We should display a small loading message before the page loads and some fallback content if it never loads.

Do we not already have these strings from previous versions of Firefox?

This is what we already have for the detail view and I think it is safe to just reuse it, though it could do with a little prettification.

Rough reproduction of the main text as we talked about. Feels to me a little as if we want an additional line beneath saying something about coming back later or when they have an internet connection to see more information.

(In reply to comment #3) > Created attachment 480151 [details] > FAIL > > Rough reproduction of the main text as we talked about. Feels to me a little as > if we want an additional line beneath saying something about coming back later > or when they have an internet connection to see more information. Maybe all that's needed is one sentence on the end of that, such as "When you're connected to the internet, this pane will feature some of the best and most popular add-ons for you to try out!"

Added the extra text, can I get UI sign-off on the strings so we can get them landed, the specific styling can come shortly after.

Comment on attachment 480222 [details] FAIL v2 ui-r+ for string. Some final styling is needed on the right pane

WIP for reference

Just the strings for pre-landing

(In reply to comment #6) > Comment on attachment 480222 [details] > FAIL v2 > > ui-r+ for string. Some final styling is needed on the right pane Having a separate paragraph for the offline message would be nice. Right now it's hard to separate from the existing text. Not sure if users will read everything. They will probably miss the last sentence.

Strings landed: http://hg.mozilla.org/mozilla-central/rev/b1557040c156

The strings contain hardcoded word "Firefox" instead of "&brandShortName;". I guess you would want to fix this and notify the localizers.

Nice catch, this fixes that.

Comment on attachment 480505 [details] [diff] [review] strings patch 2 rev 1 [checked in] Landed: http://hg.mozilla.org/mozilla-central/rev/1bbc1bc4d118

Something got landed with this bug as reference: http://hg.mozilla.org/mozilla-central/rev/3019bcf3e9ab But I don't see any patch in the attachment list that corresponds, and the change are for Pinstripe/Mac only???

(In reply to comment #16) > Something got landed with this bug as reference: > http://hg.mozilla.org/mozilla-central/rev/3019bcf3e9ab And got backed out again immediately after http://hg.mozilla.org/mozilla-central/rev/398deaf9cbdf

This implements the loading a failure cases, for now it treats moving to a new domain as a failure, unless we figure out what is causing bug 602256 we actually can't reliably block the load of the new domain so opening them into a new window would leave the page shown in both the new window and get add-ons which doesn't seem like the right choice. The theming is taken directly from the existing discovery pane and is the same on all platforms.

Comment on attachment 481279 [details] [diff] [review] patch rev 1 >+ home: null, Had to look at the code to see what this was - rename to something like homepageURL? >+ gDiscoverView.home = Services.io.newURI(url, null, null); >+ } >+ catch (e) { >+ gDiscoverView.showError(); >+ return; >+ } >+ >+ if (gDiscoverView.loaded) >+ gDiscoverView.loadBrowser(notifyInitialized); Nit: Would prefer if |var self = this;| was used here - that's the way used through most of the rest of the code for this sort of thing. >+ loadBrowser: function(aCallback) { >+ this.node.selectedPanel = this._loading; >+ >+ if (aCallback) >+ this.loadListeners.push(aCallback); >+ >+ gDiscoverView._browser.goHome(); >+ }, In my testing, if the first load doesn't complete (eg, by switching to offline mode, or hitting the stop button in the navigation bar), subsequent loads won't complete either - it just constantly shows the "Loading..." message. >+ onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) { >+ // Only care about the network stop status events >+ if (!(aStateFlags & (Ci.nsIWebProgressListener.STATE_IS_NETWORK)) || >+ !(aStateFlags & (Ci.nsIWebProgressListener.STATE_STOP))) >+ return; >+ >+ // Sometimes we stop getting onLocationChange events so we must redo the >+ // url tests here (bug 602256) >+ var location = this._browser.currentURI; >+ >+ // If the new page is not on the correct host or is not https when the >+ // default page is https or there was an error loading the page then show >+ // the error message >+ if (location.host != this.home.host || Since the initial page is about:blank, its possible for this to be called for that URL - which has no host, so accessing location.host will throw. Its also possible that this.home won't be initialized yet, as the webprogress listener is registered before the async call (that sets this.home) completes. >+ (aRequest && aRequest instanceof Ci.nsIHttpChannel && !aRequest.requestSucceeded)) { If AMO returns a 404 or 403 error, it doesn't seem correct to display a message that suggests the problem lies with the user's internet connection. It should either say there was some error with the webservice, or just display whatever the webservive returned. >+ onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, >+ aMaxSelfProgress, aCurTotalProgress, >+ aMaxTotalProgress) { }, >+ onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) { }, No need to specify what the arguments are, if the functions are never used. >+ <deck id="discover-view" flex="1" class="view-pane"> >+ <vbox id="discover-loading" align="center" pack="center" flex="1"> >+ <hbox class="loading discover-box" align="center"> >+ <image/> >+ <label value="&loading.label;"/> >+ </hbox> >+ </vbox> >+ <hbox id="discover-error" flex="1" align="center"> >+ <spacer flex="1"/> >+ <hbox flex="1" class="discover-box" align="center"> >+ <image class="discover-logo"/> >+ <vbox flex="1" align="stretch"> >+ <label class="discover-title">&discover.title;</label> >+ <description class="discover-description">&discover.description2;</description> >+ <description class="discover-footer">&discover.footer;</description> >+ </vbox> >+ </hbox> >+ <spacer flex="1"/> >+ </hbox> The loading message box should be styled the same as other loading message boxes. The other box here could be either, though I'm leaning towards having that constistent too. I'm happy to have all such boxes using the style you've added here (for bug 601022, I've made them quite similar anyway) - just as long as they're consistent. For bug 601022 I've been using class="view-alert". Also, other similar boxes have a <spacer flex="1"/> at the top, and <spacer flex="3"/> at the bottom - so it appears more to the top of the view, closer to where you'd typically start reading from.

Comment on attachment 481279 [details] [diff] [review] patch rev 1 >-#detail-view .loading > image { >+.loading > image { > list-style-image: url("chrome://global/skin/icons/loading_16.png"); > } This should be moved out of the details section, since its no longer specific to that.

(In reply to comment #19) > Comment on attachment 481279 [details] [diff] [review] > patch rev 1 > > >+ home: null, > > Had to look at the code to see what this was - rename to something like > homepageURL? Done > >+ gDiscoverView.home = Services.io.newURI(url, null, null); > >+ } > >+ catch (e) { > >+ gDiscoverView.showError(); > >+ return; > >+ } > >+ > >+ if (gDiscoverView.loaded) > >+ gDiscoverView.loadBrowser(notifyInitialized); > > Nit: Would prefer if |var self = this;| was used here - that's the way used > through most of the rest of the code for this sort of thing. Done > >+ loadBrowser: function(aCallback) { > >+ this.node.selectedPanel = this._loading; > >+ > >+ if (aCallback) > >+ this.loadListeners.push(aCallback); > >+ > >+ gDiscoverView._browser.goHome(); > >+ }, > > In my testing, if the first load doesn't complete (eg, by switching to offline > mode, or hitting the stop button in the navigation bar), subsequent loads won't > complete either - it just constantly shows the "Loading..." message. Fixed and tested > >+ onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) { > >+ // Only care about the network stop status events > >+ if (!(aStateFlags & (Ci.nsIWebProgressListener.STATE_IS_NETWORK)) || > >+ !(aStateFlags & (Ci.nsIWebProgressListener.STATE_STOP))) > >+ return; > >+ > >+ // Sometimes we stop getting onLocationChange events so we must redo the > >+ // url tests here (bug 602256) > >+ var location = this._browser.currentURI; > >+ > >+ // If the new page is not on the correct host or is not https when the > >+ // default page is https or there was an error loading the page then show > >+ // the error message > >+ if (location.host != this.home.host || > > Since the initial page is about:blank, its possible for this to be called for > that URL - which has no host, so accessing location.host will throw. Its also > possible that this.home won't be initialized yet, as the webprogress listener > is registered before the async call (that sets this.home) completes. Taken care of the first by explicitly ignoring the about:blank cases. For the second I've made it so the progress listener isn't registered until homepageURL is set. > >+ (aRequest && aRequest instanceof Ci.nsIHttpChannel && !aRequest.requestSucceeded)) { > > If AMO returns a 404 or 403 error, it doesn't seem correct to display a message > that suggests the problem lies with the user's internet connection. It should > either say there was some error with the webservice, or just display whatever > the webservive returned. I don't think this warrants adding new strings but I'm also positive that we don't want to display any AMO failure page here. We did agree in the meeting that we were just going to use the same message for all cases but I guess that was before we added the more specific tagline. I guess we could just hide that bit in these cases? A fair bit more effort for what should be a very rare case though. > >+ onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, > >+ aMaxSelfProgress, aCurTotalProgress, > >+ aMaxTotalProgress) { }, > >+ onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) { }, > > No need to specify what the arguments are, if the functions are never used. Done > >+ <deck id="discover-view" flex="1" class="view-pane"> > >+ <vbox id="discover-loading" align="center" pack="center" flex="1"> > >+ <hbox class="loading discover-box" align="center"> > >+ <image/> > >+ <label value="&loading.label;"/> > >+ </hbox> > >+ </vbox> > >+ <hbox id="discover-error" flex="1" align="center"> > >+ <spacer flex="1"/> > >+ <hbox flex="1" class="discover-box" align="center"> > >+ <image class="discover-logo"/> > >+ <vbox flex="1" align="stretch"> > >+ <label class="discover-title">&discover.title;</label> > >+ <description class="discover-description">&discover.description2;</description> > >+ <description class="discover-footer">&discover.footer;</description> > >+ </vbox> > >+ </hbox> > >+ <spacer flex="1"/> > >+ </hbox> > > The loading message box should be styled the same as other loading message > boxes. The other box here could be either, though I'm leaning towards having > that constistent too. I'm happy to have all such boxes using the style you've > added here (for bug 601022, I've made them quite similar anyway) - just as long > as they're consistent. For bug 601022 I've been using class="view-alert". So how about I just set class="view-alert" on them and let bug 601022 take care of the styling? > Also, other similar boxes have a <spacer flex="1"/> at the top, and <spacer > flex="3"/> at the bottom - so it appears more to the top of the view, closer to > where you'd typically start reading from. Fixed

Please remove the spacers, as that should be done using css styling and instead of putting align="center" on a box use -moz-box-align:center, so themers can overrride the styling. Styling doesn't belong in content.

(In reply to comment #21) > So how about I just set class="view-alert" on them and let bug 601022 take care > of the styling? Yep, that works.

(In reply to comment #22) > Please remove the spacers, as that should be done using css styling and instead > of putting align="center" on a box use -moz-box-align:center, so themers can > overrride the styling. Styling doesn't belong in content. For these cases, we don't use the same flex value for the 2 spacers - its purposefully not centered (its more to the top, nearer to where you'd normally start reading). So the effect can't be done without using spacers. However, the flex attribute on the spacers could be moved into the theme (via -moz-box-flex). I'll look into doing that in bug 601022.

Waiting on bug 601022 as we want to use the view-alert class there

Un-butrotted and fixes from the review and other landings.

Comment on attachment 493077 [details] [diff] [review] patch rev 2 > var gDiscoverView = { > node: null, > enabled: true, > // Set to true after the view is first shown. If initialization completes > // after this then it must also load the discover homepage > loaded: false, > _browser: null, >+ _loading: null, >+ homepageURL: null, >+ loadListeners: [], loadListeners seems like it would be a private/internal property, so prefix it with "_" (and maybe homepageURL? not sure). >+ this._loading = document.getElementById("discover-loading"); >+ this._error = document.getElementById("discover-error"); _error isn't listed as a property of gDiscoverView like _loaded is. >+ self._browser.addProgressListener(self, Ci.nsIWebProgress.NOTIFY_ALL | Ci.nsIWebProgress.NOTIFY_STATE_ALL); Nit: wrap this long line. >+ loadBrowser: function(aCallback) { >+ this.node.selectedPanel = this._loading; >+ >+ if (aCallback) >+ this.loadListeners.push(aCallback); >+ >+ if (this._browser.currentURI.equals(this.homepageURL)) >+ this._browser.reload(); >+ else >+ this._browser.goHome(); >+ }, Looks like this will mess up if its called before homepageURL is set (and generally shouldn't be used outside of this view's code) - prefix with "_" as a guard/warning? >+ // If the hostname is the same and either the home scheme is not https or >+ // the new location is https then continue with the load Huh? Reword, or add punctuation, or something. >+ if (aLocation.host == this.homepageURL.host && >+ (!this.homepageURL.schemeIs("https") || aLocation.schemeIs("https"))) >+ return; I assume we're just blindly relying on AMO to always target external links to _blank? (I'm ok with that) >+ >+ this.showError(); >+ aRequest.cancel(Components.results.NS_BINDING_ABORTED); Will this leave the browser in a state where the discovery page is still usable? I would have expected to need to cancel the request before onLocationChange (using something like nsIURIContentListener.onStartURIOpen). Also, will aborting the request trigger onStateChange? If so, might not need to be calling showError() here. >+ onSecurityChange: function(aWebProgress, aRequest, aState) { ... >+ this.showError(); >+ aRequest.cancel(Components.results.NS_BINDING_ABORTED); Ditto. >diff --git a/toolkit/mozapps/extensions/test/browser/browser_discovery.js b/toolkit/mozapps/extensions/test/browser/browser_discovery.js ... >+ onLocationChange: function(aWebProgress, aRequest, aLocation) { }, >+ onSecurityChange: function(aWebProgress, aRequest, aState) { }, >+ onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, >+ aMaxSelfProgress, aCurTotalProgress, >+ aMaxTotalProgress) { }, >+ onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) { }, This would be a lot more readable if the parameters were omitted (since they're not being used anyway).

Updated from comments

Comment on attachment 496192 [details] [diff] [review] patch rev 3 >+ function setURL(aURL) { >+ try { >+ self.homepageURL = Services.io.newURI(aURL, null, null); >+ } >+ catch (e) { Style consistency nit: No newline between } and catch.

Landed: http://hg.mozilla.org/mozilla-central/rev/0c45b1fc2f7c

Backed out

For some reason sendMouseEvent works more reliably than synthesizeMouse here. Also includes a fix for an assertion that gets logged whenever the add-ons manager is closed.

Comment on attachment 498948 [details] [diff] [review] bustage fix Mind giving this a quick stamp Rob?

Comment on attachment 498948 [details] [diff] [review] bustage fix not at all

Landed: http://hg.mozilla.org/mozilla-central/rev/33ef0dc20824

Why is a non standard font used for Winstripe: MetaWebPro-Book ? This ia not very common font on Windows platform, and it is a licensed font

(In reply to comment #36) > Why is a non standard font used for Winstripe: MetaWebPro-Book ? > This ia not very common font on Windows platform, and it is a licensed font To attempt to match the get add-ons page as much as possible, if not present it should just fall back to something else.

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110119 Firefox/4.0b10pre ID:20110119030331 in off-line mode.