325100 - Move DOM inspector C++ components into gklayout

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

Attachment: inspector CVSMoves, rev. 1

Comment 2

[Benjamin Smedberg]

Attachment: inspector patch, rev. 1

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Benjamin Smedberg]

I'm happy to add a separate --disable-inspector-api configure flag if that's preferable.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How about a new patch with the new configure flag, then?  That makes sense to me as well.

Comment 7

[Benjamin Smedberg]

Attachment: inspector patch, rev. 2

Since cvsmoves are in some state of limbo, would you object to my simply adding the files in layout/inspector and not preserving cvsblame?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Alex Vincent [:WeirdAl]]

It would have been really nice for the Inspector guys to know about this...

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[[e-mail]]

Does this make it safe for me to submit a patch to DOMI using nsPIDOMWindow.h?

Comment 13

[Benjamin Smedberg]

Neil, yes

bz, I'll recommit that tomorrow.

Comment 14

[Peter van der Woude [:Peter6]]

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

Comment 15

[[e-mail]]

Hmm, it turns out inLayoutUtils.cpp was already including nsPIDOMWindow.h ;-)

Comment 16

[Benjamin Smedberg]

Hrm, WFM on mac, though the browser is totally nonfunctional in other ways I haven't looked into yet.

/me tries windows

Comment 17

[Peter van der Woude [:Peter6]]

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

Comment 18

[timeless]

Attachment: remove references to dead makefiles

Comment 19

[Benjamin Smedberg]

Comment on attachment 224379 [details] [diff] [review]
remove references to dead makefiles

allmakefiles.sh patches don't usually need review

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in the allmakefiles.sh patch.

Comment 21

[Benjamin Smedberg]

Attachment: Move DOMI binaries to layout, branch 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)

Comment 22

[Benjamin Smedberg]

Fixed on 1.8 branch

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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