Closed
Bug 835151
Opened 12 years ago
Closed 12 years ago
Implement update checking inside about flyout in metro panel
Categories
(Firefox for Metro Graveyard :: Flyouts, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: bbondy, Assigned: bbondy)
References
Details
(Whiteboard: feature=work)
Attachments
(1 file, 2 obsolete files)
|
(deleted),
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
Implement update checking inside about flyout in metro panel. The mockup showed a line for if there were updates available or not. This bug is to implement update checking in the metro Firefox about panel if we want to do that.
I think the current plan for MVP is at most to have updates but only silently with no UI.
|
||
Updated•12 years ago
|
Hardware: x86_64 → x86
Whiteboard: need to hook this up to an epic on updating - Asa
|
||
Updated•12 years ago
|
Whiteboard: need to hook this up to an epic on updating - Asa → [metro-mvp?]
|
||
Updated•12 years ago
|
Blocks: metrov1triage
|
|
||
Updated•12 years ago
|
Whiteboard: [metro-mvp?]
|
|
||
Updated•12 years ago
|
Summary: Implement update checking inside about flyout in metro panel → work - Implement update checking inside about flyout in metro panel
Whiteboard: feature=work
|
|
||
Updated•12 years ago
|
Summary: work - Implement update checking inside about flyout in metro panel → Implement update checking inside about flyout in metro panel
|
Assignee | |
Comment 3•12 years ago
|
||
Other updater reviews are higher priority, but I'd also like to land this in the current iteration as well.
The bulk of the code in the patch is copied from the about dialog updater.
This patch will likely get replaced with a newer one after I do testing on oak, but just wanted to see if you had any feedback (after the others are reviewed) if you happen to get to it before I update.
Assignee: nobody → netzen
Attachment #741001 -
Flags: feedback?(robert.bugzilla)
|
|
Assignee | |
Updated•12 years ago
|
Attachment #741001 -
Attachment description: Patch v0.9 (Untested) → Patch v1 (Untested)
Attachment #741001 -
Flags: feedback?(robert.bugzilla) → review?(robert.bugzilla)
|
|
||
Comment 4•12 years ago
|
||
Comment on attachment 741001 [details] [diff] [review]
Patch v1 (Untested)
Last review comment already discussed on irc with bbondy.
naming is a tad cumbersome (e.g. isResponsibleForUpdates, isResponsibleForUpdatesOfInstallation, etc.). Perhaps hasMutex would be better.
Attachment #741001 -
Flags: review?(robert.bugzilla) → review-
|
|
||
Comment 5•12 years ago
|
||
Comment on attachment 741001 [details] [diff] [review]
Patch v1 (Untested)
oops... wrong bug
Attachment #741001 -
Flags: review- → review?(robert.bugzilla)
|
|
||
Comment 6•12 years ago
|
||
Comment on attachment 741001 [details] [diff] [review]
Patch v1 (Untested)
>diff --git a/browser/metro/base/content/aboutPanel.js b/browser/metro/base/content/aboutPanel.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/metro/base/content/aboutPanel.js
Please add license header
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>@@ -0,0 +1,567 @@
>+var gAppUpdater;
>+var AboutPanelUI = {
>+ get _aboutVersionLabel() {
>+ return document.getElementById('about-version-label');
>+ },
>+
>+ init: function() {
>+ // Include the build ID if this is an "a#" (nightly or aurora) build
>+ let version = Services.appinfo.version;
>+ if (/a\d+$/.test(version)) {
>+ let buildID = Services.appinfo.appBuildID;
>+ let buildDate = buildID.slice(0,4) + "-" + buildID.slice(4,6) +
>+ "-" + buildID.slice(6,8);
>+ this._aboutVersionLabel.textContent +=" (" + buildDate + ")";
>+
>+ window.addEventListener('MozFlyoutPanelShowing', this, false);
>+#if MOZ_UPDATE_CHANNEL != release
>+ let defaults = Services.prefs.getDefaultBranch("");
>+ let channelLabel = document.getElementById("currentChannel");
>+ channelLabel.value = defaults.getCharPref("app.update.channel");
>+#endif
This doesn't look right... you have a test for a nightly or aurora and then #if MOZ_UPDATE_CHANNEL != release. Also, the event listener for MozFlyoutPanelShowing is only added for nightly and aurora.
>+ }
>+ },
>+
Looks like this patch doesn't include similar changes to aboutDialog.js that are needed for isResponsibleForUpdates. While there please remove the extra whitespace in aboutDialog.js so it is easier to see differences between aboutDialog.js and aboutPanel.js.
|
|
||
Comment 7•12 years ago
|
||
Comment on attachment 741001 [details] [diff] [review]
Patch v1 (Untested)
>diff --git a/browser/metro/base/content/bindings/flyoutpanel.xml b/browser/metro/base/content/bindings/flyoutpanel.xml
>--- a/browser/metro/base/content/bindings/flyoutpanel.xml
>+++ b/browser/metro/base/content/bindings/flyoutpanel.xml
>@@ -71,16 +71,17 @@
> </method>
>
> <method name="show">
> <body>
> <![CDATA[
> if (this.isVisible)
> return;
>
>+ this._fire("MozFlyoutPanelShowing");
> this.setAttribute("visible", "true");
> this.classList.add("flyoutpanel-slide-in");
> DialogUI.pushPopup(this, this);
> ]]>
> </body>
> </method>
>
> <method name="handleEvent">
>@@ -93,16 +94,27 @@
> case 'MozContextUIDismiss':
> this.hide();
> break;
> }
> ]]>
> </body>
> </method>
>
>+ <method name="_fire">
>+ <parameter name="aName"/>
>+ <body>
>+ <![CDATA[
>+ let event = document.createEvent("Events");
>+ event.initEvent(aName, true, false);
>+ this.dispatchEvent(event);
>+ ]]>
>+ </body>
>+ </method>
If you are going to add more places where this is used I am fine with it otherwise you might as well just do this in show.
|
|
||
Comment 8•12 years ago
|
||
Comment on attachment 741001 [details] [diff] [review]
Patch v1 (Untested)
>diff --git a/browser/metro/base/content/browser-ui.js b/browser/metro/base/content/browser-ui.js
>--- a/browser/metro/base/content/browser-ui.js
>+++ b/browser/metro/base/content/browser-ui.js
>@@ -59,17 +59,18 @@ let Elements = {};
>
> /**
> * Cache of commonly used string bundles.
> */
>
> var Strings = {};
> [
> ["browser", "chrome://browser/locale/browser.properties"],
>- ["brand", "chrome://branding/locale/brand.properties"]
>+ ["brand", "chrome://branding/locale/brand.properties"],
>+ ["aboutPanel", "chrome://browser/locale/aboutPanel.properties"]
So, these strings are in browser.properties in non metro. Any reason not to just add them there? If so it might be a good thing to do the same in non metro?
>diff --git a/browser/metro/base/content/browser.xul b/browser/metro/base/content/browser.xul
>--- a/browser/metro/base/content/browser.xul
>+++ b/browser/metro/base/content/browser.xul
>@@ -13,16 +13,18 @@
> <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
> %globalDTD;
> <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd">
> %browserDTD;
> <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
> %brandDTD;
> <!ENTITY % prefsDTD SYSTEM "chrome://browser/locale/preferences.dtd">
> %prefsDTD;
>+<!ENTITY % aboutPanelDTD SYSTEM "chrome://browser/locale/aboutPanel.dtd">
>+%aboutPanelDTD;
> #ifdef MOZ_SERVICES_SYNC
> <!ENTITY % syncDTD SYSTEM "chrome://browser/locale/sync.dtd">
> %syncDTD;
> #endif
> ]>
>
> <window id="main-window"
> onload="Browser.startup();"
>@@ -38,16 +40,17 @@
> xmlns:html="http://www.w3.org/1999/xhtml">
>
> <script type="application/javascript" src="chrome://browser/content/browser.js"/>
> <script type="application/javascript" src="chrome://browser/content/browser-scripts.js"/>
> <script type="application/javascript" src="chrome://browser/content/browser-ui.js"/>
> <script type="application/javascript" src="chrome://browser/content/Util.js"/>
> <script type="application/javascript" src="chrome://browser/content/input.js"/>
> <script type="application/javascript;version=1.8" src="chrome://browser/content/appbar.js"/>
>+ <script type="application/javascript;version=1.8" src="chrome://browser/content/aboutPanel.js"/>
So I'm also in the know, why do you have to specify the javascript version here?
>diff --git a/browser/metro/locales/en-US/chrome/aboutPanel.dtd b/browser/metro/locales/en-US/chrome/aboutPanel.dtd
>new file mode 100644
>--- /dev/null
>+++ b/browser/metro/locales/en-US/chrome/aboutPanel.dtd
>@@ -0,0 +1,52 @@
>+<!-- This Source Code Form is subject to the terms of the Mozilla Public
>+ - License, v. 2.0. If a copy of the MPL was not distributed with this
>+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>+
>+
>+<!ENTITY aboutHeader.title "About">
>+<!ENTITY aboutHeader.product.label "&brandShortName;">
>+<!ENTITY aboutHeader.company.label "By &vendorShortName;">
>+<!ENTITY aboutHeader.policy.label "Read the &brandShortName; privacy policy online">
>+<!-- LOCALIZATION NOTE (update.checkingForUpdates): try to make the localized text short (see bug 596813 for screenshots). -->
>+<!ENTITY update.checkingForUpdates "Checking for updatesâ¦">
>+<!-- LOCALIZATION NOTE (update.checkingAddonCompat): try to make the localized text short (see bug 596813 for screenshots). -->
>+<!ENTITY update.checkingAddonCompat "Checking Add-on compatibilityâ¦">
>+<!-- LOCALIZATION NOTE (update.noUpdatesFound): try to make the localized text short (see bug 596813 for screenshots). -->
>+<!ENTITY update.noUpdatesFound "&brandShortName; is up to date">
>+<!-- LOCALIZATION NOTE (update.adminDisabled): try to make the localized text short (see bug 596813 for screenshots). -->
>+<!ENTITY update.adminDisabled "Updates disabled by your system administrator">
>+<!-- LOCALIZATION NOTE (update.notResponsible): try to make the localized text short (see bug 596813 for screenshots). -->
Please remove the (see bug 596813 for screenshots) for the last item since there are no screenshots for that text in that bug.
>+<!ENTITY update.notResponsible "&brandShortName; is being updated by another instance">
This string will need to be reviewed by UX
|
|
||
Updated•12 years ago
|
Attachment #741001 -
Flags: review?(robert.bugzilla) → review-
|
|
Assignee | |
Comment 9•12 years ago
|
||
(In reply to Robert Strong [:rstrong] (do not email) from comment #6)
> Comment on attachment 741001 [details] [diff] [review]
> Patch v1 (Untested)
> Looks like this patch doesn't include similar changes to aboutDialog.js that
> are needed for isResponsibleForUpdates. While there please remove the extra
> whitespace in aboutDialog.js so it is easier to see differences between
> aboutDialog.js and aboutPanel.js.
That's added for Desktop into the bug that introduced that concept for Desktop. So bug 794937.
|
|
Assignee | |
Comment 10•12 years ago
|
||
Attachment #741001 -
Attachment is obsolete: true
Attachment #744971 -
Flags: review?(robert.bugzilla)
|
|
||
Updated•12 years ago
|
Attachment #744971 -
Flags: review?(robert.bugzilla) → review+
|
|
Assignee | |
Comment 11•12 years ago
|
||
- Rebased
- Added commit message
Attachment #744971 -
Attachment is obsolete: true
Attachment #749994 -
Flags: review+
|
|
Assignee | |
Comment 12•12 years ago
|
||
Testing notes in bug 794937
|
|
Assignee | |
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
|
||
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•