491310 - Port changes in the new Firefox Page Info implementation since the initial port.

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Philip Chee]

We haven't been keeping up with the updates with the Firefox PageInfo implementation. I'm going to pick the easy ones first. In no particular order they are:

Bug 393986 - Unnecessarily creating arrays using map in page info

Bug 405105 - Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver] when closing the pageInfo window.
*** Serge Forgot permissions.js ***

Bug 426029 - Media preview displays broken images for images with data url  

Bug 458167 - (comments-only changes) Grammar Nazi Vol1: s/it's/its/

Bug 398170 - XUL fixes for accessibility on PageInfo screens
Bug 398910 - Non-namespaced ARIA role and property support

Bug 416799 - Buttons in Page Info > Security are stretched vertically when text wraps

Bug 378667 - Better "passwords for given realm" query support

Bug 415282 - Larry UI for Owner shows "No Information Provided," but Page Info says differently

Bug 420246 - gBrowser.securityUI doesn't clear invalid cert data after using back button

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Do the easy stuff first.

Firefox changes ported in this patch:

Bug 458167 - (comments-only changes) Grammar Nazi Vol1: s/it's/its/

Bug 393986 - Unnecessarily creating arrays using map in page info
Bug 405105 -  Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver] when closing the pageInfo window. (* Serge Forgot to do permissions.js *)

Bug 426029 -  Media preview displays broken images for images with data url  

Bug 398170 -  XUL fixes for accessibility on PageInfo screens
Bug 398910 -  Non-namespaced ARIA role and property support

Bug 416799 -  Buttons in Page Info > Security are stretched vertically when text wraps
Bug 378667 -  Better "passwords for given realm" query support
Bug 415282 -  Larry UI for Owner shows "No Information Provided," but Page Info says differently
Bug 420246 -  gBrowser.securityUI doesn't clear invalid cert data after using back button
Bug 482480 -  Text on provided identity information seems contradicting

>+  if (!mimeType && item instanceof nsIImageLoadingContent)
Firefox also tests for && !isBG here. Do we need this bit?

>+  if (!mimeType && item instanceof nsIImageLoadingContent)
>+    [mimeType, numFrames] = getContentTypeFromImgRequest(item);
...
>  function getContentTypeFromImgRequest(item) 
>   try {
>-    var imageItem = item.QueryInterface(nsIImageLoadingContent);
Do we still need the try block now that I'm doing |item instanceof nsIImageLoadingContent| beforehand?

>-            <label value="&permImage;" control="imageRadioGroup"/>
>-            <hbox>
>+            <label value="&permImage;" control="permImageGroup imageRadioGroup"/>
>+            <hbox id="permImageGroup" role="group">
I have no idea what this does, I'm just cutting and pasting here.

Comment 2

[[e-mail]]

(In reply to comment #1)
>>+            <label value="&permImage;" control="permImageGroup imageRadioGroup"/>
>>+            <hbox id="permImageGroup" role="group">
>I have no idea what this does, I'm just cutting and pasting here.
Looks like accessibility stuff?

Comment 3

[[e-mail]]

Comment on attachment 376254 [details] [diff] [review]
Patch v1.0 Do the easy stuff first.

>   try {
>-    var imageItem = item.QueryInterface(nsIImageLoadingContent);
You'd have to ask someone whether the imgIRequest methods can fail.

>+            <label value="&permImage;" control="permImageGroup imageRadioGroup"/>
I don't think these changes are right.

>+                <hbox align="center">
This doesn't look right. The row itself probably needs align="center", but I'm not sure offhand whether you then still need the hbox itself.

Comment 4

[Philip Chee]

>>+            <label value="&permImage;" control="permImageGroup imageRadioGroup"/>
>>+            <hbox id="permImageGroup" role="group">
> I have no idea what this does, I'm just cutting and pasting here.

>>+            <label value="&permImage;" control="permImageGroup imageRadioGroup"/>
> I don't think these changes are right.

Cc Marco. What is the right thing to do here?

Comment 5

[Philip Chee]

Attachment: PageInfo Security Tab

>>+                <hbox align="center">
> This doesn't look right. The row itself probably needs align="center", but I'm
> not sure offhand whether you then still need the hbox itself.

Looking at the surrounding code, it's a two column grid. If you get rid of the hbox and use a three column grid and then squish the pageinfo window horizontally, I don't think you can do what the current markup can (see attached screenshot for current behaviour).

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to comment #2)
> (In reply to comment #1)
> >>+            <label value="&permImage;" control="permImageGroup imageRadioGroup"/>
> >>+            <hbox id="permImageGroup" role="group">
> >I have no idea what this does, I'm just cutting and pasting here.
> Looks like accessibility stuff?

Exactly. What this does is:
a) Gives the hbox a role of "group" which makes it a groupbox element. The goal is to get screen readers to announce which permission one should either use the Default for (checkbox) or select radio buttons.
b) The label simply assigns the accessible name to both the radiogroup and the "virtual" group created under a). The focusable element is, however, only the radiogroup element so that the behavior is that if you click the label, you should be redirected to the radio buttons.

Comment 7

[Philip Chee]

Attachment: Patch v1.1

>>   try {
>>-    var imageItem = item.QueryInterface(nsIImageLoadingContent);
> You'd have to ask someone whether the imgIRequest methods can fail.

I asked in m.d.t.xpcom and this is what Boris had to say:

On Fri, 08 May 2009 00:37:44 -0400, Boris Zbarsky wrote:
> Philip Chee wrote:
>>     var imageRequest =
>> item.getRequest(nsIImageLoadingContent.CURRENT_REQUEST);
> 
> With that argument, this will never throw.  Might return null.  This is 
> documented in the API, I think.
> 
>>       mimeType = imageRequest.mimeType;
> 
> I don't think this ever throws in sane situations right now.  Not that 
> the IDL is useful on this score.  It might well lie, of course (e.g. 
> return an empty string if the image is not loaded enough).
> 
>>       var image = imageRequest.image;
> 
> Similar to mimeType.
> 
>>         numFrames = image.numFrames;
> 
> No idea; would need to audit the relevant code....
> 
> -Boris

Firefox has been using this code without a try/catch block for a while so it looks pretty baked in.

>>+            <label value="&permImage;" control="permImageGroup imageRadioGroup"/>
> I don't think these changes are right.

Marco says in Comment 6:

> b) The label simply assigns the accessible name to both the radiogroup and the
> "virtual" group created under a). The focusable element is, however, only the
> radiogroup element so that the behavior is that if you click the label, you
> should be redirected to the radio buttons.

