106.00 KB, patch
|Details | Diff | Splinter Review|
1.30 KB, patch
|Details | Diff | Splinter Review|
26.79 KB, patch
|Details | Diff | Splinter Review|
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.
this does the aforementioned, minus inIFileSearch. i'll do that in a separate patch.
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).
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.
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)
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 ;-)
16 years ago
Attachment #126121 - Flags: superreview?(jst)
Comment on attachment 126121 [details] [diff] [review] patch v1 (remove inIPNGEncoder and inIBitmap*) (checked in) sr=jst
Attachment #126121 - Flags: superreview?(jst) → superreview+
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?
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.
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.
Sorry for the spam, track progress on this (as a standalone component) at http://uzilla.mozdev.org/screencapture.html
Um, this bug safe to close then?
(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.
Looks that way to me as well. I'd say that we can safely remove that then and get this bug closed. Patch coming.
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
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
(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.
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 #252346 - Flags: superreview?(neil) → superreview+
Comment on attachment 252346 [details] [diff] [review] removal patch v1.1 r=db48x
Attachment #252346 - Flags: review?(db48x) → review+
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]
(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
(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.
ok fine. Do you have another bug open for that work, or would you prefer reopening this one?
(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.
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.
You need to log in before you can comment on or make changes to this bug.