Closed
Bug 201264
Opened 21 years ago
Closed 19 years ago
Media tab should have a column named "count" for objects used more than once, instead of repeating them
Categories
(SeaMonkey :: Page Info, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: berkut.bugzilla, Assigned: florian)
References
Details
(Keywords: polish)
Attachments
(3 files, 4 obsolete files)
21.92 KB,
image/png
|
Details | |
13.85 KB,
patch
|
florian
:
review+
florian
:
superreview+
asa
:
approval-aviary1.1a2+
asa
:
approval1.8b3+
|
Details | Diff | Splinter Review |
2.47 KB,
patch
|
db48x
:
review+
neil
:
superreview+
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312 In Page Info window --> Media tab instead of listing an image (or other media type) everytime it's used in a webpage, list it only once and have a colunm named something like "count", with number of instances the same image (or other type of media) is used in a page. This will greatly reduce clutter and tidy up the Media tab. Reproducible: Always Steps to Reproduce: 1. Right click on a page, select Page Info 2. Go to Media tab 3. Actual Results: If the page uses same iamge many times, there will be a line for each sue, even though it's the same image. Expected Results: Change this behavior to list an image only once in one line, and add a "count" column. I know I'm using an old build, but I didn't want to wait for new build to download, because I'm pretty sure it's same in new builds. Maybe I'll attach a screenshot later once I figure out how to do it.
Reporter | ||
Comment 1•21 years ago
|
||
Comment 2•21 years ago
|
||
I'm confirming this bug as I have found no dupes
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Updated•20 years ago
|
Keywords: polish
Summary: page info media tab should have a column named "count" for images used more than once in a webpage → Media tab should have a column named "count" for objects used more than once, instead of repeating them
Updated•20 years ago
|
Product: Browser → Seamonkey
Assignee | ||
Comment 3•19 years ago
|
||
Attachment #186012 -
Flags: review?(db48x)
Assignee | ||
Comment 4•19 years ago
|
||
I'm sorry, in the previous diff file paths were wrong.
Attachment #186012 -
Attachment is obsolete: true
Attachment #186013 -
Flags: review?(db48x)
Assignee | ||
Updated•19 years ago
|
Attachment #186012 -
Flags: review?(db48x)
Assignee | ||
Updated•19 years ago
|
Attachment #186013 -
Attachment is obsolete: true
Attachment #186013 -
Flags: review?(db48x)
Assignee | ||
Comment 5•19 years ago
|
||
Comment 6•19 years ago
|
||
Comment on attachment 186018 [details] [diff] [review] patch v3 >+ <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="4" >+ width="1" id="image-count" label="&mediaCount;"/> >+ <treecol sortSeparators="true" hidden="true" persist="hidden width" flex="4" >+ width="1" id="image-count" label="&mediaCount;"/> make the flex="1". r=db48x with that one change.
Attachment #186018 -
Flags: review+
Assignee | ||
Comment 7•19 years ago
|
||
Attachment #186018 -
Attachment is obsolete: true
Updated•19 years ago
|
Attachment #186033 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #186033 -
Flags: superreview?(jag)
Comment 8•19 years ago
|
||
Comment on attachment 186033 [details] [diff] [review] patch v4 >Index: browser/base/content/pageInfo.js >Index: browser/base/content/pageInfo.xul >Index: browser/locales/en-US/chrome/browser/pageInfo.dtd While I can r= for browser/, I can't sr=. >Index: xpfe/browser/resources/content/pageInfo.js >=================================================================== I think it would be useful to set up some |const|s for the column indices so if we need to add any other columns we won't have to worry about fixing "magic constants" throughout the code. It'll also make the code more readable, e.g.: imageView.data[i][COUNT_COLUMN]++; Perhaps in another bug, though. >@@ -499,18 +501,36 @@ > view.selection.select(0); > } > >+function addImage(url, type, alt, elem, isBg) >+{ >+ if (url == "") >+ return; >+ if (!imageHash[url] || !imageHash[url][type] || !imageHash[url][type][alt]) { >+ imageView.addRow([url, type, alt, 1, elem, isBg]); >+ // I wish I could do imageHash[url][type][alt] = imageView.data.length without getting errors. >+ if (!imageHash[url]) >+ imageHash[url] = {}; >+ if (!imageHash[url][type]) >+ imageHash[url][type] = {}; >+ imageHash[url][type][alt] = imageView.data.length; >+ } else { >+ var i = imageHash[url][type][alt] - 1; >+ imageView.data[i][3]++; >+ } >+} Just being nit-picky here, but it seems to me your imageHash[][][] gives an index into imageView.data. I would actually store |imageView.data.length - 1| to make that point a bit more clear. sr=jag
Attachment #186033 -
Flags: superreview?(jag) → superreview+
Assignee | ||
Comment 9•19 years ago
|
||
(In reply to comment #8) > Just being nit-picky here, but it seems to me your imageHash[][][] gives an > index into imageView.data. I would actually store |imageView.data.length - 1| > to make that point a bit more clear. If I store |imageView.data.length - 1| then the first index will be 0 and |!imageHash[url][type][alt]| will be |true|. Is it worth changing indexes and the test (to something like |typeof(imageHash[url][type][alt]) == "undefined"|)?
Comment 10•19 years ago
|
||
(In reply to comment #9) > (In reply to comment #8) > > Just being nit-picky here, but it seems to me your imageHash[][][] gives an > > index into imageView.data. I would actually store |imageView.data.length - 1| > > to make that point a bit more clear. > > If I store |imageView.data.length - 1| then the first index will be 0 and > |!imageHash[url][type][alt]| will be |true|. > > Is it worth changing indexes and the test (to something like > |typeof(imageHash[url][type][alt]) == "undefined"|)? Ah yes, I see now. That's a dangerous bit in the code, someone's bound to make the same mistake I did. I would say either document this with a comment, or by making that last test become |imageHash[url][type][alt] === undefined|, perhaps with a smaller comment pointing out that it avoids matching row index 0. Triple equals is called the identity test. |0 === undefined| evaluates to false, while |undefined === undefined| is true.
Assignee | ||
Comment 11•19 years ago
|
||
Same patch, but the index values stored are now |imageView.data.length - 1| as super-reviewer requested. Carrying forward r+ and sr+ and requesting approval.
Assignee: db48x → f.qu
Attachment #186033 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #186786 -
Flags: superreview+
Attachment #186786 -
Flags: review+
Attachment #186786 -
Flags: approval1.8b3?
Attachment #186786 -
Flags: approval-aviary1.1a2?
Updated•19 years ago
|
Attachment #186786 -
Flags: approval1.8b3?
Attachment #186786 -
Flags: approval1.8b3+
Attachment #186786 -
Flags: approval-aviary1.1a2?
Attachment #186786 -
Flags: approval-aviary1.1a2+
Assignee | ||
Updated•19 years ago
|
Whiteboard: [checkin needed]
Comment 12•19 years ago
|
||
Chris, willing to check this in if not resolved before this mail gets to you? (author was poking in #developers)
Assignee | ||
Comment 13•19 years ago
|
||
Patch checked in. Thanks timeless.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Comment 14•19 years ago
|
||
(In reply to comment #9) >If I store |imageView.data.length - 1| then the first index will be 0 and >|!imageHash[url][type][alt]| will be |true|. You should use the in operator, then not only doesn't the value matter, but you also avoid spaming the JS console with javascript strict warnings. if (!(url in imageHash)) imageHash[url] = {}; if (!(type in imageHash[url])) imageHash[type] = {}; if (!(alt in imageHash[url][type])) { imageHash[url][type][alt] = imageView.data.length; imageView.addRow(url, type, alt, 1, elem, isBg); } else { imageView.data[imageHash[url][type][alt]][3]++; }
Assignee | ||
Comment 15•19 years ago
|
||
(In reply to comment #14) > You should use the in operator Yes.
Attachment #188961 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #188961 -
Flags: review?(db48x)
Comment 16•19 years ago
|
||
Comment on attachment 188961 [details] [diff] [review] patch v6 Good, you fixed my typo - at 00:32 AM I also can't spell either ;-)
Attachment #188961 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Updated•19 years ago
|
Attachment #188961 -
Flags: review?(db48x) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #188961 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #188961 -
Flags: approval1.8b4?
Assignee | ||
Updated•19 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•19 years ago
|
Attachment #188961 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #188961 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 17•19 years ago
|
||
patch v6 checked in by Timeless. Marking FIXED.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•