>>+                <hbox align="center">
> This doesn't look right. The row itself probably needs align="center", but I'm
> not sure offhand whether you then still need the hbox itself.

I've put the align="center" on the <row> and it seems to work. I've kept the <hbox> since I didn't think converting the grid to three columns was necessary.

Comment 8

[[e-mail]]

(In reply to comment #6)
> the behavior is that if you click the label, you
> should be redirected to the radio buttons.
This is certainly the behaviour before the patch, but not with patch v1.0

Comment 9

[Philip Chee]

Comment on attachment 376624 [details] [diff] [review]
Patch v1.1

Cancelling review requests based on Comment 8

Comment 10

[Philip Chee]

Attachment: Patch v1.2 Use aria-labelledby

Marco says in Comment 6:

> b) The label simply assigns the accessible name to both the radiogroup and the
> "virtual" group created under a). The focusable element is, however, only the
> radiogroup element so that the behavior is that if you click the label, you
> should be redirected to the radio buttons.

Neil says in Comment 8:

>> the behavior is that if you click the label, you
>> should be redirected to the radio buttons.
> This is certainly the behaviour before the patch, but not with patch v1.0

I've switched to using aria-labelledby="label_id" which gets rid of the twee control bit, restores the focus to the radio buttons. I think that this should work for screen readers but I can't tell so I'm asking Marco to review the ARIA changes.

Comment 11

[Philip Chee]

> +  let imageRequest = item.getRequest(nsIImageLoadingContent.CURRENT_REQUEST); 
Err this let should be a var now that I've removed the try/catch block.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 377859 [details] [diff] [review]
Patch v1.2 Use aria-labelledby

Yep this is correct, thanks!

Comment 13

[[e-mail]]

Comment on attachment 377859 [details] [diff] [review]
Patch v1.2 Use aria-labelledby

Hmm, what if someone uses an <object> for an animated GIF?

>+  var numFrames;
= 0?

>               <row><!-- History -->
I think all three rows could use align="center"

