Closed Bug 339102 Opened 18 years ago Closed 17 years ago

SoC Tracking - Enhanced Page Info window

Categories

(Firefox :: Page Info Window, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 alpha4

People

(Reporter: chofmann, Assigned: florian)

References

(Depends on 2 open bugs, )

Details

Attachments

(3 files, 14 obsolete files)

16.69 KB, image/png
beltzner
: ui-review+
Details
152.76 KB, patch
Details | Diff | Splinter Review
2.55 KB, patch
Details | Diff | Splinter Review
Title/Summary 	Enhanced Page Info window
Student 	Florian QUEZE
Student Email 	f.queze@gmail.com
Component: General → Page Info
OS: Windows XP → All
Depends on: 214265
QA Contact: general → page.info
I was thinking of suggesting fixing Bug 265698 which has an unreviewed patch as part of this.  But if tabs aren't going to be used anymore then it's a moot point.  
The project is over, and despite my absenteeism as mentor, Florian has done a great job fixing up the UI for Page Info and making it more useful. Shortly he'll be posting some screenshots and patches to this bug where we can continue to polish this feature and get it in on trunk for wider testing. He'll also put down a full changelog.
Attached patch updated patch (obsolete) — Splinter Review
new feed tab
screenshot: http://mozilla.queze.net/soc/progress/2006-09-16/pageinfo-soc-2006-09-16-2.png
Attachment #235397 - Attachment is obsolete: true
Attachment #238850 - Flags: review?(mconnor)
Attachment #235397 - Flags: review?(mconnor)
Attached image new image for the icons (obsolete) —
Attached patch patch v3 (obsolete) — Splinter Review
Assignee: beltzner → f.qu
Attachment #238850 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #241083 - Flags: review?(mconnor)
Attachment #238850 - Flags: review?(mconnor)
Attachment #241083 - Flags: review?(mconnor) → review?(mano)
Comment on attachment 241083 [details] [diff] [review]
patch v3

First-cycle review:

Index: browser/components/pageinfo/content/pageinfo.css
===================================================================
+#viewGroup radio {
+  -moz-binding: url("chrome://browser/content/pageinfo/pageinfo.xml#viewbutton");
+  -moz-box-orient: vertical;
+  -moz-box-align: center;
+  -moz-appearance: none;
+}

For now, put this in the skin file(s), and get rid of this file.

Index: browser/components/pageinfo/content/pageinfo.xml
===================================================================
+<bindings id="pageinfoBindings"
+          xmlns="http://www.mozilla.org/xbl"
+          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+          xmlns:xbl="http://www.mozilla.org/xbl">
+
+  <!-- based on preferences.xml paneButton -->
+  <binding id="viewbutton" extends="chrome://global/content/bindings/radio.xml#radio">
+    <resources>
+      <stylesheet src="chrome://browser/content/pageinfo/pageinfo.css"/>
+    </resources>
+    <content>
+      <xul:image class="viewButtonIcon" xbl:inherits="src"/>
+      <xul:label class="viewButtonLabel" xbl:inherits="value=label"/>
+    </content>
+    <implementation implements="nsIAccessibleProvider">
+      <property name="accessible">
+        <getter>
+          <![CDATA[
+            var accService = Components.classes["@mozilla.org/accessibilityService;1"]
+                                       .getService(Components.interfaces.nsIAccessibilityService);
+            return accService.createXULListitemAccessible(this);
+          ]]>
+        </getter>
+      </property>
+    </implementation>
+  </binding>
+</bindings>

Use chrome://mozapps/content/extensions/extensions.xml#viewbutton instead, I will open a bug on moving that binding out of extensions.xml to a bit more generic place.


Index: browser/components/feeds/jar.mn
===================================================================
Index: browser/components/feeds/content/feeds.xml
===================================================================
Index: browser/components/feeds/content/pageInfoOverlay.css
===================================================================

I'm not sure I see the point of overlaying the pageinfo dialog from the feeds component, let me think about it.

Has belzner approved the behavior of the subscribe button (appears only after selecting the item)?

