bogus dependency from contentAreaUtils.js on navigator.js

VERIFIED DUPLICATE of bug 61809

Status

P3
normal
VERIFIED DUPLICATE of bug 61809
18 years ago
10 years ago

People

(Reporter: bugs, Assigned: bugzilla)

Tracking

({arch})

Trunk
Future

Firefox Tracking Flags

(Not tracked)

Details

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

Comment 1

18 years ago
I already know all of this, and yes, I know about <browser/>.
(Assignee)

Comment 2

18 years ago
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.
Severity: major → normal
Priority: P3 → P4
Summary: Poorly Factored Code in new files in xpfe/communicator → Poorly factored code in new files in xpfe/communicator
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. 
(Assignee)

Comment 4

18 years ago
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? :)
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. 
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

18 years ago
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?
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. 

Updated

18 years ago
Keywords: arch, helpwanted, mozilla1.0
(Assignee)

Comment 9

18 years ago
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...
Status: NEW → ASSIGNED
Keywords: helpwanted, mozilla1.0
Priority: P4 → P1
Netscape nav triage team: per Alec Flett's pre-triage recommendation, this 
bug is nsbeta1-.
Keywords: nsbeta1-
(Assignee)

Updated

18 years ago
Priority: P1 → P3
(Assignee)

Updated

17 years ago
Target Milestone: --- → Future
(Assignee)

Comment 11

17 years ago
blah blah
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → INVALID
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.  
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: Poorly factored code in new files in xpfe/communicator → bogus dependency from contentAreaUtils.js on navigator.js
(Assignee)

Comment 13

17 years ago
yoink.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → INVALID
Sorry, this bug is not INVALID, work remains to be done here. 

cc brendan
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 15

17 years ago

*** This bug has been marked as a duplicate of 61809 ***
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → DUPLICATE
Thank you. 
(Assignee)

Comment 17

17 years ago
You're welcome.
(Assignee)

Comment 18

17 years ago
You're more than welcome, rather.
Status: RESOLVED → VERIFIED
Will you two quit flirting?

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

/be
because we love you ;-)
Product: Core → Mozilla Application Suite

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.