[inspector] remove bitmap/png and search functionality

RESOLVED FIXED

Status

--
minor
RESOLVED FIXED
16 years ago
12 years ago

People

(Reporter: dwitte, Assigned: sdwilsh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
caillon mentioned that this stuff should probably be removed from inspector,
specifically:

- the png writing functionality (inIPNGEncoder)
- all bitmap-related stuff (inIBitmap*)
- its associated boxModel.js fu
- inIFileSearch and related search fu

patch coming up.
(Reporter)

Comment 1

16 years ago
this does the aforementioned, minus inIFileSearch. i'll do that in a separate
patch.
(Reporter)

Comment 2

16 years ago
most of that patch is removing whole files; the interesting parts are
boxModel.js and the makefiles (maybe some more REQUIRES lines can be culled)

todo: check wsm-colorpicker.js, whack inIFileSearch and other search fu, check
dom.js/domNode.js for unused code, check makefile dependencies.

Alex: is there any chance you could test the above patch, to make sure it
doesn't break stuff? (just applying the js/xul/dtd portions is fine; the
idl/cpp/h/makefile changes are just removing stuff, so that's irrelevant for
testing).
(Reporter)

Updated

16 years ago
Attachment #126121 - Flags: review?(caillon)
Error: this.getSubjectComputedStyle is not a function
Source File: chrome://inspector/content/viewers/boxModel/boxModel.js
Line: 192

I get this error for the margin, border, and padding entries.

dwitte: looking at the Box Model view after your changes, I don't think we go
far enough.  We might as well dispense of the menulist and show all five sets
(position, dimension, margin, border, and padding) simultaneously.

I'd give a conditional r+, with restoring that function from your patch and the
filing of a bug, dependent on this one, for removing the menulist as well.  But
also wait for caillon's r+ as well.
The boxmodel images, including borders, was useful, although I never understood
why that had to go through screen grabs.
Useful perhaps, but it's been broken for ages and when it doesn't crash or
otherwise be borked, it only works on one platform.  I heard someone was working
on it, but that was over a month ago and I haven't seen any progress anywhere
about it.  If someone steps up and produces a useful/non-sucky implementation
that is cross-platform, I will consider this for re-addition.
(Reporter)

Comment 6

16 years ago
fixes the boxmodel horkage (adds back a function, which was living in the wrong
place and got deleted); and removes a couple more makefile lines.

(this applies on top of prev patch)
(Reporter)

Updated

16 years ago
Attachment #126180 - Flags: review?(caillon)
And wouldn't this be potentially more useful as a independent component and not
part of the DOM inspector? I.e. a component that can be instantiated from
wherever and can be used to render any DOM node into a PNG file...
Comment on attachment 126121 [details] [diff] [review]
patch v1 (remove inIPNGEncoder and inIBitmap*) (checked in)

r=caillon, conditional on attachment 126180 [details] [diff] [review] landing simultaneously as this.
Attachment #126121 - Flags: review?(caillon) → review+
Comment on attachment 126180 [details] [diff] [review]
fix bustage (applies on top of v1)

Not going to explicitly r= this, since it is just a supplement to the other
patch.	This patch does not have r= by itself since it should not get checked
in by itself, but the other patch does not have r= without this patch.	So
technically r=caillon here, it's just not going to get marked n stuff ;-)
Attachment #126180 - Flags: review?(caillon)
Comment on attachment 126121 [details] [diff] [review]
patch v1 (remove inIPNGEncoder and inIBitmap*) (checked in)

sr=jst
Attachment #126121 - Flags: superreview?(jst) → superreview+
(Reporter)

Comment 11

16 years ago
Comment on attachment 126121 [details] [diff] [review]
patch v1 (remove inIPNGEncoder and inIBitmap*) (checked in)

checked in.

Not going to explicitly mark the second patch as such, since it is just a
supplement to the first
patch. While the second patch cannot be checked in by itself, the first patch
could not be checked in without the second patch. So
technically the second patch was checked in, it's just not going to get marked.

I'll leave this bug open for the search fu. :)
Attachment #126121 - Attachment description: patch v1 (remove inIPNGEncoder and inIBitmap*) → patch v1 (remove inIPNGEncoder and inIBitmap*) (checked in)
Hold up. why is this stuff being removed? Does Joe know about this? 

Comment 13

16 years ago
It seems somewhat lame to yank a component that many people find very useful,
just because it is buggy or doesn't work at all on some platforms.  This is the
kind of feature that is often described as "that would be awesome! if only it
worked".  So I think it should be a priority for interested parties to make it
work, instead of just bailing out and removing it.

What?! Don't look at me!
Except there are no interested parties; this has been broken for over a year and
it just confuses people now.  As I said earlier, I would not object to it going
back in if someone comes up with something that works.

Comment 15

16 years ago
Uzilla, LLC has contracted the Mozdev Group to work on this.  It's a back burner
project for them, but we are committed to making it happen.  See
http://www.uzilla.net/uzilla/blog/2003/05/22/index.cfm

I agree with Caillon at this point -- no reason to keep non-working (it's not
just buggy in my experience) stuff in the UI.  

Progress to date includes:  inScreenCapturer.cpp line 194 depth was not getting
proper native color depth. Changed macro COLORRES to BITSPIXEL and color depth
is now correct.

Comment 16

16 years ago
Sorry for the spam, track progress on this (as a standalone component) at
http://uzilla.mozdev.org/screencapture.html
Product: Core → Other Applications
(Assignee)

