Closed Bug 325100 Opened 15 years ago Closed 15 years ago

Move DOM inspector C++ components into gklayout

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Keywords: fixed1.8.1)

Attachments

(4 files, 1 obsolete file)

The DOM inspector components used for flashing and internal inspection use various internal APIs and I'd like to be able to version them with gecko instead of having to ship them with DOMI. This also means that DOMI is binary-free, which is good.

This is basically the same model we already use for venkman.
Attachment #210016 - Flags: review?(dbaron)
Attached patch inspector patch, rev. 1 (obsolete) — Splinter Review
Attachment #210017 - Flags: review?(dbaron)
Priority: -- → P2
This seems fine with me, and the patch looks OK with a quick skim, although I wonder if MOZ_XUL is the appropriate test.  But I want to give other people a chance to react as well.
Yeah, it seems that if we do the venkman model we should be using a different ifdef...

Also, a followup bug after this to simplify code (eg if this stuff is linked into layout some stuff from nsIInspectorCSSUtils can just move to nsContentUtils, etc) might be nice.

But in general looks ok to me.
I'm happy to add a separate --disable-inspector-api configure flag if that's preferable.
How about a new patch with the new configure flag, then?  That makes sense to me as well.
Attachment #210016 - Flags: review?(dbaron) → review+
Attachment #210017 - Flags: review?(dbaron) → review-
Depends on: 327153
Since cvsmoves are in some state of limbo, would you object to my simply adding the files in layout/inspector and not preserving cvsblame?
Attachment #210017 - Attachment is obsolete: true
Attachment #219320 - Flags: review?(dbaron)
Comment on attachment 219320 [details] [diff] [review]
inspector patch, rev. 2

You missed some ifdefs in nsLayoutModule.cpp (you should probably ifdef the includes, and definitely the generic factory constructors).  With that fixed, r=dbaron.
Attachment #219320 - Flags: review?(dbaron) → review+
Fixed on trunk. mconnor and I would like to pick this up for 1.8.1, but I think that can wait until after a3.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
It would have been really nice for the Inspector guys to know about this...
Er... this managed to lose the patch for bug 285204.  The version of inDOMView.cpp that's in the new location is 1.36, but the version in Attic in the old location is 1.37 (not counting the checkin that removed it)...  Could we please get this fixed before anyone checks into the files in the new location and things _really_ get screwed up with the version numbering?

It looks like the cvsmove was done on May 16, so bug 285204 seems to be the only bug affected (see http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=%2Fmozilla%2Fextensions%2Finspector&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-05-16&maxdate=&cvsroot=%2Fcvsroot).
Does this make it safe for me to submit a patch to DOMI using nsPIDOMWindow.h?
Neil, yes

bz, I'll recommit that tomorrow.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060521 Minefield/3.0a1 ID:2006052111 [cairo]

Did this break DOMi in FF (not working since the 20060520 0650pdt build) ?
I haven't seen a bug filed for it yet
Hmm, it turns out inLayoutUtils.cpp was already including nsPIDOMWindow.h ;-)
Hrm, WFM on mac, though the browser is totally nonfunctional in other ways I haven't looked into yet.

/me tries windows
DOMi shows up(chrome and all), but no content

No chrome package registered for chrome://communicator/content/tasksOverlay.xul 

Warning: Failed to load overlay from chrome://communicator/content/tasksOverlay.xul.
Source file: chrome://inspector/content/inspector.xul

Warning: reference to undefined property event.originalTarget.nodeType
Source file: chrome://inspector/content/inspector.xml
Line: 455

Error: this.mDOMView has no properties
Source file: chrome://inspector/content/viewers/dom/dom.js
Line: 76

Depends on: 338746
Blocks: 339229
Attachment #224379 - Flags: review? → review?(benjamin)
Comment on attachment 224379 [details] [diff] [review]
remove references to dead makefiles

allmakefiles.sh patches don't usually need review
Attachment #224379 - Flags: review?(benjamin) → review+
Checked in the allmakefiles.sh patch.
dbaron said he doesn't need to re-review this. mconnor, can you approve? There are file add/removes from extensions/inspector/base/* to layout/inspector/* that I'm going to do manually (can't do cvsmoves on a branch, because they're already done on trunk)
Attachment #225465 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #225465 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Fixed on 1.8 branch
Keywords: fixed1.8.1
Depends on: 342136
It looks like this left extensions/inspector/build in various states of removal on the trunk and 1.8 branch.  The extensions/inspector/Makefile.in doesn't seem to do anything in build, but there are still some files there, although build/src/ has been removed on the 1.8 branch (but not from allmakefiles.sh).
You need to log in before you can comment on or make changes to this bug.