>                 <hbox>
And the hboxes still need it too. (I won't make you convert them to a grid.)

Comment 14

[Philip Chee]

Attachment: [checked in] Patch v1.3 nits fixed.

Carrying forward r+/sr+

>> +  let imageRequest = item.getRequest(nsIImageLoadingContent.CURRENT_REQUEST); 
> Err this let should be a var now that I've removed the try/catch block.
Fixed.

>>+  var numFrames;
>= 0?
Fixed.

>>               <row><!-- History -->
> I think all three rows could use align="center"
Fixed.

>>                 <hbox>
> And the hboxes still need it too.
Fixed.

> (I won't make you convert them to a grid.)
Thank goodness for small favours.

> Hmm, what if someone uses an <object> for an animated GIF?
Dunno. This question is too subtle for a rodent of little brain.

Comment 15

[Philip Chee]

Putting this somewhere so I don't forget.
============= ToDo =============

Bug 315010 -  Page Info should list links and images in SVG pages.
  (part not ported?)

Bug 377364 - Page Info: Suppress the jumping in the bottom half of the Media tab. Improve consistency with the Element Properties

Bug 388504 -  It should be possible for an extension to reload Page Info on a different document
  (Suite Bug 444917)

Bug 458579 -  Feed tab missing in page info
  (for opening/focusing existing pageinfo window)

Bug 493225 -  Remove the unused members of pageInfoTreeView

Bug 419437 -  Missing context menu icons
  (context menu item IDs part)

Bug 448833 -  Page Info > Security > More is confusing

Bug 491732 -  add "Share Location" to Page Info
  (See Suite Bug 491835)

Bug 383880 -  The Page Info RSS section's feed links should be clickable  
Bug 418879 -  Long feed URLs cause scrollbar to appear in Page Info
Bug 444878 -  Feed URLs needs tooltip in Page Info

Probably not applicable to Suite:
Bug 388334 -  Page Info theme should be more reusable by extensions.

Comment 16

[[e-mail]]

(In reply to comment #14)
> > Hmm, what if someone uses an <object> for an animated GIF?
> Dunno. This question is too subtle for a rodent of little brain.
I filed bug 493484 on Firefox so you could just wait and port their fix ;-)

Comment 17

[Robert Kaiser]

Comment on attachment 377936 [details] [diff] [review]
[checked in] Patch v1.3 nits fixed.

Pushed as http://hg.mozilla.org/comm-central/rev/408645ca4920

Comment 18

[Philip Chee]

Attachment: Patch v2.0 Part 2

Firefox changes ported in this patch:

Bug 419437 -  Missing context menu icons
  (This drive by patch added a couple of IDs to the context menu)

Bug 448833 -  Page Info > Security > More is confusing

Bug 386333 -  Page Info knows nothing about dataURL images embeded via CSS

feeds.xml:
---------_
Bug 383880 -  The Page Info RSS section's feed links should be clickable  
Bug 418879 -  Long feed URLs cause scrollbar to appear in Page Info
Bug 444878 -  Feed URLs needs tooltip in Page Info
  (These are basically the same patch but it took them three tries to get it right)

Comment 19

[Philip Chee]

Attachment: Patch v2.1 Part 2

Now includes one line fix for SeaMonkey Bug 156734 - embed objects not shown in page info preview

> Firefox changes ported in this patch:
> 
> Bug 419437 - Missing context menu icons
>   (This drive by patch added a couple of IDs to the context menu)
> 
> Bug 448833 - Page Info > Security > More is confusing
> 
> Bug 386333 - Page Info knows nothing about dataURL images embeded via CSS
> 
> feeds.xml:
> ---------_
> Bug 383880 - The Page Info RSS section's feed links should be clickable  
> Bug 418879 - Long feed URLs cause scrollbar to appear in Page Info
> Bug 444878 - Feed URLs needs tooltip in Page Info
>   (These are basically the same patch but it took them three tries to get it
> right)

Comment 20

[Philip Chee]

FYI: These are fixed:
> Bug 315010 -  Page Info should list links and images in SVG pages.
  (I was mistaken every thing here seems to have been ported)
> Bug 458579 -  Feed tab missing in page info
  remaining part |.notifyObservers(window, "page-info-dialog-loaded", null)| fixed in Bug 444917.

Comment 21

[[e-mail]]

Comment on attachment 384270 [details] [diff] [review]
Patch v2.1 Part 2

>+        <xul:vbox>
>+          <xul:vbox align="start">
>+            <xul:hbox>
>+              <xul:label xbl:inherits="value=feedURL,tooltiptext=feedURL"
>+                         class="text-link"
>+                         flex="1"
>+                         onclick="openUILink(this.value, event);" crop="end"/>
>+            </xul:hbox>
>+          </xul:vbox>
>+        </xul:vbox>
Whoa, there are way too many boxes here. Just what are you trying to achieve?

>+  if (!mimeType && /^data:/.test(url)) {
>+    var dataMimeType = /^data:(image\/.*);/.exec(url);
Duplicate test - not worth testing for data: both without and with image

>+    if (dataMimeType)
>+      mimeType = dataMimeType[1];
Nit: use .test and RegExp.$1

>+      (item instanceof HTMLObjectElement && /^image\//.test(mimeType)) ||
>+      (item instanceof HTMLEmbedElement && /^image\//.test(mimeType)) ||
[Could possibly avoid duplicating the text of the image text. Also I notice that the old code had the indentation wrong too.]

Comment 22

[Philip Chee]

>>+            </xul:hbox>
>>+          </xul:vbox>
>>+        </xul:vbox>
> Whoa, there are way too many boxes here. Just what are you trying to achieve?

But Neil, but that's actually *your* code from bug 418879 comment 21:

> Because the inner vbox has align="start" the hbox will only attempt to consume
> the preferred width of the label however as the label text length increases its
> preferred width increases until the vbox can take no more and then as it is
> flexible the label should then begin to crop. At least, that's the theory!

>>+    if (dataMimeType)
>>+      mimeType = dataMimeType[1];
> Nit: use .test and RegExp.$1

<https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Deprecated_Features#RegExp_Properties>
$1, ..., $9 are deprecated. Any other suggestions?

Comment 23

[[e-mail]]

(In reply to comment #22)
>But Neil, but that's actually *your* code from bug 418879 comment 21
Oh dear. Good thing we don't have QBO for Bugzilla...

>>Nit: use .test and RegExp.$1
><https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Deprecated_Features#RegExp_Properties>
>$1, ..., $9 are deprecated. Any other suggestions?
Tell that to the 96 users :-P OK, so some are tests of RegExp.$1 ;-)

Comment 24

[[e-mail]]

Comment on attachment 384270 [details] [diff] [review]
Patch v2.1 Part 2

>+  // if we have a data url, get the MIME type from the url
>+  if (!mimeType && /^data:/.test(url)) {
>+    var dataMimeType = /^data:(image\/.*);/.exec(url);
OK, so I noticed anyway that this regexp is wrong - data: url's don't have to have a ; it could be a , and also this looks for the last ; instead of the first and also it should be case insensitive. (Phew!)

>     let imageMimeType = /^image\/(.*)/.exec(mimeType);
Just noticed that exec is file style here, so go back to that.

Comment 25

[Philip Chee]

Attachment: Patch v2.2 Part 2

>>+  if (!mimeType && /^data:/.test(url)) {
>>+    var dataMimeType = /^data:(image\/.*);/.exec(url);
> Duplicate test - not worth testing for data: both without and with image
Fixed.

> OK, so I noticed anyway that this regexp is wrong - data: url's don't have to
> have a ; it could be a , and also this looks for the last ; instead of the
> first and also it should be case insensitive. (Phew!)
Fixed. Although I can't come up with a test case for a image data url that isn't base64 encoded.

>>+    if (dataMimeType)
>>+      mimeType = dataMimeType[1];
> Nit: use .test and RegExp.$1
  [...]
> Just noticed that exec is file style here, so go back to that.
OK.

>>+      (item instanceof HTMLObjectElement && /^image\//.test(mimeType)) ||
>>+      (item instanceof HTMLEmbedElement && /^image\//.test(mimeType)) ||
> [Could possibly avoid duplicating the text of the image text.
Fixed.

> Also I notice that the old code had the indentation wrong too.]
I think the intention was to vertically align on |item instanceof|

Comment 26

[[e-mail]]

Comment on attachment 384411 [details] [diff] [review]
Patch v2.2 Part 2

>       <xul:vbox flex="1">
>         <xul:hbox flex="1">
>           <xul:textbox flex="1" readonly="true" xbl:inherits="value=name"
>                        class="feedTitle"/>
>           <xul:label xbl:inherits="value=type"/>
>         </xul:hbox>
>-        <xul:textbox xbl:inherits="value=feedURL" readonly="true"/>
>+        <xul:vbox>
Ah, so the outer <vbox> in my comment on that other bug actually referred to the one at the top of this snippet; you don't need this one here too.

>+          <xul:vbox align="start">
You might want to add flex="1" to keep in the style of the other boxes (although in fact the flex on these outer boxes has no effect whatsoever.)

>+              <xul:label xbl:inherits="value=feedURL,tooltiptext=feedURL"
>+                         class="text-link"
>+                         flex="1"
>+                         onclick="openUILink(this.value, event);" crop="end"/>
Hmm, for links, don't we normally crop="center"?

>+    var dataMimeType = /^data:(image\/.*?)[;,]/i.exec(url);
>+    if (dataMimeType)
>+      mimeType = dataMimeType[1];
I think this needs a .toLowerCase(); - as I recall the other sources of a mime type already provide lower case.

Comment 27

[Philip Chee]

Attachment: [checked in] Patch v2.3 Part 2 (nits fixed) r=neil sr=neil

Carrying forward r+/sr+ from Neil.

>>+        <xul:vbox>
> Ah, so the outer <vbox> in my comment on that other bug actually referred to
> the one at the top of this snippet; you don't need this one here too.
Fixed.

>>+          <xul:vbox align="start">
> You might want to add flex="1" to keep in the style of the other boxes
> (although in fact the flex on these outer boxes has no effect whatsoever.)
Fixed.

>>+                         onclick="openUILink(this.value, event);" crop="end"/>
> Hmm, for links, don't we normally crop="center"?
In this particular case I think crop="end" works better. In any case there is a tooltip so the user can see the whole URL anyway. Leaving as crop="end"
 
>>+    var dataMimeType = /^data:(image\/.*?)[;,]/i.exec(url);
>>+    if (dataMimeType)
>>+      mimeType = dataMimeType[1];
> I think this needs a .toLowerCase(); - as I recall the other sources of a mime
> type already provide lower case.
Fixed.

Comment 28

[Robert Kaiser]

Comment on attachment 384800 [details] [diff] [review]
[checked in] Patch v2.3 Part 2 (nits fixed) r=neil sr=neil

Pushed as http://hg.mozilla.org/comm-central/rev/5a909c7ad82d - I'll leave it to Philip to mark FIXED if applicable.

Comment 29

[Philip Chee]

============= To Do =============

Bug 377364 - Page Info: Suppress the jumping in the bottom half of the Media tab. Improve consistency with the Element Properties

Bug 388334 - Page Info theme should be more reusable by extensions.
  (Probably not applicable to Suite)

Bug 398928 - Allow moving a unified window by dragging its toolbar

Bug 491732 - add "Share Location" to Page Info
  (Suite Bug 491835)

Bug 491825 - Remove spaces from strings

Bug 493225 - Remove the unused members of pageInfoTreeView

============= Gecko 1.9.2 =============

Bug 456106 - Switch to new drag and drop api in toolkit/browser

Comment 30

[Philip Chee]

Attachment: [checked in] Patch Sv1.0 Remove trailing spaces.

Port Bug 491825 - Remove spaces from strings

Trivial fix. According to KaiRo over irc:

<Ratty> any L10n impact?
<KaiRo> Ratty: actually, no, should be good to land within the slushy freeze without L10n-specific approval, the meaning and the string IDs are unchanged.

Comment 31

[Philip Chee]

Firefox Bug 377364 makes the following changes to the media tab:

1. The specified dimensions and actual dimensions (when different) are on the same row,
2. When the size is unknown, display "Unknown (not cached)"
3. ...and get rid of the "Cache Source" row,
4. Concantate the Title and Alt row into one "Associated text".
5. A min-height value for the grid was added in the CSS to always keep enough room for 5 rows, to prevent the jumping behavior they had before.  The only (uncommon) case that will still make the UI jump is when both the alt (or title) and
longdesc attributes are set on the HTML tag.

I think we should port #1 and #2. I am doubtful about #3 and #4. #5 sounds useful (adjusted to the number of rows we have). What do you think?

Comment 32

[[e-mail]]

I don't see us wanting any of these changes.

(In reply to comment #31)
> 1. The specified dimensions and actual dimensions (when different) are on the
> same row,
> 2. When the size is unknown, display "Unknown (not cached)"
> 3. ...and get rid of the "Cache Source" row,
> 4. Concantate the Title and Alt row into one "Associated text".
> 5. A min-height value for the grid was added in the CSS to always keep enough
> room for 5 rows, to prevent the jumping behavior they had before.  The only
> (uncommon) case that will still make the UI jump is when both the alt (or
> title) and longdesc attributes are set on the HTML tag.
Do they hide rows rather than, um, specifying "Not specified"?

Comment 33

[Philip Chee]

> I don't see us wanting any of these changes.
> 
> (In reply to comment #31)
>> 1. The specified dimensions and actual dimensions (when different) are on the
>> same row,
I'd like at least to get #1. e.g.

Before:
Specified Dimensions: N x N
Actual Dimensions: N x N 

After:
Image Dimensions: N x N (scaled to N x N)

This is in fact a 4px bug see attachment 78014 [details] where NS4 uses scaled _from_ rather than _to_.

I don't care about the rest either.

[...]
>> 5. A min-height value for the grid was added in the CSS to always keep enough
>> room for 5 rows, to prevent the jumping behavior they had before.  The only
>> (uncommon) case that will still make the UI jump is when both the alt (or
>> title) and longdesc attributes are set on the HTML tag.
> Do they hide rows rather than, um, specifying "Not specified"?
Yup. They do they do (well .collapsed anyway)

Comment 34

[Stefan [:stefanh]]

Comment on attachment 386668 [details] [diff] [review]
[checked in] Patch Sv1.0 Remove trailing spaces.

http://hg.mozilla.org/comm-central/rev/a191c217645a

(I'm not resolving this since I'm unsure if everything is done here)

Comment 35

[Philip Chee]

Attachment: Patch Tv1.0 for pageInfoTreeView()

-  this.copycol = copycol;
+  this.copycol = arguments.length == 2 ? arguments[1] : copycol;

Firefox doesn't do this but I didn't want to break the existing pageInfo extensions I have installed that try sending pageInfoTreeView() two arguments with the copycol in the second parameter.

Comment 36

[[e-mail]]

Comment on attachment 389967 [details] [diff] [review]
Patch Tv1.0 for pageInfoTreeView()

>-  this.copycol = copycol;
>+  this.copycol = arguments.length == 2 ? arguments[1] : copycol;
Maybe keep two arguments and use arg2 || arg1 to choose?

Comment 37

[Philip Chee]

Attachment: Patch Tv1.1 for pageInfoTreeView()

> Maybe keep two arguments
I'd like to keep the function signature the same as Firefox. Besides an extension developer new to SeaMonkey without any knowledge of the history might be somewhat confused at why we take two arguments but only use one.

> and use arg2 || arg1 to choose?
How about:

+  this.copycol = arguments[1] || copycol;

Not too unreadable I hope. I've added a comment explaining why we do this.

Comment 38

[[e-mail]]

(In reply to comment #37)
> How about:
> 
> +  this.copycol = arguments[1] || copycol;
> 
> Not too unreadable I hope. I've added a comment explaining why we do this.
No, it would have to be two arguments to make sense. But I like the comment.

Comment 39

[Philip Chee]

Attachment: Patch Tv1.2 for pageInfoTreeView() [Checkin: Comment 40]

> No, it would have to be two arguments to make sense. But I like the comment.

Over IRC: <NeilAway> I think I prefer the first version, but with the extra comment

OK. Nits fixed.

Comment 40

[Serge Gautherie (:sgautherie)]

Comment on attachment 390459 [details] [diff] [review]
Patch Tv1.2 for pageInfoTreeView()
[Checkin: Comment 40]


http://hg.mozilla.org/comm-central/rev/fb2a3908a314

Comment 41

[Philip Chee]

I think that's it. I'll close this bug and file new bugs for any other issues.

Summary of remnants:

============= Covered Elsewhere =============
Bug 398928 - Allow moving a unified window by dragging its toolbar
  (Part of Suite Bug 460699)

============= WONTFIX =============
Bug 377364 - Page Info: Suppress the jumping in the bottom half of the Media tab. Improve consistency with the Element Properties

============= NOT APPLICABLE =============
Bug 388334 - Page Info theme should be more reusable by extensions.
Bug 484374 - groupbox.collapsable needs the adjustment of margin/padding

============= Gecko 1.9.2 =============
Bug 456106 - Switch to new drag and drop api in toolkit/browser
Bug 494808 - Page Info > Media doesn't list background images anymore