210096 - [inspector] remove bitmap/png and search functionality

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Attachment: patch v1 (remove inIPNGEncoder and inIBitmap*) (checked in)

this does the aforementioned, minus inIFileSearch. i'll do that in a separate
patch.

Comment 2

[[e-mail]]

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).

Comment 3

[Alex Vincent [:WeirdAl]]

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.

Comment 4

[Bradley Baetz (:bbaetz)]

The boxmodel images, including borders, was useful, although I never understood
why that had to go through screen grabs.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 6

[[e-mail]]

Attachment: fix bustage (applies on top of v1)

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)

Comment 7

[Johnny Stenback (:jst)]

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 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

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 ;-)

Comment 10

[Johnny Stenback (:jst)]

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

sr=jst

Comment 11

[[e-mail]]

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. :)

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Hold up. why is this stuff being removed? Does Joe know about this? 

Comment 13

[Joe Hewitt (gone)]

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!

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

[Andy Edmonds]

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

[Andy Edmonds]

Sorry for the spam, track progress on this (as a standalone component) at
http://uzilla.mozdev.org/screencapture.html

Comment 17

[Shawn Wilsher :sdwilsh]

Um, this bug safe to close then?

Comment 18

[Sylvain Pasche]

(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.

Comment 19

[Shawn Wilsher :sdwilsh]

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

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: removal patch v1.0

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.

Comment 21

[Sylvain Pasche]

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

Comment 22

[Shawn Wilsher :sdwilsh]

(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.

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: removal patch v1.1

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.

Comment 24

[Daniel Brooks [:db48x]]

Comment on attachment 252346 [details] [diff] [review]
removal patch v1.1

r=db48x

Comment 25

[Nickolay_Ponomarev]

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

Comment 26

[Shawn Wilsher :sdwilsh]

(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.

Comment 27

[Sylvain Pasche]

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.

Comment 28

[Shawn Wilsher :sdwilsh]

(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

[Sylvain Pasche]

ok fine. Do you have another bug open for that work, or would you prefer reopening this one?

Comment 30

[Shawn Wilsher :sdwilsh]

(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.

Comment 31

[Shawn Wilsher :sdwilsh]

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.