Comment 17

12 years ago
Um, this bug safe to close then?

Comment 18

12 years ago
(In reply to comment #17)
> Um, this bug safe to close then?

See comment #17, the "inIFileSearch and related search fu" is still remaining. It looks like there is no consumers of inIFileSearch interface in the codebase for instance.

(Assignee)

Comment 19

12 years ago
Looks that way to me as well.  I'd say that we can safely remove that then and get  this bug closed.  Patch coming.
(Assignee)

Comment 20

12 years ago
Posted patch removal patch v1.0 (obsolete) — Splinter Review
I cannot, for the life of me, figure out how to remove files for a patch, but this takes them out of the build process.  Either someone needs to tell me how to go about removing them for a patch, or whoever commits them will need to remove them.
Assignee: dwitte → comrade693+bmo
Status: NEW → ASSIGNED
Attachment #252190 - Flags: review?(db48x)

Comment 21

12 years ago
Apparently the whole mozilla/extensions/inspector/resources/content/search/ directory looks like to be dead code too. I didn't understand exactly what all this was trying to do (looks like unfinished). I couldn't find the bug associated to the commit done at the time (if there was one):

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2001-11-14+20%3A00&maxdate=2001-11-14+20%3A10&cvsroot=%2Fcvsroot
(Assignee)

Comment 22

12 years ago
(In reply to comment #21)
> Apparently the whole mozilla/extensions/inspector/resources/content/search/
> directory looks like to be dead code too. I didn't understand exactly what all
> this was trying to do (looks like unfinished). I couldn't find the bug
> associated to the commit done at the time (if there was one):

You know, that doesn't surprise me to be honest.  I was planning on doing an audit of the entire extension's code over my spring break next month.

(Assignee)

Updated

12 years ago
Attachment #252190 - Flags: review?(db48x)
(Assignee)

Comment 23

12 years ago
This actually compiles.  As for removing the search code in the extensions/inspector/resources/content/search/, I'd like to leave that for another bug.
Attachment #252190 - Attachment is obsolete: true
Attachment #252346 - Flags: superreview?(neil)
Attachment #252346 - Flags: review?(db48x)

Updated

12 years ago
Attachment #252346 - Flags: superreview?(neil) → superreview+
Comment on attachment 252346 [details] [diff] [review]
removal patch v1.1

r=db48x
Attachment #252346 - Flags: review?(db48x) → review+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
Landed attachment 252346 [details] [diff] [review]. Not sure if this should be marked fixed now.

Checking in extensions/inspector/resources/content/search/inSearchUtils.js;
/cvsroot/mozilla/extensions/inspector/resources/content/search/inSearchUtils.js,v  <--  inSearchUtils.js
new revision: 1.7; previous revision: 1.6
done
Checking in layout/build/nsLayoutModule.cpp;
/cvsroot/mozilla/layout/build/nsLayoutModule.cpp,v  <--  nsLayoutModule.cpp
new revision: 1.169; previous revision: 1.168
done
Checking in layout/inspector/public/Makefile.in;
/cvsroot/mozilla/layout/inspector/public/Makefile.in,v  <--  Makefile.in
new revision: 1.8; previous revision: 1.7
done
Removing layout/inspector/public/inIFileSearch.idl;
/cvsroot/mozilla/layout/inspector/public/inIFileSearch.idl,v  <--  inIFileSearch.idl
new revision: delete; previous revision: 1.6
done
Checking in layout/inspector/src/Makefile.in;
/cvsroot/mozilla/layout/inspector/src/Makefile.in,v  <--  Makefile.in
new revision: 1.31; previous revision: 1.30
done
Removing layout/inspector/src/inFileSearch.cpp;
/cvsroot/mozilla/layout/inspector/src/inFileSearch.cpp,v  <--  inFileSearch.cpp
new revision: delete; previous revision: 1.18
done
Removing layout/inspector/src/inFileSearch.h;
/cvsroot/mozilla/layout/inspector/src/inFileSearch.h,v  <--  inFileSearch.h
new revision: delete; previous revision: 1.12
done
Whiteboard: [checkin needed]
(Assignee)

Comment 26

12 years ago
(In reply to comment #25)
> Landed attachment 252346 [details] [diff] [review]. Not sure if this should be marked fixed now.

As per Comment #18, I think it's all set.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 27

12 years ago
Let me clarify. In the "inIFileSearch and related search fu" which was remaining, your last patch removed the inIFileSearch component, but the "related search fu" javascript glue still remains.

In particular, I suspect than building an inspector without the directory:

mozilla/extensions/inspector/resources/content/search/

would still work properly. Maybe you will be able to confirm this after the code audit you wanted to do.
(Assignee)

Comment 28

12 years ago
(In reply to comment #27)
> would still work properly. Maybe you will be able to confirm this after the
> code audit you wanted to do.

Yeah, I am aware that there is a decent amount of dead code that is packaged, and I plan to go over that and get rid of all the dead code in one swoop.

Comment 29

12 years ago
ok fine. Do you have another bug open for that work, or would you prefer reopening this one?
(Assignee)

Comment 30

12 years ago
(In reply to comment #29)
> ok fine. Do you have another bug open for that work, or would you prefer
> reopening this one?
> 

I plan on making a new bug - I haven't opened one yet however.
(Assignee)

Comment 31

12 years ago
Note that a decent chunk of search code is being removed by Bug 121774 once I get an r and sr on the patch.  It looks like /search can be canned.
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.