62338 - bogus dependency from contentAreaUtils.js on navigator.js

Status: VERIFIED DUPLICATE of bug 61809

(SeaMonkey :: UI Design, defect, P3)

Description

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

Recently some code and files seem to have moved out of navigator into new 
locations in xpfe/communicator. While cleanup work is appreciated, files should 
not be moved or restructured without some appreciation of the bigger picture. 

Nitpicking specific files: 

- contentAreaUtils.js has several functions (editPage, sendPage) that do NOT 
belong in a shared resource like communicator. In fact, the functionality of 
sendPage is already provided by mail/news' dynamic 
overlay 'mailNavigatorOverlay.xul' which lives in 
mailnews/base/resources/content, and is overlayed by the chrome registry into 
navigator.xul. Communicator components must not pollute the shared resource 
pools with dependencies, as it is not always guaranteed that they are 
installed. While plenty of such dependencies exist today, we do not want to 
create new ones. 

- 'contextAreaClick.js' has a function (middleMousePaste) which has a 
dependency on a function defined only in navigator.js. As such this function is 
essentially useless here. 

File placement is completely wrong. Browsing facilities are toolkit level 
services that should live in the global package, not in the form of overlays 
that force clients to suffer from byzantine include chains, but encapsulated 
widgets that provide the services one would expect from a web page display 
area. Specifically, a browser widget should provide: 

- context menus (including built-in XUL menu nodes and methods, which can be 
overridden by clients wishing to customize)
- tooltips

At a lower level (probably browserBindings.xml which is included automatically 
& is part of the embedding toolkit) basic keyboard navigation facilities and 
drag & drop.

Clients should only have to say

<browser .. />

Everything else should be automatic.  

With regard to placement, the communicator package exists only for shared 
content specific to the communicator suite. This means cross app utilities like 
taskbar/task menu etc. Communicator is NOT the place for items that are of use 
to third party applications. Global is the correct home. 

I'm copying jag as he indicated that he's developing a xul <browser/> widget, 
and much of this code and material in Navigator could be cleanly refactored 
into it.

Comment 1

[Blake Ross]

I already know all of this, and yes, I know about <browser/>.

Comment 2

[Blake Ross]

Actually, I disagree with many of your points.  I also don't understand why 
this is assigned to me.  This was all temporary until <browser/> is working.  I 
would be happy to help design it, but I'm not going to be its main 
implementor.  Is this bug to implement <browser/>?  If not, I don't understand 
its purpose.

Comment 3

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

I'm not asking you to make <browser/>. I'm asking you to collaborate with jag
(if he's still interested in working on it, and he indicated to me that he was)
in cleanly factoring the code rather than pushing it around.

I don't think it's a good idea to move code around for no real reason,
especially not into inappropriate places, regardless of what this enables you to
implement.

What are your disagreements with the direction I outlined?

I ask that you consult me before doing moves like this in the future. 

Comment 4

[Blake Ross]

It seems to me that the implementation of <browser/> is going to allow for the 
removal of these files, so I'm not sure I see a point in trying to clean them 
up until that happens.

None of this code was moved around for `no real reason'.  Navigator's content 
area context menu lived in /navigator, which was absurd.  Sidebar needed the 
same context menu, and it made MailNews pull in all of navigator.js.

I obviously didn't think /communicator was an inappropriate place for it or I 
wouldn't have put the new files there.  I thought /global was for truly generic 
utilities like the files and classes that currently reside there (nsClipboard, 
nsDragAndDrop, nsJSComponentManager, generic wizard stuff, etc...).  The 
content area didn't seem generic enough to me at the time to fit this 
description.  I guess, in hindsight, it is.

Yes, obviously the dependency on navigator.js in contentAreaClick.js was an 
oversight.  I'm pretty sure we already have another bug on that issue that 
you're cc'ed on.

Regarding the functions in contentAreaUtils.js: those are needed in sidebar, 
mailnews, composer, and navigator.  Is it better to have a dynamic overlay onto 
all of them?  I guess.  I'm not sure I see how damaging (although extraneous 
and unnecessary, I agree) it is to have these functions in contentAreaUtils.js 
even if certain apps aren't installed, since they won't be called anyways in 
that case.

I didn't touch or move tooltips.

jag, if you expressed interest in refactoring all of this, and you reviewed 
basically all of these changes in the first place...erm...why didn't you say 
anything? :)

Comment 5

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

Perceived need (sidebar) is not a reason to create a mess. I do not like the
idea of implementing features in an inappropriate fashion simply because 'things
will be fixed later'. In reality, there is no later. I'm setting higher
standards for Navigator to allow for that. 

Comment 6

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

As for the mailnews & editor code in communicator - it is irrelevant whether or
not the code is called when those components are not installed. The code should
not be present. Ideally, the FE code would be structured in a way that you could
do a mailnews only install. I understand that that is not possible currently but
I see no reason to make the situation worse. 

Comment 7

[Peter "jag" Annema]

blake, I'm constantly learning too :-) I did point out those things I really
didn't like, but I missed a few things too, and some things I just wasn't aware
of. o/'' You live, you learn o/''

So how about we closely look, with Ben's comments in mind, at some of the
changes we recently made, and come up with a "what belongs where" plan?

Comment 8

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

At this point I'd be satisfied by these browser-related files moving into
xpfe/global/resources/content/browser (make a new dir), the mail/editor related
functions being removed and some comments added to the files indicating that
they are temporary. 

Comment 9

[Blake Ross]

Okay, Ben's right (was there ever any doubt?).  I've been running into lots of 
limitations with this code lately.  I'll work with jag to clean this all up...

Comment 10

[Marcia Knous [:marcia]]

Netscape nav triage team: per Alec Flett's pre-triage recommendation, this 
bug is nsbeta1-.

Comment 11

[Blake Ross]

blah blah

Comment 12

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

Excluding FUTURE XUL <browser/> development, and fixed bugs (contentAreaUtils.js
now no longer includes matter for optional components such as mail), the
dependency on navigator.js ('readFromClipboard') is yet to be resolved. REOPEN,
resummarize to reflect remaining work necessary.  

Comment 13

[Blake Ross]

yoink.

Comment 14

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

Sorry, this bug is not INVALID, work remains to be done here. 

cc brendan

Comment 15

[Blake Ross]

*** This bug has been marked as a duplicate of 61809 ***

Comment 16

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

Thank you. 

Comment 17

[Blake Ross]

You're welcome.

Comment 18

[Blake Ross]

You're more than welcome, rather.

Comment 19

[Brendan Eich [:brendan]]

Will you two quit flirting?

Damn, why do I have to be cc'd?

/be

Comment 20

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

because we love you ;-)