(I didn't review the overlay itself yet for those reasons).

Index: browser/components/feeds/content/pageInfoOverlay.js
===================================================================
+function initFeedTab()
+{
+  var feedTypes = {};
+  feedTypes["application/rss+xml"] = theBundle.getString("feedRss");
+  feedTypes["application/atom+xml"] = theBundle.getString("feedAtom");
+

nit:
  const feedTypes = {
   "application/rss+xml": theBundle.getString("feedRss"),
   "application/atom+xml": theBundle.getString("feedAtom")
  };

Index: browser/components/Makefile.in
===================================================================
Index: browser/components/pageinfo/Makefile.in
===================================================================
Index: browser/components/pageinfo/jar.mn
===================================================================

You don't need to build this folder separately, just add those to browser/base/jar.mn

i.e.
content/browser/pageinfo/foo.bar (content/pageinfo/foo.bar)

Index: browser/components/pageinfo/content/pageInfo.js
===================================================================

I wish all the global functions here were scoped in gPageInfoWindow so we could use js getters for things like the string bundle (and also, implement the image permission observer observer as part of it, and then add it as a weak observer).


+pageInfoTreeView.prototype = {

gPageInfoTreeView

+// mmm, yummy. global variables.
+var theWindow = null;
+var theDocument = null;

General comment (and that is only if you aren't introducing gPageInfoWindow): all global variables should be prefixed with "g" (I would also get rid of the "the" prefix).


+// localized strings (will be filled in when the document is loaded)
+// this isn't all of them, these are just the ones that would otherwise have been loaded inside a loop
+var gStrings = {}

nit: missing semicolon;


+const DRAGSERVICE_CONTRACTID    = "@mozilla.org/widget/dragservice;1";
+const TRANSFERABLE_CONTRACTID   = "@mozilla.org/widget/transferable;1";
+const ARRAY_CONTRACTID          = "@mozilla.org/supports-array;1";
+const STRING_CONTRACTID         = "@mozilla.org/supports-string;1";


+const nsICookiePermission  = Components.interfaces.nsICookiePermission;
+const nsIPermissionManager = Components.interfaces.nsIPermissionManager;
+var permissionManager = Components.classes["@mozilla.org/permissionmanager;1"].getService(nsIPermissionManager);
+var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);


+
+// a number of services I'll need later
+// the cache services
+const nsICacheService = Components.interfaces.nsICacheService;
+const cacheService = Components.classes["@mozilla.org/network/cache-service;1"].getService(nsICacheService);
+var httpCacheSession = cacheService.createSession("HTTP", 0, true);
+httpCacheSession.doomEntriesIfExpired = false;
+var ftpCacheSession = cacheService.createSession("FTP", 0, true);
+ftpCacheSession.doomEntriesIfExpired = false;
+var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch2);
+var aserv = Components.classes["@mozilla.org/atom-service;1"].getService(Components.interfaces.nsIAtomService);
+var dateService = Components.classes["@mozilla.org/intl/scriptabledateformat;1"].getService(nsIScriptableDateFormat);

This isn't safe. Initialize them onload (or better, get them lazily with a js getter if you do scope everything in gPageInfoWindow).

+const nsICertificateDialogs = Components.interfaces.nsICertificateDialogs;
+const nsCertificateDialogs = "@mozilla.org/nsCertificateDialogs;1"

s/nsCertificateDialogs/CERTIFICATEDIALOGS_CONTRACTID/


+// interfaces for the different html elements
+const nsIAnchorElement   = Components.interfaces.nsIDOMHTMLAnchorElement
+const nsIImageElement    = Components.interfaces.nsIDOMHTMLImageElement
+const nsIAreaElement     = Components.interfaces.nsIDOMHTMLAreaElement
+const nsILinkElement     = Components.interfaces.nsIDOMHTMLLinkElement
+const nsIInputElement    = Components.interfaces.nsIDOMHTMLInputElement
+const nsIFormElement     = Components.interfaces.nsIDOMHTMLFormElement
+const nsIAppletElement   = Components.interfaces.nsIDOMHTMLAppletElement
+const nsIObjectElement   = Components.interfaces.nsIDOMHTMLObjectElement
+const nsIEmbedElement    = Components.interfaces.nsIDOMHTMLEmbedElement
+const nsIButtonElement   = Components.interfaces.nsIDOMHTMLButtonElement
+const nsISelectElement   = Components.interfaces.nsIDOMHTMLSelectElement
+const nsITextareaElement = Components.interfaces.nsIDOMHTMLTextAreaElement
+const nsIQuoteElement    = Components.interfaces.nsIDOMHTMLQuoteElement
+const nsIModElement      = Components.interfaces.nsIDOMHTMLModElement
+

If these are just for instanceof checks, you can simply use |if (elem instanceof HTMLAnchorElement)| for example.

+function doHelpButton() {
+  var helpdoc;
+  var tabControl = document.getElementById("tabbox");
+  switch (tabControl.selectedTab.id) {
+    case "generalTab":
+      helpdoc = "pageinfo_general";
+      break;
+    case "formsTab":
+      helpdoc = "pageinfo_forms";
+      break;
+    case "linksTab":
+      helpdoc = "pageinfo_links";
+      break;
+    case "mediaTab":
+      helpdoc = "pageinfo_media";
+      break;
+    case "securityTab":
+      helpdoc = "pageinfo_security";
+      break;
+    case "p3pTab":
+      helpdoc = "pageinfo_privacy";
+      break;
+    default:
+      helpdoc = "pageinfo_general";
+      break;
+  }
+  openHelp(helpdoc, 'chrome://browser/locale/help/help.rdf');
+}
+

While you're rewriting this file, please make this method use a map, i.e.
const helpTopics = { "generalTab": "pageinfo_general", ... }; openHelp(helpTopics[tabControl.selectedTab.id], "chrome://browser/locale/help/help.rdf");

+function saveMedia()
+{
+  var tree = document.getElementById("imagetree");
+  var count = tree.view.selection.count;
+  if (count < 1)
+    return;
+
+  if (count == 1)
+  {
+    var item = getSelectedImage(tree);
+    var url = imageView.data[tree.currentIndex][0];
+
+    if (url)
+      // XXX Firefox specfic

Time to get rid of this comment.


Index: browser/components/pageinfo/content/pageInfo.xul
===================================================================
+  <deck id="mainDeck" flex="1">
+    <!-- General page information -->
+    <vbox id="generalPanel">
+      <textbox class="header" readonly="true" crop="end" id="titletext"/>
+      <grid>
+        <columns>
+          <column/>
+          <column style="width: .5em;"/>

Use a class and move this to the skin file.

+          <column flex="1"/>
+        </columns>
+        <rows>
+          <row>
+            <label control="urltext" value="&generalURL;"/>
+            <separator/>
+            <textbox readonly="true" crop="end" id="urltext"/>

You should almost never crop textboxes in this UI (esp. not for url fields!).


+    <!-- Media information -->
+    <vbox id="mediaPanel">
+      <tree id="imagetree" flex="1" onselect="onImageSelect();" ondraggesture="onBeginLinkDrag(event,'image-address','image-alt')" contextmenu="picontext">
+        <treecols>
+          <treecol sortSeparators="true" persist="hidden width" flex="10"
+                        width="10" id="image-address" label="&mediaAddress;"/>

General comment: one attribute per line style in > 80 characters lines in xul files.

Index: browser/themes/winstripe/browser/pageInfo.css
===================================================================

+#securityTab textbox {
+  margin-left: 4px;

-moz-margin-start (only for winstripe).



Index: security/manager/pki/resources/jar.mn
===================================================================
Index: security/manager/pki/resources/content/PageInfoOverlay.js
===================================================================
Index: security/manager/pki/resources/content/PageInfoOverlay.xul
===================================================================


Kaie should review these changes
Attachment #241083 - Flags: review?(mano) → review-
mano: Almost all of those things you flagged in pageInfo.js and pageInfo.xul, while certainly valid review comments, come from pre-existing code that I wrote ages ago. Perhaps it would be better to do a cvs move for those files instead? That would ease the burden of review that Florian has to bear.
Or just patch the the current files first, I sorta question the value of a sub-folder (sure doesn't hurt though).
BTW, s/never/always on the crop attribute which isn't supported by the textbox binding at all. I was surprised to see current pageInfo setting it on all of its textboxes for no apparent reason...
Heh, the textbox binding has a history of changing out from under us. Which attribute is no longer supported?
crop ;)
(In reply to comment #9)
> Or just patch the the current files first, I sorta question the value of a
> sub-folder (sure doesn't hurt though).
> 

Changing the name (or the location) of the files allows extensions to overlay both the old and the new page info, so that a single .xpi can contain an extension compatible with the newer versions of Firefox and Firefox 2.0, 1.5, ...
Changing the name of the global variables and scoping the functions in gPageInfoWindow will also be annoying for extensions developers, though if they have two different overlays they can have two different js files too.

> I'm not sure I see the point of overlaying the pageinfo dialog from the feeds
> component, let me think about it.

The feeds tab of page info needs (for the subscribe button) things that are in the feeds components, and that are built only if MOZ_FEEDS is defined.


> Has belzner approved the behavior of the subscribe button (appears only after
> selecting the item)?

No. Maybe mconnor, but I'm not sure.

> +function doHelpButton() {
[...]
> While you're rewriting this file, please make this method use a map, i.e.

Or remove this function completely? Are we going to have a help file for page info?
(In reply to comment #13)
> Changing the name (or the location) of the files allows extensions to overlay
> both the old and the new page info, so that a single .xpi can contain an
> extension compatible with the newer versions of Firefox and Firefox 2.0, 1.5,

Changing the location doesn't really help this situation. Its quite easy to use a different overlay depending on the appversion
What Mossap said (dynamic overlays are also a good fall back if you manage to use the old IDs for new UI elements).

Also, MOZ_FEEDS is a requirement for building Fx, see bug 355112 and bug 355444.

As for scoping, we can always add a shim for backwards-compatibility, see 
http://lxr.mozilla.org/seamonkey/source/toolkit/content/globalOverlay.js#216 for example.
(In reply to comment #13)

> > Has belzner approved the behavior of the subscribe button (appears only after
> > selecting the item)?
> 
> No. Maybe mconnor, but I'm not sure.

OK, CCing both.
> > +function doHelpButton() {
> [...]
> > While you're rewriting this file, please make this method use a map, i.e.
> 
> Or remove this function completely? Are we going to have a help file for page
> info?
Yes, adding documentation is bug 314211. But I wonder why we're not specifying the help topic in the xul file, like that:
<vbox id="generalPanel" helpTopic="pageinfo_general">
This would shorten doHelpButton() quite a bit.

By the way, the openHelp() call doesn't work until you add this:
<script type="application/x-javascript" src="chrome://help/content/contextHelp.js"/>
Blocks: 314211
Attached patch patch v4 security manager part (obsolete) — Splinter Review
Attachment #243136 - Flags: review?(kaie)
Attached patch patch v4 (obsolete) — Splinter Review
updated patch following most review comment.
I didn't scope everything in a gPageInfoWindow object. I'm not totally against the idea, but if I do it, I would prefer doing it in another bug.

I changed also two things following older review comments: I added constants for the columns numbers in imageView, and I changed the function processing recursively the frames so that the frames aren't processed in parallel anymore.
Attachment #241083 - Attachment is obsolete: true
Attachment #243138 - Flags: review?(mano)
Comment on attachment 243138 [details] [diff] [review]
patch v4

Please attach a diff between pageInfo.js to the new version.

What's the point of the no-op Makefile? You don't need to build this folder at all, jar.mn will peak the files for you.

I still think that having the feeds stuff applied as an overlay from the feeds component is an overkill, esp. now that browser/ cannot be built without --enable-feeds
Attachment #243138 - Flags: review?(mano) → review-
Oh, I didn't realize you've moved the files to browser/components. Since this is not really a component, the right place is browser/base/pageinfo.
Attachment #243136 - Flags: review?(kaie) → review?(kengert)
Attached patch patch v5 (obsolete) — Splinter Review
updated patch
Attachment #243138 - Attachment is obsolete: true
Attachment #243851 - Flags: review?(mano)
Comment on attachment 243852 [details] [diff] [review]
diff between pageInfo.js and the new version

General comment: code-style in browser/ is usually

function func_name(aFoo)
{
  if (...)
    one-line

  if (...) {
    multiple lines
  }
  else if (...) {
   ...
  }

  try {
   ...
  }
  catch(ex) { }
}

I don't mind if you choose something else, but please try to be consistent.

 
> //******** define a js object to implement nsITreeView
>-function pageInfoTreeView(columnids, copycol)
>+function gPageInfoTreeView(columnids, copycol)

Ugh, sorry, I read this code wrong earlier, thus I'm taking my comment back, this should remain pageInfoTreeView.

>+var permissionManager = Components.classes["@mozilla.org/permissionmanager;1"].getService(nsIPermissionManager);
>+var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
>+var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch2);
>+var aserv = Components.classes["@mozilla.org/atom-service;1"].getService(Components.interfaces.nsIAtomService);
>+

Please do not initialize those in the global scope; also, unless these are used very often, you don't need to cache them. this is getService, not createInstance.


>-  } 
>-  else 
>+    gWindow = null;

gWindow is already set to null in its declaration.

>+    gDocument = window.arguments[0].doc;
>+    docTitle = gBundle.getFormattedString("frameInfo.title", [gDocument.location.toString()]);

nit: you don't need to explicitly call toString.
 

>-    docTitle = theBundle.getString("pageInfo.title");
>+    docTitle = gBundle.getFormattedString("pageInfo.title", [gDocument.location.toString()]);

ditto.

>   }
> 
>   document.title = docTitle;
>-  
>-  document.getElementById("main-window").setAttribute("relatedUrl", theDocument.location.toString());
>+
>+  document.getElementById("main-window").setAttribute("relatedUrl", gDocument.location.toString());

Please fix this instance too.

>   // do the easy stuff first
>   makeGeneralTab();
> 
>+  // init media view
>+  var imageTree = document.getElementById("imagetree");
>+  gImageView.getCellProperties = function(row, col, props)
>+  {
>+    if (gImageView.data[row][COL_IMAGE_SIZE] == gStrings.unknown &&
>+        gImageView.data[row][COL_IMAGE_ADDRESS].indexOf('https:') != 0)

Is https: in the middle valid?

>     pageSize = cacheEntryDescriptor.dataSize;
>     kbSize = pageSize / 1024;
>-    sizeText = theBundle.getFormattedString("generalSize", [formatNumber(Math.round(kbSize*100)/100), formatNumber(pageSize)]);
>-
>-    expirationText = formatDate(cacheEntryDescriptor.expirationTime*kmsPerSec, gStrings.notSet);
>+    sizeText = gBundle.getFormattedString("generalSize", [formatNumber(Math.round(kbSize*100)/100), formatNumber(pageSize)]);

try to wrap your lines at 80 characters or less please.


>+  var ret = fp.show();
>+
>+  if (ret == nsIFilePicker.returnOK)
>+    return fp.file.QueryInterface(nsILocalFile);
>+  else
>+    return null;

nit: no |else| after return.

> }
> 
> function saveMedia()
> {
>   var tree = document.getElementById("imagetree");
>-  var item = getSelectedImage(tree);
>-  var url = imageView.data[tree.currentIndex][0];
>+  var count = tree.view.selection.count;
>+  if (count < 1)
>+    return;
> 
>-  if (url)
>-    // XXX Firefox specfic
>-    saveURL(url, null, 'SaveImageTitle', false, false, makeURI(item.baseURI));
>+  if (count == 1)
>+  {
>+    var item = getSelectedImage(tree);
>+    var url = gImageView.data[tree.currentIndex][COL_IMAGE_ADDRESS];
>+
>+    if (url)
>+      saveURL(url, null, 'SaveImageTitle', false, false, makeURI(item.baseURI));
>+  }
>+  else
>+  {
>+    var odir  = selectSaveFolder();
>+    var start = new Object();
>+    var end   = new Object();
>+    var numRanges = tree.view.selection.getRangeCount();
>+
>+    var rowArray = new Array();
>+    for (var t=0; t<numRanges; t++)
>+    {
>+      tree.view.selection.getRangeAt(t, start, end);
>+      for (var v = start.value; v <= end.value; v++)
>+        rowArray.push(v);
>+    }
>+    var saveAnImage = function()
>+    {
>+      var v = rowArray.shift();
>+      var dir  = odir.clone();
>+      var item = gImageView.data[v][COL_IMAGE_NODE];
>+      var url  = gImageView.data[v][COL_IMAGE_ADDRESS];
>+      var uri  = makeURI(url);
>+      try
>+      {
>+        var iuri = uri.QueryInterface(Components.interfaces.nsIURL);
>+        dir.append(decodeURIComponent(iuri.fileName));
>+      }
>+      catch(ex)
>+      {
>+      }
>+      internalSave(url, null, null, null, null, false, 'SaveImageTitle',
>+                    new AutoChosen(dir, uri), makeURI(item.baseURI));
>+      if (rowArray.length)
>+        setTimeout(saveAnImage, 200);
>+      // This delay is a hack which prevents the download manager from opening many times.
>+    }
>+    saveAnImage();
>+  }
>+}
>+
>+function onBlockImage()
>+{
>+  var checkbox = document.getElementById("blockImage");
>+  var url = document.getElementById("imageurltext").value;
>+  var host = ioService.newURI(url, null, null).host;
>+  if (checkbox.checked) {
>+    var uri = ioService.newURI("http://"+host, null, null);
>+    permissionManager.add(uri, "image", nsIPermissionManager.DENY_ACTION);
>+  } else {
>+    permissionManager.remove(host, "image");
>+  }

Why do you create the uri object twice? BTW, you may want to include contentAreaUtils.js and use the makeURI (js) method instead.


>+function makeBlockImage(url)
>+{
>+  var checkbox = document.getElementById("blockImage");
>+  var imagePref = prefs.getIntPref("permissions.default.image");
>+  if (!(/^https?:/.test(url)) || imagePref == 2)
>+    checkbox.setAttribute("hidden", "true");

Add a comment here explaing why should the checkbox be hidden.

nit: use the hidden property instead.

>+  else {
>+    var host = ioService.newURI(url, null, null).host;
>+    if (host)
>+    {
>+      checkbox.removeAttribute("hidden");

ditto.

>+      checkbox.label = gBundle.getFormattedString("mediaBlockImage", [host]);
>+      var uri = ioService.newURI("http://"+host, null, null);

again, why do we need the uri object twice?

>+      var perm = permissionManager.testPermission(uri, "image");
>+      if (perm == nsIPermissionManager.DENY_ACTION)
>+        checkbox.setAttribute("checked", "true");
>+      else
>+        checkbox.removeAttribute("checked");

you can simplify this:

checkbox.checked = perm == nsIPermissionManager.DENY_ACTION;


>+function doOpen(url)
>+{
>+  var where = prefs.getIntPref("browser.link.open_external");
>+  if (where == 1)
>+    // replace the current page
>+    gDocument.location.href = url;
>+  else if (where == 2)
>+    //open in new window
>+    window.open(url, gDocument);
>+  else {
>+    // open in new tab
>+    var browser;
>+    if (window.opener) {
>+      browser = window.opener.gBrowser;
>+    } else {
>+      var windowMediator =
>+        Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                  .getService(Components.interfaces.nsIWindowMediator);
>+      var browserWin = windowMediator.getMostRecentWindow("navigator:browser");
>+      if (!browserWin) {
>+        throw "Unable to get Browser";
>+        return;
>+      }
>+      browser = browserWin.getBrowser();
>+    }
>+    browser.selectedTab = browser.addTab(url, null, gDocument);
>+  }
>+}

you can use openUILink from utilityOverlay.js.
Attachment #243852 - Attachment is patch: true
Attachment #243852 - Flags: review-
Attachment #243851 - Flags: review?(mano)
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #243851 - Attachment is obsolete: true
(In reply to comment #24)
[...]
> >+  gImageView.getCellProperties = function(row, col, props)
> >+  {
> >+    if (gImageView.data[row][COL_IMAGE_SIZE] == gStrings.unknown &&
> >+        gImageView.data[row][COL_IMAGE_ADDRESS].indexOf('https:') != 0)
> 
> Is https: in the middle valid?

This test isn't to check the validity of the url. It's because the files loaded over https aren't cached (by default) and I think in this case we shouldn't display all rows as broken. Maybe I should test the browser.cache.disk_cache_ssl preference before?
Attachment #243852 - Attachment is obsolete: true
Attachment #244125 - Flags: review?(mano)
Comment on attachment 243136 [details] [diff] [review]
patch v4 security manager part

I made a whitespace-ignore-diff of the pre-patch version of .xul and the after-patch version of .js

You are simply moving the code from .xul to .js.

You are not changing any of the JS code.

What is the purpose of the change?

Can you please explain why you added
  #ifndef MOZ_PHOENIX
?
(In reply to comment #28)
> (From update of attachment 243136 [details] [diff] [review] [edit])
> I made a whitespace-ignore-diff of the pre-patch version of .xul and the
> after-patch version of .js
> 
> You are simply moving the code from .xul to .js.
> 
> You are not changing any of the JS code.
> 
> What is the purpose of the change?
> 
> Can you please explain why you added
>   #ifndef MOZ_PHOENIX
> ?
> 

In the old page info window, the xul file was an overlay which added the Security tab.

The new Page Info for Firefox displays the security info for the page on the first tab.
http://mozilla.queze.net/soc/progress/2006-08-25/pageinfo-soc-2006-08-25-1.png

It needs the JS code (unchanged), bug not the xul file.
So I moved the JS code to a separate file.
Florian, had a chance to discuss the feeds tab with beltzner?
(In reply to comment #30)
> Florian, had a chance to discuss the feeds tab with beltzner?
> 

No. The last discussion about the feeds tab was in September with mconnor, IIRC.
Share the details, please :) ?
To be clear, I don't see the point of only showing the button for the selected feed (and probably also not for having a button for each one).
Attachment #244125 - Flags: ui-review?(beltzner)
(In reply to comment #32)
> Share the details, please :) ?
> 

August, 31st
fqueze: I'm not really happy with the feed tab which still needs more work in my opinion
[...]
mconnor: do you have screenshots I can look at?
mconnor: for the feeds tab especially
fqueze: http://mozilla.queze.net/soc/progress/2006-08-25/
mconnor: hmm
fqueze: I would like to make the "using: Live Bookmarks" (on the last screenshot) be a dropdown list
mconnor: btw
mconnor: we want to redo the feed stuff to match the new branch feed subscription stuff
mconnor: also
mconnor: maybe you want to build a richlistbox-based UI there
mconnor: since both of those strings will be longish
mconnor: i.e. have the title in bold, and the link underneath
fqueze: hmm
mconnor: maybe have the feed type (RSS x/Atom) on the same line as the title
fqueze: :)
mconnor: seems like a plan eh? ;)

September, 18th
fqueze: http://mozilla.queze.net/soc/progress/2006-09-16/pageinfo-soc-2006-09-16-2.png do you like how it looks?
mconnor: RSS instead of Rss ;)
mconnor: please use the generic landmass version of the globe
mconnor: but
mconnor: I think I like this a lot
mconnor: :)

(In reply to comment #33)
> To be clear, I don't see the point of only showing the button for the selected
> feed (and probably also not for having a button for each one).
> 

I did that like it is in the new extension manager: the buttons for an extension appear only when they are useful (i.e. when that extension is selected).
Comment on attachment 244125 [details] [diff] [review]
diff between pageInfo.js and the new version (2)

Please set the review flag back once this is ui-reviewed.
Attachment #244125 - Flags: review?(mano)
Florian: thanks for your patience. I've been evaluating these changes for the past few days and have several comments. On the whole, there's a lot of great work here! I'm hoping to be able to post my full comment set tomorrow, along with what needs to be done before ui-r and what I think will probably need further evaluation through wider testing by landing this on trunk.
Comment on attachment 244125 [details] [diff] [review]
diff between pageInfo.js and the new version (2)

This is great work, and a huge step forward. I tested it using the extension on your web page since I couldn't get the code to compile (I'm assuming there's a titch of bitrot there).

Before I bless this for landing, though, there's a few nits that need to be worked out. Bullets marked + must be addressed to get ui-r+, those marked - are things that can be addressed in follow-on bugs as far as I'm concerned.

Finally, and again, sincere apologies for the delay. I've had these notes in various forms for months, but just never found the time to flesh them out and put them here. I owe you a drink. And a pony.

Overall comments
================

 + the theming doesn't seem to work well on OSX, with rendering errors plainly visible in the tab header; you'll want to reach out for some assistance there

 + when the primary content of a page is a table view, the first item in the table view should always be selected, but not given focus (like you have in the rich UI listbox on the "permission" tab)

 - in table views, the element name (image name, RSS feed title, etc) is always more interesting and recognizable than the URI, so it should be listed first

 - we might hear some screaming about removing the "Links" and "Forms" tabs, but I agree with your rationale about removing them as they're more DOM inspector-y and less "meta-information about this page"-y

General Tab
===========

 + we're going to want to create a separate security tab again, as there's increased security context (phishing, EV, etc) that we'll eventually want to expose here in the Firefox 3 timeframe; for now, maybe just re-use the existing security tab. You can leave a summary here (ie: "Encrypted", "Partially encrypted", "Not encrypted") and a button "(More)" that links over to a dedicated Security Tab.

 + munging the security icon with the general icon is confusing, please go back to the general globe icon and we'll reinstate the security icon when we recreate that tab as per above

 + the meta tags should be available in the General tab having the meta tags in an expandable section would be useful

 - the encoding of a page seems like it's important meta-information to have, so we'll probably want to re-instate it; we need to get more feedback from users here. 

 - right-click options on all fields should be for "Copy" which copies the entire field contents.

 - might want to consider a "Copy details" button which creates a tabbed clipboard entry of the information visible so a user can paste it into a text document

Media Tab
=========

 + [Mac only?] the "media preview" block resizes to fit the image, causing some of the items between the table view and the media preview to get clipped

 + [Mac only?] the "Save As..." button isn't styled as a button, it's just text floating there, until you hover over it at which point the button shows (may be related to above)

 + don't duplicate the permission for blocking images here and on the permissions tab - it should be in one place or the other (seems to make more sense on the permissions tab)

 - the table should show the image name (without the full URI) as a "name" column, then the type column, then the "location" column

 - the address link should have a right click "copy image location" item (the string should be available from the browser.js context menu)

 - we need to build in recognition for a bunch more media types here, perhaps even incorporating code from things like VideoDownloader to get direct links to embedded FLV files and the like; calling this "media" ATM seems vestigal since few people embed the type of media we can detect, so the type is almost always "Image"

 - nit: the (Save As...) button should be below the media preview, not above it

 - it would be nice to have "Save As" and "Save All" links on the context menu in the table view

 - [Mac only?] selecting all in the table view makes the rest of the tab contents vanish
 
Feed Tab
========
 + The tab should always appear, even when there aren't any feeds on a page (at that point it should just be empty)

 + [Mac only?] the "preview in %shortname" only appears when I've selected something. It should always appear, and just be disabled when nothing's selected.

 - the UI here isn't the greatest right now, but I'm torn on what the right way to go is. I kind of like the mockup you made here: http://mozilla.queze.net/soc/progress/2006-08-18/pageinfo-soc-2006-08-18-3.png but it does look a little busy. I wonder if something like:
   
   Top Stories                   ( Subscribe ) (Preview)
   Recently added links          ( Subscribe ) (Preview)

   might work well

 - your mockups showed you playing with including the format of the feed; can we get that from the parser? If so, do we think it's really worth including? maybe it is ...

Permissions Tab
===============
 - while I like the Addons Manager style rich UI, I wonder if it's neccessary instead of just a simpler UI like:

   [ ] Use default permissions

      [ ] Load images
      [ ] Open popup windows
      [ ] Accept cookies
          [ ] only for this session
      [ ] Allow this page to install add ons

Overall, I like the changes a lot. Once again, great work, and I really like that it came from SoC!
Attachment #244125 - Flags: ui-review?(beltzner) → ui-review-
(In reply to comment #37)
> (From update of attachment 244125 [details] [diff] [review])
> I tested it using the extension on your web page
> since I couldn't get the code to compile
> (I'm assuming there's a titch of bitrot there).
I'll update both the extension and the patch.

> Overall comments
> ================
>  + the theming doesn't seem to work well on OSX, with rendering errors plainly
> visible in the tab header
I'll check that.

>  + when the primary content of a page is a table view, the first item in the
> table view should always be selected, but not given focus (like you have in the
> rich UI listbox on the "permission" tab)
An example? In which tab do you see this problem?

> General Tab
> ===========
>  + we're going to want to create a separate security tab again, as there's
> increased security context (phishing, EV, etc) that we'll eventually want to
> expose here in the Firefox 3 timeframe; for now, maybe just re-use the existing
> security tab. You can leave a summary here (ie: "Encrypted", "Partially
> encrypted", "Not encrypted") and a button "(More)" that links over to a
> dedicated Security Tab.
>  + munging the security icon with the general icon is confusing, please go back
> to the general globe icon and we'll reinstate the security icon when we
> recreate that tab as per above
OK

>  + the meta tags should be available in the General tab having the meta tags 
> in an expandable section would be useful
I'm not sure that someone without HTML knowledge would understand what those meta tags are. Maybe we should display only a few of them like for example "description"?
 
>  - the encoding of a page seems like it's important meta-information to have,
> so we'll probably want to re-instate it; we need to get more feedback from
> users here. 
That's important for developers, but who else cares?
 
>  - right-click options on all fields should be for "Copy" which copies the
> entire field contents.
For the moment we have the default textbox context menu, I haven't changed it, but I should do it, maybe in another bug.

> Media Tab
> =========
>  + [Mac only?] the "media preview" block resizes to fit the image, causing some
> of the items between the table view and the media preview to get clipped
That's not mac only. I'll try to fix that.

>  + [Mac only?] the "Save As..." button isn't styled as a button [...]
I'll check that.

>  + don't duplicate the permission for blocking images here and on the
> permissions tab - it should be in one place or the other (seems to make more
> sense on the permissions tab)
That's duplicated only for images on the same server as the page. This is very useful on the media tab for images coming from other servers.

>  - [Mac only?] selecting all in the table view makes the rest of the tab
> contents vanish
Not mac only. That's what I wanted. When multiple images are selected, we can't display the info for one of them, and we can't display it for all of them. So only "save as" is possible.

> Feed Tab
> ========
>  + The tab should always appear, even when there aren't any feeds on a page (at
> that point it should just be empty)
Why?
An updated xpi file (macos x only) is available there: http://mozilla.queze.net/soc/xpi/pageinfo-soc-macos-2007-03-27-1.xpi

ChangeLog:
 * Tab appearance fixed on mac os x
 * The Security tab is back (like the old one for the moment).
 * The meta tags are back on the general tab, in an expendable section which is closed when Page Info opens.
 * The security part in the general tab is now only a summary, with a button to go to the Security Tab. That security part is in an expandable groupbox which is open when Page Info opens.
 * The list of images in the media tab doesn't resize any more because of the size of the image preview.
 * The encoding of the page is back on the general tab.
Thanks, Florian. I want to try and get this landed, so I think we should move forward. There are a few things that we'll want to get sorted in follow up bugs, though, which I'll list here:

 - some of the text baselines seem clipped on OSX (like the bit in the Security part of the general tab)
 - in the Media tab, the UI still moves around as fields appear/vanish - this is distracting, and fields should always be there even if empty
 - there's still some redundant information in the Media tab, and some of the information could be laid out better for tighter information density

Overall, a fantastic improvement over what we have now, and more easily extended. Please attach a patch and I'll give it a ui-r.
Attached patch patch v8 (obsolete) — Splinter Review
Updated patch.
Attachment #241082 - Attachment is obsolete: true
Attachment #243136 - Attachment is obsolete: true
Attachment #244123 - Attachment is obsolete: true
Attachment #244125 - Attachment is obsolete: true
Attachment #260058 - Flags: ui-review?(beltzner)
Attachment #260058 - Flags: review?(mano)
Attachment #243136 - Flags: review?(kengert)
Attached image new image for the icons
Comment on attachment 260058 [details] [diff] [review]
patch v8

Let's make sure to file the follow up bugs that are outstanding from comment 37 and comment 40

Mano, over to you (finally).

Florian, thanks for your continued patience!
Attachment #260058 - Flags: ui-review?(beltzner) → ui-review+
Blocks: 273151
Comment on attachment 260058 [details] [diff] [review]
patch v8

Sorry it too me so long to get to this.

>? browser/themes/pinstripe/browser/pageInfo.png
>? browser/themes/winstripe/browser/pageInfo.png

So by the way, we're using the same image in both themes?

>Index: browser/base/content/browser.js
>===================================================================
> 
> // doc - document to use for source, or null for this window's document
> // initialTab - name of the initial tab to display, or null for the first tab
> function BrowserPageInfo(doc, initialTab)
> {
>   var args = {doc: doc, initialTab: initialTab};
>   toOpenDialogByTypeAndUrl("Browser:page-info",
>                            doc ? doc.location : window.content.document.location,
>-                           "chrome://browser/content/pageInfo.xul",
>-                           "chrome,dialog=no",
>+                           "chrome://browser/content/pageinfo/pageInfo.xul",
>+                           "chrome,menubar,extra-chrome,toolbar,dialog=no,resizable",

what is the window-features change for?

>Index: browser/base/content/pageinfo/feeds.js
>===================================================================
>+  const feedTypes = {
>+    "application/rss+xml": gBundle.getString("feedRss"),
>+    "application/atom+xml": gBundle.getString("feedAtom")
>+  };
>+
>+  // get the feeds
>+  var linkNodes = gDocument.getElementsByTagName("link");
>+  var length = linkNodes.length;
>+  for (var i = 0; i < length; i++)
>+    if (linkNodes[i].rel == "alternate" &&
>+        linkNodes[i].type in feedTypes &&
>+        linkNodes[i].href)
>+      addRow(linkNodes[i].title,
>+             feedTypes[linkNodes[i].type],
>+             linkNodes[i].href);
>+
>+  var feedListbox = document.getElementById("feedListbox");
>+  if (feedListbox.getRowCount() > 0)
>+    document.getElementById("feedTab").hidden = false;

>> Feed Tab
>> ========
>> + The tab should always appear, even when there aren't any feeds on a page (at
>> that point it should just be empty)
>Why?

Was that figured somewhere else?

>+function onSubscribeFeed()
>+{
>+  var listbox = document.getElementById("feedListbox");
>+  var elt = listbox.selectedItem;
>+  if (!elt)
>+    throw("No feed selected");
>+  var href = elt.getAttribute("feedURL");
>+  openUILink(href, null, false, true, false, null);

nit: just inline this - |elt.getAttribute("feedURL")|.

You don't need the to null-check elt, it would throw either way (or rather, never).

>Index: browser/base/content/pageinfo/feeds.xml
>===================================================================

>+  <binding id="feed" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <content>
>+      <xul:hbox flex="1">

What's this box for? you only have one element in place.

>+        <xul:vbox flex="1">
>+          <xul:hbox flex="1">
>+            <xul:textbox flex="1" readonly="true" xbl:inherits="value=name" style="font-weight:bold;"/>

Set a class here and style it in the themes.

>+            <xul:label xbl:inherits="value=type"/>
>+          </xul:hbox>
>+          <xul:textbox xbl:inherits="value=feedURL" readonly="true"/>
>+          <xul:hbox class="feed-susbscribe"/>

Set flex="1" here, and then you can just remove the feed-not-selected binding.

>+        </xul:vbox>
>+      </xul:hbox>
>+    </content>
>+  </binding>
>+
>+  <binding id="feed-selected">

nit: confusing naming, please choose a name that doesn't seem like it overrides
the "feed" binding.

>+    <content>
>+      <xul:hbox flex="1">

You can remove the containing box here too once you set flex="1" in the feed binding.

>Index: browser/base/content/pageinfo/pageInfo.js
>===================================================================
>+function onLoadPageInfo()
>+{
>+  //dump("===============================================================================\n");

just remove this.

>+
>+  // do the easy stuff first
>+  makeGeneralTab();
>+
>+  // init media view
>+  var imageTree = document.getElementById("imagetree");
>+  gImageView.getCellProperties = function(row, col, props)

This should rather be set when gImageView is initialized.

>+  {
>+    var aserv = Components.classes[ATOM_CONTRACTID]
>+                          .getService(Components.interfaces.nsIAtomService);
>+
>+    if (gImageView.data[row][COL_IMAGE_SIZE] == gStrings.unknown &&
>+        gImageView.data[row][COL_IMAGE_ADDRESS].indexOf('https:') != 0)

use a regexp here so we don't accept https: somewhere in the middle.

>+function toggleGroupbox(id)
>+{
>+  var elt = document.getElementById(id);
>+  if (elt.hasAttribute("closed")) {
>+    elt.removeAttribute("closed");
>+    if (id == "metaTags")
>+      elt.flex = 1;
>+  } else {

general comment: please avoid |} else [if] {| code style in new code.

>+function makeGeneralTab()
>+{
>+  var title = (gDocument.title) ? gBundle.getFormattedString("pageTitle", [gDocument.title]) : gBundle.getString("noPageTitle");

general comment: please try to keep the source code lines no longer than 80 characters.

declare sizeText here:
>+    if (cacheEntryDescriptor)
>+    {
>+      var pageSize = cacheEntryDescriptor.dataSize;
>+      var kbSize = formatNumber(Math.round(pageSize / 1024 * 100) / 100);
>+      var sizeText = gBundle.getFormattedString("mediaFileSize", [kbSize]);
>+    }
>+    else
>+      sizeText = gStrings.unknown;
>+    gImageView.addRow([url, type, sizeText, alt, 1, elem, isBg]);


>+function saveMedia()
>+{
>+  var tree = document.getElementById("imagetree");
>+  var count = tree.view.selection.count;
>+  if (count < 1)
>+    return;
>+

nit: you don't need this early return, there's nothing done outside the else block.

>+  if (count == 1)
>+  {
>+    var item = getSelectedImage(tree);
>+    var url = gImageView.data[tree.currentIndex][COL_IMAGE_ADDRESS];
>+
>+    if (url)
>+      saveURL(url, null, 'SaveImageTitle', false, false, makeURI(item.baseURI));

s/'/"/

>+  }
>+  else
>+  {
>+    var odir  = selectSaveFolder();
>+    var start = new Object();
>+    var end   = new Object();

s/new Object()/{ }/

>+    var saveAnImage = function()
>+    {
>+      var v = rowArray.shift();
>+      var dir  = odir.clone();
>+      var item = gImageView.data[v][COL_IMAGE_NODE];
>+      var url  = gImageView.data[v][COL_IMAGE_ADDRESS];
>+      var uri  = makeURI(url);
>+      try
>+      {
>+        var iuri = uri.QueryInterface(Components.interfaces.nsIURL);
>+        dir.append(decodeURIComponent(iuri.fileName));
>+      }
>+      catch(ex) { }
>+
>+      internalSave(url, null, null, null, null, false, 'SaveImageTitle',
>+                    new AutoChosen(dir, uri), makeURI(item.baseURI));

>+      if (rowArray.length)
>+        setTimeout(saveAnImage, 200);
>+      // This delay is a hack which prevents the download manager from opening many times.
>+    }
>+    saveAnImage();

Hrm, this is pretty confusing, the following would be cleaner IMO:

var saveAnImage = function(aURIString, aChosenData, aBaseURI) {
  internalSave(aURIString, null, null, null, null, false, "SaveImageTitle",
               aChosenData, aBaseURI);
}

for (var i=0; i < rowArray.length; i++) {
  var dir = odir.clone();
  var item = gImageView.data[rowArray[i][COL_IMAGE_NODE];
  var uriString = gImageView.data[v][COL_IMAGE_ADDRESS];
  var uri = makeURI(uriString);

  try {
    uri.QueryInterface(Components.Interfaces.nsIURL);
    dir.append(decodeURIComponent(uri.fileName));
  }
  catch(ex) { /* data: uris */ }

  if (i==0)
    saveAnImage(uriString, new AutoChosen(dir, uri), makeURI(item.baseURI));
  else {
    // This delay is a hack which prevents the download manager from opening many times.
    setTimeout(saveAnImage, 200, uriString, new AutoChosen(dir, uri),
               makeURI(item.baseURI));
  }
}

But ugh, is this issue filed? is this hack known to work after new the thread manager landing, etc.?


>Index: browser/base/content/pageinfo/permissions.js
>===================================================================
nit: all new code in browser/ should use aFoo arguments-style.

>+const ALLOW = nsIPermissionManager.ALLOW_ACTION;   // 1
>+const BLOCK = nsIPermissionManager.DENY_ACTION;    // 2
>+const SESSION = nsICookiePermission.ACCESS_SESSION;// 8
>+var gPermURI;
>+var gPrefs;

Please name gPrefObj's functions to keep us js-consle users happy.

>+var gPermObj = {
>+  image: function()
>+  {
>+    var imagePref = gPrefs.getIntPref("permissions.default.image");
>+    if (imagePref == 2)

nit: just inline imagePref (ditto for the other functions in gPermObj).

>Index: browser/locales/en-US/chrome/browser/pageInfo.dtd
>===================================================================
> <!ENTITY  generalType           "Type:">
> <!ENTITY  generalSize           "Size:">
> <!ENTITY  generalReferrer       "Referring URL:">
> <!ENTITY  generalSource         "Cache Source:">
> <!ENTITY  generalSent           "Sent:">
> <!ENTITY  generalModified       "Modified:">
> <!ENTITY  generalExpires        "Expires:">
> <!ENTITY  generalEncoding       "Encoding:">
>-<!ENTITY  generalMeta           "Meta:">
>+<!ENTITY  generalMeta           "Meta (1 tag)">

Please rename the entity so that localizers are notified.

>Index: browser/locales/en-US/chrome/browser/pageInfo.properties
>===================================================================

>-pageInfo.title=Page Info
>-frameInfo.title=Frame Info
>- 
>+pageInfo.title=Page Info - %S
>+frameInfo.title=Frame Info - %S
>+

ditoo.

>Index: browser/themes/pinstripe/browser/pageInfo.css
>===================================================================

>+richlistitem {
>+  -moz-binding: url("chrome://browser/content/pageinfo/feeds.xml#feed");

>+richlistitem .feed-susbscribe{
>+  -moz-binding: url("chrome://browser/content/pageinfo/feeds.xml#feed-not-selected");
>+}
>+
>+richlistitem[selected="true"] .feed-susbscribe{
>+  -moz-binding: url("chrome://browser/content/pageinfo/feeds.xml#feed-selected");
>+}
>+

Hrm, could you make these rules only apply to richlistitem[feed="true"] or such? Otherwise
this will apply to new panes as well.

Also, these should go into a new css file on the content side.

>Index: browser/themes/winstripe/browser/pageInfo.css
>===================================================================
>+
>+/* General Tab */
>+.caption-icon {
>+  width: 9px;
>+  height: 9px;
>+  background-repeat: no-repeat;
>+  background-position: center;
>+  margin-left: 1px;
>+  margin-right: 3px;

For RTL UI support, please use -moz-margin-start/-moz-margin-end in winstripe.

Looks pretty good otherwise.
Attachment #260058 - Flags: review?(mano) → review-
(In reply to comment #44)
> (From update of attachment 260058 [details] [diff] [review])
> >? browser/themes/pinstripe/browser/pageInfo.png
> >? browser/themes/winstripe/browser/pageInfo.png
> 
> So by the way, we're using the same image in both themes?
Yes it's the same image. Not sure if this is correct.

> >Index: browser/base/content/browser.js
> >-                           "chrome://browser/content/pageInfo.xul",
> >-                           "chrome,dialog=no",
> >+                           "chrome://browser/content/pageinfo/pageInfo.xul",
> >+                           "chrome,menubar,extra-chrome,toolbar,dialog=no,resizable",
> 
> what is the window-features change for?
|toolbar| so that we see the icons at the top, and when |toolbar| is set |resizable| is needed so that it keeps being resizable. The others that I added are useless.

> >> Feed Tab
> >> ========
> >> + The tab should always appear, even when there aren't any feeds on a page (at
> >> that point it should just be empty)
> >Why?
> 
> Was that figured somewhere else?
No. Maybe that would be better in another bug if we want to discuss it more...


>     // This delay is a hack which prevents the download manager from opening
> many times.
>     setTimeout(saveAnImage, 200, uriString, new AutoChosen(dir, uri),
>                makeURI(item.baseURI));
> But ugh, is this issue filed?
I don't know.

> is this hack known to work after new the thread manager landing, etc.?
When I tested it this morning, the hack worked and was still needed in my build, but it's not really up to date (20070324). When did the new thread manager land?
Attached patch patch v9 (obsolete) — Splinter Review
Attachment #260058 - Attachment is obsolete: true
Attachment #261415 - Flags: review?(mano)
Comment on attachment 261415 [details] [diff] [review]
patch v9

Reviewed over /msg.
Attachment #261415 - Flags: review?(mano)
Target Milestone: --- → Firefox 3 alpha4
Depends on: 377339
Attached patch patch v10 (obsolete) — Splinter Review
Attachment #261415 - Attachment is obsolete: true
Attachment #261425 - Flags: review?(mano)
Attached patch patch v11 (obsolete) — Splinter Review
Attachment #261425 - Attachment is obsolete: true
Attachment #261426 - Flags: review?(mano)
Attachment #261425 - Flags: review?(mano)
Comment on attachment 261426 [details] [diff] [review]
patch v11

OK, let's land this, please make sure to file all the follow ups we discussed over IRC and/or with beltzner.

r=mano.
I will add the missing line to pageInfo.css on checkin ;)
Attachment #261426 - Flags: review?(mano) → review+
Attached patch patch v12Splinter Review
Should be ready for checkin.
Attachment #261426 - Attachment is obsolete: true
mozilla/browser/base/jar.mn 1.112
mozilla/browser/base/content/browser.js 1.777
mozilla/browser/base/content/pageInfo.js delete
mozilla/browser/base/content/pageInfo.xul delete
mozilla/browser/base/content/pageinfo/feeds.js initial revision: 1.1
mozilla/browser/base/content/pageinfo/feeds.xml initial revision: 1.1
mozilla/browser/base/content/pageinfo/pageInfo.js initial revision: 1.1
mozilla/browser/base/content/pageinfo/pageInfo.xul initial revision: 1.1
mozilla/browser/base/content/pageinfo/permissions.js initial revision: 1.1
mozilla/browser/base/content/pageinfo/security.js initial revision: 1.1
mozilla/browser/locales/en-US/chrome/browser/pageInfo.dtd 1.10
mozilla/browser/locales/en-US/chrome/browser/pageInfo.properties 1.6
mozilla/browser/themes/pinstripe/browser/jar.mn 1.42
mozilla/browser/themes/pinstripe/browser/pageInfo.css 1.7
mozilla/browser/themes/pinstripe/browser/pageInfo.png initial revision: 1.1
mozilla/browser/themes/winstripe/browser/jar.mn 1.36
mozilla/browser/themes/winstripe/browser/pageInfo.css 1.7
mozilla/browser/themes/winstripe/browser/pageInfo.png initial revision: 1.1
mozilla/security/manager/pki/resources/jar.mn 1.54
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 377349
No longer blocks: 377349
Depends on: 377349
This is great work, thank you Florian.

Just one question to whoever knows the answer; why was the rendering mode removed from the General tab? It seemed quite useful.
Depends on: 377353
(In reply to comment #54)
> Just one question to whoever knows the answer; why was the rendering mode
> removed from the General tab? It seemed quite useful.

The goal is to make this window usable for someone without technical knowledge. While the rendering mode can seem quite useful to (web)developers, I fear it doesn't mean anything for most users. Basically, that's the same idea for anything that I removed.

However, it should really be easy to add those things back with an extension; and I will probably create soon an extension to add developer friendly information to Page Info.
Thank you. One last thing, your fix for bug 344660 made the types shown in the Image Properties window and Media tab inconsistent. You may want to address that.
Depends on: 377364
(In reply to comment #56)
> Thank you. One last thing, your fix for bug 344660 made the types shown in the
> Image Properties window and Media tab inconsistent. You may want to address
> that.

Yes, that's something I want to fix. See bug 377364 for that.
Can this bug fix bug 353721?
(In reply to comment #55)
> (In reply to comment #54)
> > Just one question to whoever knows the answer; why was the rendering mode
> > removed from the General tab? It seemed quite useful.
> 
> The goal is to make this window usable for someone without technical knowledge.
> While the rendering mode can seem quite useful to (web)developers, I fear it
> doesn't mean anything for most users.

I'm not sure I can follow your reasoning. Usefulness for average users doesn't necessarily degrade usefulness for developers. I, both as a developer and as an advanced user, already have plenty of extensions. That's fine. But I wouldn't want to install one just to get the rendering mode info.
At the same time, "Type", "Encoding", "Size" and usually the Meta info is still quite technical and of no interest for average users. Maybe there should be an "Advanced" tab.
Depends on: 377396
Depends on: 377397
(In reply to comment #59)
> At the same time, "Type", "Encoding", "Size" and usually the Meta info is still
> quite technical and of no interest for average users.
That's why initially I removed "Encoding" and the Meta tags too, but it seems
there's a limit of how much it is possible to remove without getting many
people complaining...

> Maybe there should be an "Advanced" tab.
As I wrote in comment 55, I plan to make an extension adding advanced information.
I'm open to any suggestion on this topic but this bug is not the right place to
discuss that.
(In reply to comment #60)
> (In reply to comment #59)
> > At the same time, "Type", "Encoding", "Size" and usually the Meta info is still
> > quite technical and of no interest for average users.
> That's why initially I removed "Encoding" and the Meta tags too, but it seems
> there's a limit of how much it is possible to remove without getting many
> people complaining...

Nobody would complain if it was moved into an "Advanced" tab.

> > Maybe there should be an "Advanced" tab.
> As I wrote in comment 55, I plan to make an extension adding advanced
> information.
> I'm open to any suggestion on this topic but this bug is not the right place to
> discuss that.

Right. Obviously I don't want to discuss an extension either, be it here or elsewhere. I was writing about the as-shipped feature set and appearance of Firefox.
(In reply to comment #61)
> Right. Obviously I don't want to discuss an extension either, be it here or
> elsewhere. I was writing about the as-shipped feature set and appearance of
> Firefox.

I think this bug is basically done now. I suggest that this discussion might be better on the newsgroups or as a new bug where the module peers can make their call on the outcome.


Depends on: 377440
Depends on: 377549
Blocks: 379183
Depends on: 386361
Blocks: 214265
No longer depends on: 214265
You need to log in before you can comment on or make changes to this bug.