Closed Bug 835151 Opened 11 years ago Closed 11 years ago

Implement update checking inside about flyout in metro panel

Categories

(Firefox for Metro Graveyard :: Flyouts, defect)

Product:
Firefox for Metro Graveyard
Component:
Flyouts
Platform:
x86
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 24

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: feature=work)

Attachments

(1 file, 2 obsolete files)

Patch v2' - About panel
38.30 KB, 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.
Blocks: 831955
No longer blocks: 831955
Hardware: x86_64 → x86
Whiteboard: need to hook this up to an epic on updating - Asa
Whiteboard: need to hook this up to an epic on updating - Asa → [metro-mvp?]
Whiteboard: [metro-mvp?]
Blocks: 833182
No longer blocks: metrov1triage
Summary: Implement update checking inside about flyout in metro panel → work - Implement update checking inside about flyout in metro panel
Whiteboard: feature=work
Blocks: 849395
No longer blocks: 833182
Summary: work - Implement update checking inside about flyout in metro panel → Implement update checking inside about flyout in metro panel
general->flyouts, 3 bugs
Component: General → Flyouts
Attached patch Patch v1 (Untested) (obsolete) — — Splinter Review 
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)
Attachment #741001 - Attachment description: Patch v0.9 (Untested) → Patch v1 (Untested)
Attachment #741001 - Flags: feedback?(robert.bugzilla) → review?(robert.bugzilla)
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 on attachment 741001 [details] [diff] [review]
Patch v1 (Untested)

oops... wrong bug
Attachment #741001 - Flags: review- → review?(robert.bugzilla)
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 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 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
Attachment #741001 - Flags: review?(robert.bugzilla) → review-
(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.
Attached patch Patch v2 - About panel (obsolete) — — Splinter Review 

        Attachment #741001 -
        Attachment is obsolete: true

        Attachment #744971 -
        Flags: review?(robert.bugzilla)

        Attachment #744971 -
        Flags: review?(robert.bugzilla) → review+

    
    

    
  


  
- Rebased
- Added commit message

        Attachment #744971 -
        Attachment is obsolete: true

        Attachment #749994 -
        Flags: review+

    
    

    
  
Testing notes in bug 794937

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a7cad4e202e1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
OS: Windows 8 Metro → Windows 8.1

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: