15345 - Remove DocLoaderObservers from all implementors...

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[rpotts (gone)]

Remove the DocLoaderObserver mechanism from the WebShell.  Any code requiring
these notifications should register as a DocLoaderObserver on the corresponding
DocLoader...

Comment 1

[Warren Harris]

Travis should verify this with the new design.

Comment 2

[don]

Move to M15 for later evaluation.

Comment 3

[travis]

Actually, no can't remove this one at this time.  Mscott is scheduled to do the 
work for supporting broadcasting the nsIWebProgress messages and then we can 
start converting things over to listen to them instead of the old ones.

Comment 4

[leger]

Updating QA Contact.

Comment 5

[don]

Move to M16 for now ...

Comment 6

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 7

[Judson Valeski]

rick, mscott. is this still an issue?

Comment 8

[Scott MacGregor]

Yeah someone still needs to finish cleaning this up.

Comment 9

[Dawn Endico]

Travis no longer works here. reassigning ownerless bugs to docshell owner,
valeski for further triage.

Comment 10

[Judson Valeski]

-> potts.

Comment 11

[rpotts (gone)]

Attachment: Changes to nsWebShellWindow to remove nsIDocumentLoaderObserver interface...

Comment 12

[rpotts (gone)]

I've just attached a patch which removes the dependency on
nsIDocumentLoaderObserver from nsWebShellWindow.cpp.

Basically, I moved the code from OnEndDocumentLoad(...) into OnStateChange(...)

Comment 13

[Scott MacGregor]

Rick these changes look great and it looks like it simplified some of the logic
in the on end document load method a bit. sr=mscott

Comment 14

[rpotts (gone)]

Attachment: Better patch to nsWebShellWindow

Comment 15

[rpotts (gone)]

Attachment: patch to nsBrowserInstance which removes nsIDocumentLoaderObserver

Comment 16

[rpotts (gone)]

I've attached a patch which removes the nsIDocumentLoaderObserver from
nsBrowserWindow...

Basically, I've re-factored the code from OnStartDocumentLoad and
OnStopDocumentLoad into two helper methods - StartDocumentLoad(...) and
EndDocumentLoad(...)

Comment 17

[Scott MacGregor]

sr=mscott on rick's latest patch.

Comment 18

[Dan M]

....snzzz...wha! mmmm yes. OK, I've looked over the patch. Can't find anything 
wrong with it. I'm looking forward to a whole new batch of endload ordering 
problems, and glad to have been invited to the party. r=danm.

Comment 19

[rpotts (gone)]

I've just checked in the changes for nsWebShellWindow and nsBrowserInstance...

Now, I'm moving on to nsWebShell and nsEditorShell :-)

Comment 20

[rpotts (gone)]

Attachment: Patch to remove nsIDocumentLoaderObserver from nsEditorShell

Comment 21

[rpotts (gone)]

I've just added a patch that whacks nsEditorShell too :-)

This patch seems rather big, so here's a run down of what I did.
- Basically, I refactored the code in OnStartDocumentLoad(...) and
  OnEndDOcumentLoad(...) into 4 helper methods called StartPageLoad()
  EndPageLoad(), StartDocumentLoad() and EndDocumentLoad().
- These new methods are called at the appropriate time in OnStateChange(...)
- Also, it turns out that nsIDocumentLoaderObserver::OnProgressURLLoad(...) and
  OnStatusURLLoad(...) are *never* called!!

I've tested the patch with normal and frameset documents..  However, I haven't
tried it out with documents that have a meta-charset tag.

So far, everything seems fine.

Comment 22

[rpotts (gone)]

I've changed the summary to better reflect the scope of this bug...

Comment 23

[rpotts (gone)]

Attachment: Patch to remove nsIDocumentLoaderObserver from nsWebShell...

Comment 24

[rpotts (gone)]

I've just attached a patch that removes the nsWebShell as a
nsIDocumentLoaderObserver...  Here's a summary of the changes:
- Added EndPageLoad(...) helper method to nsDocShell.

- Moved "document done" work in nsDocShell from OnStateChange(...) into
  EndPageLoad(...)

- Moved mDocLoaderObserver notifications in nsWebShell into a termporary
  implementation of OnStateChange(...).  This code can be deleted once
  all of the callers on SetDocLoaderObserver have been fixed!

- Moved the rest of the OnEndDocumentLoad(... )code in nsWebShell into
  EndPageLoad(...)

- Tried to cleanup tabbing and curly-braces as I went :-)

- Remember, OnStatusURLLoad(...) and OnProgressURLLoad(...) are never called,
  so I ignored them in OnStateChange(...) :-)

Comment 25

[Scott MacGregor]

sr=mscott for both the editor shell and webshell changes. Can you make sure
simon or someone else on the editor team scans through the editor shell changes
too just to be safe?

Comment 26

[rpotts (gone)]

hey simon,

are you around to review the nsEditorShell patches?

-- rick

Comment 27

[Simon Fraser [no longer active]]

I need to do some testing with them before I give an r=

Comment 28

[Simon Fraser [no longer active]]

The nsEditorShell changes test out fine (loading framesets shows alert, changing 
charsets before editing works). r=sfraser

Comment 29

[rpotts (gone)]

Just checked in the nsEditorShell changes...

Comment 30

[rpotts (gone)]

Just checked in the changes to nsDocShell and nsWebShell...

Comment 31

[Dawn Endico]

travis is no longer @netscape.com
moving to docshell component and reassigning qa contact

Comment 32

[Oliver Klee]

Milestone 0.8 has been released. We should either resolve this bug or update its
milestone.

Comment 33

[Péter Bajusz]

Clearing very old milestone (M16) in hope of reevaluation.

Comment 34

[Judson Valeski]

-> valeski

Comment 35

[Judson Valeski]

Attachment: removal of nsIDocumentLoadObserver from ActiveX control.

Comment 36

[Judson Valeski]

security has a dead weight member var using it.

Index: src/nsSecureBrowserUIImpl.h
===================================================================
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsSecureBrowserUIImpl.h,v
retrieving revision 1.7
diff -u -r1.7 nsSecureBrowserUIImpl.h
--- nsSecureBrowserUIImpl.h     2001/04/01 19:38:12     1.7
+++ nsSecureBrowserUIImpl.h     2001/04/25 23:34:13
@@ -30,7 +30,6 @@
 #include "nsXPIDLString.h"
 #include "nsString.h"
 #include "nsIObserver.h"
-#include "nsIDocumentLoaderObserver.h"
 #include "nsIDOMElement.h"
 #include "nsIDOMWindowInternal.h"
 #include "nsIStringBundle.h"
@@ -70,7 +69,6 @@

   nsCOMPtr<nsIDOMWindowInternal> mWindow;
   nsCOMPtr<nsIDOMElement> mSecurityButton;
-  nsCOMPtr<nsIDocumentLoaderObserver> mOldWebShellObserver;
   nsCOMPtr<nsIStringBundle> mStringBundle;
   nsCOMPtr<nsIURI> mCurrentURI;

Comment 37

[Judson Valeski]

Attachment: removal of loader from cookies.

Comment 38

[Stephen P. Morse]

r=morse for the changes to the cookie service

Comment 39

[Adam Lock]

Can I just verify that the various nsIWebProgressListener methods are called 
exactly where nsIDocumentLoaderObserver methods used to be?

If so r=adamlock for the ActiveX changes.

Comment 40

[Judson Valeski]

Adam, I verified that the calls are made right on top of one another, for the 
document level notification see 
http://lxr.mozilla.org/mozilla/source/uriloader/base/nsDocLoader.cpp#499 . 
You'll notice that they are paired. Thanks.

Comment 41

[Judson Valeski]

Attachment: -u diffs of wallet removal of loader

Comment 42

[Judson Valeski]

Attachment: new wallet file (the diff is hard to read)

Comment 43

[Judson Valeski]

Attachment: xmlterminal diffs to remove loader

Comment 44

[Stephen P. Morse]

r=morse for the changes to the wallet service.

Comment 45

[Judson Valeski]

Attachment: diffs to remove loader from the html parser robot

Comment 46

[Judson Valeski]

Attachment: removal of un-used nsIDocumentLoaderObserver impl from viewer.

Comment 47

[Judson Valeski]

Attachment: viewer and crawler loader removal. this patch superceeds the previous viewer patch.

Comment 48

[Judson Valeski]

note: there are no uses of nsIDocumentLoaderObserver in the commercial tree.

This bug is going to address removal of nsIDocumentLoaderObservers. I've filed
78236 to address nsIDocLoader registration removal.

Comment 49

[Judson Valeski]

Attachment: updated xmlterminal diffs that incorporate reviewer's ([e-mail]) comments.

Comment 50

[Judson Valeski]

I ran the viewer/crawler regression tests
(http://www.mozilla.org/newlayout/regress.html) and w/ out combing the output,
everything seemed to be fine WRT loading start/stop notifications.

Comment 51

[Chris Waterson]

r=waterson on the nsWebCrawler.cpp stuff.

Comment 52

[Judson Valeski]

Attachment: diffs to remove nsIDocLoaderObserver from pics.

Comment 53

[Judson Valeski]

Attachment: updated pics diff.

Comment 54

[Judson Valeski]

a note on the pics diffs. pics is not part of the default build, nor does it
build if you try to build it by hand (it lacks special #include files), so, my
changes have not been compiled there.

Comment 55

[Judson Valeski]

Attachment: updated viewer/crawler patch. this is the code that the regression tests was run against. only diff between this one and the previous one is that I've made crawler support weak references (required by nsIWebProgress).

Comment 56

[Judson Valeski]

Attachment: patch to remove nsIDocLoaderObserver from mail/news

Comment 57

[rpotts (gone)]

r=rpotts for viewer/crawler patch.

Comment 58

[rpotts (gone)]

sr=rpotts for viewer/crawler patch

Comment 59

[Judson Valeski]

mscott, please r= the mail/news changes.

nsIMsgPrintEngine.idl -
* it now derrives straight from nsISupports

nsMsgPrintEngine -
* now implements nsIWebProgressListener and weakref (required by the webprogress
impl)
* moved the OnStart[End]DocLoad handling logic into the webprogresslistener
equiv states in OnStateChange.
* registers itself as a nsIWebProgressLIstener by obtaining the nsIWebProgress
iface from the dochell.

nsMessenger -
* pulled the no longer needed loaderobserver member var.
* nsIMsgStatusFeedback impl no longer impls nsIDocLoaderObserver making the
entire SetDocLoaderObserver() call dead weight.

I tested printing of a mail message on windows and it printed fine (I had to
apply hyatt printing regression patch to 77733 to get printing to work at all
first).

Comment 60

[Judson Valeski]

webcrawler and viewer diffs have been checked in.

Comment 61

[Scott MacGregor]

r=mscott for the mailnews change. assuming printing still works after your
changes =).

Comment 62

[Judson Valeski]

Attachment: final cut, tree-wide diffs which remove nsIDocumentLoaderObservers, and the docloader's use of them.

Comment 63

[Judson Valeski]

Attachment: description of the previous patch, including reviewer names.

Comment 64

[Judson Valeski]

rpotts has sr'd this.

Comment 65

[Judson Valeski]

fix is in.