120622 - [FIX]Clean up Print Interfaces & Implement Page Navigation for PrintPreview

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Attachment: patch (k drive)

This patch does the following:

1) Removes the arg of nsIDOMWindow in the nsIWebBrowserPrint calls
2) Adds a couple of new methods and an attr to nsIWebBrowserPrint (navigate, is
frameset, & exitPP)
3) Removes all but one method from nsIContentViewerFile.idl/h the remaining
call is for print regression testing
4) Removes the "static" implementation of nsIContentViewerFile.h
5) Fixed up nsIContentViewerFile.idl and turned it back on so it is now
generating the header file
6) Removed all uses of nsIContentViewerFile.h except for the WebCrawler (uses
it for Printing Regression testing)
7) nsDocumentViewer.cpp now implements nsIWebBrowserPrint.idl this makes it
easier to add new print functionality in one place
8) You can now ask an instance of the ContentViewer for a nsIWebBrowserPrint to
do printing (it retruns the nsIWebBrowserPrint interface implemented by the
nsDocumentViewer)
9) Anybody who was using nsIContentViewerFile to print will now use
nsIWebBrowserPrint
10) You can now do a "GetInterface()" on a GlobalWindow for a
nsIWebBrowserPrint
11) The browser UI now uses the GetInterface on the GlobalWindow to get a
nsIWebBrowserPrint object to do printing and this can be used for all printing
functionality

Comment 2

[Bill Law]

Rod, that patch doesn't seem to match your description.  Or is it just me? 
E.g., there's no .idl files patched, no mention of nsIContentViewerFile.

Comment 3

[rods (gone)]

Comment on attachment 66140 [details] [diff] [review]
patch (k drive)

I somehow uploaded the wrong patch (sorry)

Comment 4

[rods (gone)]

Attachment: correct patch (k drive)

(See comments from last patch)

Comment 5

[Adam Lock]

r=adamlock for docshell/webshell/embedding changes.

I have some nits:

1. Our other public interfaces tend to use the THIS_IS_A_CONSTANT notation
rather than kThisIsAConstant as seen in nsIWebBrowserPrint.idl
2. Predominant style in embedding/browser/webBrowser/nsWebBrowser.cpp is 4 space
indents.
3. nsWebBrowser::GetWebBrowserPrint doesn't do a pointer check on the
aWebBrowserPrint
4. mozilla/mailnews/base/src/nsMessenger.cpp is #include'ing nsIWebShell.h again,
we're trying to slowly kill that interface so if it can be removed again...

Comment 6

[rods (gone)]

adam, I fixed items 1-3, item 4 the nsIWebShell didn't creep back in, it is
being used in several places. Also, Printing in the DocumentViewer uses it
extensively.

Comment 7

[dcone (gone)]

r=dcone

Comment 8

[Bill Law]

(1) This doesn't look right, near the top of
DocumentViewerImpl::PrintPreviewNavigate:
+  scrollableView->ScrollTo(0, 20000, PR_TRUE);
If this does something magic, it seems like it deserves a comment.

(2) This code:
+  PRInt32     numDocs = mPrt->mPrintDocList->Count();
+  PRUnichar** array   = (PRUnichar**) nsMemory::Alloc(numDocs *
sizeof(PRUnichar*));
+  if (!array) 
+    return NS_ERROR_OUT_OF_MEMORY;
+  
+  for (PRInt32 i=0;i<mPrt->mPrintDocList->Count();i++) {
could save calling mPrt->mPrintDocList->Count() the second time (just use numDocs).

(3) This code doesn't look right:
+    // Use the URL if the doc is empty
+    if (docURLStr != nsnull && nsCRT::strlen(docURLStr) > 0) {
+      nsMemory::Free(docTitleStr);
+      docTitleStr = docURLStr;
+      docURLStr = nsnull;
+    } else {
+      nsMemory::Free(docURLStr);
+    }
+    
+    if (docURLStr == nsnull || (docURLStr != nsnull && nsCRT::strlen(docURLStr)
== 0)) {
+      CleanupDocTitleArray(array, i);
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+    array[i] = docURLStr;

Shouldn't the first if condition be:
if (!docTitleStr || !*docTitleStr)
and the second be the same?  This test could be moved inside the previous one,
which will save evaluating it again, if docTitleStr isn't empty.  And the
assignment should be from docTitleStr, right?

(4) This comment seems to having some trailing verbiage that doesn't apply:
+   * This exists PrintPreview mode and returns the current
+   * @return void
+   */
+  void exitPrintPreview();

Comment 9

[rods (gone)]

Thanks, Bill

1) The 20000, I was using that line for testing for testing and forgot to 
   take it out before my patch.
2) Oops
3) Probably some of the worst coding I have done in years  (Would anyone believe I
   purposely put it in there to see if anyone was actually doing the review?, Ok
   maybe not, but it was worth a try)
4) Fixed

Comment 10

[Bill Law]

re: (3)

That's OK, because when I review, I just look through the code quickly to try to
spot the stuff people have put in to see if we're really reviewing :-).

Comment 11

[(not reading, please use [e-mail] instead)]

r=sspitzer on the mailnews stuff, except please remove the "inner" test for
mPrintSettings.  it will always evaluate to true:

if (!mPrintSettings) {
  if (mPrintSettings == nsnull) {
  }
}

from:

+              if (!mPrintSettings) 
+              {
                 nsCOMPtr<nsIPrintOptions> printService =
do_GetService(kPrintOptionsCID, &rv);
+                if (NS_SUCCEEDED(rv)) 
+                {
+                  if (mPrintSettings == nsnull) 
+                  {                    
printService->CreatePrintSettings(getter_AddRefs(mPrintSettings));
                   }
                   NS_ASSERTION(mPrintSettings, "You can't PrintPreview without
a PrintSettings!");
                 }
               }

Comment 12

[rods (gone)]

Attachment: patch

This is pretty much the same as the last patch except that nsWebBrowser.cpp no
longer implements the nsIWebBrowserPrint interface. You can do a GetInterface
on that object to get the nsIWebBrowserPrint  which is implemented by
nsDocumentViewer

Comment 13

[Adam Lock]

Rod, shouldn't we be using do_GetInterface instead of code like this:

nsCOMPtr<nsIInterfaceRequestor> ifaceReq = do_QueryInterface(mWebBrowser);
NS_ASSERTION(ifaceReq, "Must impl nsIInterfaceRequestor");
nsCOMPtr<nsIWebBrowserPrint> print;
ifaceReq->GetInterface(NS_GET_IID(nsIWebBrowserPrint), getter_AddRefs(print));

Otherwise r=adamlock

Comment 14

[rods (gone)]

I forgot about that macro. Thanks, I will change them over.

Comment 15

[Chris Waterson]

Caveat: I don't really understand the design here, so hopefully it's right.
Maybe you can explain it to me sometime.


This is bad practice, because it holds services past XPCOM shutdown. Use a naked
pointer and a shutdown listener instead:

+  static nsCOMPtr<nsIPrintSettings> gGlobalPrintSettings;
+


This is really overkill. QI will leave |sqf| untouched if the QI fails:

+    nsIPageSequenceFrame * sqf = nsnull;
+    if (NS_SUCCEEDED(CallQueryInterface(rootFrame, &sqf)) && sqf) {



I'm confused. Are you using the in-flow list or the sibling list to chain these
together? (I.e., why don't you call currentPage->NextInFlow in the for
PRINTPREVIEW_NEXT_PAGE?)

+  if (aType == nsIWebBrowserPrint::PRINTPREVIEW_PREV_PAGE) {
+    if (currentPage) {
+      currentPage->GetPrevInFlow(&fndPageFrame);
+      if (!fndPageFrame) {
+        return NS_OK;
+      }
+    } else {
+      return NS_OK;
+    }
+  } else if (aType == nsIWebBrowserPrint::PRINTPREVIEW_NEXT_PAGE) {
+    if (currentPage) {
+      currentPage->GetNextSibling(&fndPageFrame);
+      if (!fndPageFrame) {
+        return NS_OK;
+      }
+    } else {
+      return NS_OK;
+    }



Why are you #if 0'ing out this code? Just remove it: CVS will remember it for
you, I promise.

@@ -4828,6 +5002,7 @@
 
 #ifdef NS_PRINT_PREVIEW
 
+#if 0
   // if we are printing another URL, then exit
   // the reason we check here is because this method can be called while 
   // another is still in here (the printing dialog is a good example).


This is zany. Why not hoist the |if (printSettings)| to the outermost condition
and simplify the logic?

+  // if they don't pass in a PrintSettings, then make one
+  // it will have all the default values
+  printSettings = aPrintSettings;
+  nsCOMPtr<nsIPrintOptions> printOptions = do_GetService(kPrintOptionsCID, &rv);
+  if (NS_SUCCEEDED(rv)) {
+    // if they don't pass in a PrintSettings, then make one
+    if (printSettings == nsnull) {
+      printOptions->CreatePrintSettings(getter_AddRefs(printSettings));
+    }
+    NS_ASSERTION(printSettings, "You can't PrintPreview without a PrintSettings!");
+  }
+  if (printSettings) printSettings->SetPrintSilent(aSilent);

You're sanity-checking aCount and aResult twice! NS_ENSURE_ARG already tests
|aCount != nsnull|, etc.

+  NS_ENSURE_ARG(aCount);
+  NS_ENSURE_ARG_POINTER(aResult);
+
+  if (aCount) 
+    *aCount = 0;
+  else 
+    return NS_ERROR_NULL_POINTER;
+  
+  if (aResult) 
+    *aResult = nsnull;
+  else 
+    return NS_ERROR_NULL_POINTER;


I think we leak the docURLStr here in the case that docTitleStr is non-null:

+    GetWebShellTitleAndURL(po->mWebShell, nsnull, &docTitleStr, &docURLStr);
+
+    // Use the URL if the doc is empty
+    if (!docTitleStr || !*docTitleStr) {
+      if (docURLStr && nsCRT::strlen(docURLStr) > 0) {
+        nsMemory::Free(docTitleStr);
+        docTitleStr = docURLStr;
+        docURLStr   = nsnull;
+      } else {
+        nsMemory::Free(docURLStr);
+      }
+      if (!docTitleStr || !*docTitleStr) {
+        CleanupDocTitleArray(array, i);
+        return NS_ERROR_OUT_OF_MEMORY;
+      }
+    }
+    array[i] = docTitleStr;


Wow, this is really bad! (But I see you're just copying the pattern here.) We'll
transiently keep have a dangling reference to the viewer that we'll addref
later! Better hope we're not the only one with this reference, or else this'll
crash.

This whole method is implemented poorly, and should be re-written.

+    }
+  }
+  else if (aIID.Equals(NS_GET_IID(nsIWebBrowserPrint))) {
+    if (mDocShell) {
+      nsCOMPtr<nsIContentViewer> viewer;
+      mDocShell->GetContentViewer(getter_AddRefs(viewer));
+      if (viewer) {
+        nsCOMPtr<nsIWebBrowserPrint> webBrowserPrint(do_QueryInterface(viewer));
+        *aSink = webBrowserPrint;
+      }


How about just doing:

  return CallQueryInterface(viewer, aSink);

+               nsCOMPtr<nsIWebBrowserPrint>
webBrowserPrint(do_QueryInterface(viewer));
+               nsIWebBrowserPrint* print =
(nsIWebBrowserPrint*)webBrowserPrint.get();
+               NS_ASSERTION(print, "This MUST support this interface!");
+               NS_ADDREF(print);
+               *aSink = print;
+               return NS_OK;


Isn't there a templatized do_GetInterface function that you could use here?
(This happens in several places.)

-
  nsCOMPtr<nsIWebBrowserPrint> print(do_GetInterface(mWebBrowser));
+    nsCOMPtr<nsIWebBrowserPrint> print;
+    mWebBrowser->GetInterface(NS_GET_IID(nsIWebBrowserPrint),
getter_AddRefs(print));



Krazy Tabs! Redundant NS_SUCCEEDED(rv) check -- pointers will be null if the
call fails, so just test those. Possible to use do_GetInterface here to simplify?

+  nsCOMPtr<nsIInterfaceRequestor> ifaceReq = do_QueryInterface(mWebBrowser);
+  NS_ASSERTION(ifaceReq, "Must impl nsIInterfaceRequestor");
+  nsCOMPtr<nsIWebBrowserPrint> print;
+  ifaceReq->GetInterface(NS_GET_IID(nsIWebBrowserPrint), getter_AddRefs(print));
+
if(print)
+
{
+    CPrintProgressDialog  dlg(mWebBrowser, m_PrintSettings);
+
+
  nsCOMPtr<nsIURI> currentURI;
+
  nsresult rv = mWebNav->GetCurrentURI(getter_AddRefs(currentURI));
+    if(NS_SUCCEEDED(rv) || currentURI) 
+    {
+
    nsXPIDLCString path;
+
    currentURI->GetPath(getter_Copies(path));
+      dlg.SetURI(path.get());
+    }
+    m_bCurrentlyPrinting = TRUE;
+    dlg.DoModal();
+    m_bCurrentlyPrinting = FALSE;


Why are you commenting out this code?

@@ -2192,7 +2192,7 @@
   {
     nsCOMPtr<nsIContentViewerFile> viewerFile = do_QueryInterface(viewer);
     if (viewerFile) {
-      viewerFile->PrintPreview(nsnull);
+      //viewerFile->PrintPreview(nsnull);
     }
   }
 }
@@ -2207,7 +2207,7 @@
   {
     nsCOMPtr<nsIContentViewerFile> viewerFile = do_QueryInterface(viewer);
     if (viewerFile) {
-      viewerFile->Print(PR_FALSE, nsnull, (nsIWebProgressListener*)nsnull);
+      //viewerFile->Print(PR_FALSE, nsnull, (nsIWebProgressListener*)nsnull);
     }
   }
 }

Comment 16

[rods (gone)]

Attachment: patch for just GetInterface changes for nsDocShell and nsGlobalWindow

Here is a patch showing the changes to the GetInterface method for DocShell and
GlobalWindow

Comment 17

[rods (gone)]

Chris, you reviewed an old patch, but some of your comments are still valid.

>>This is bad practice, because it holds services past XPCOM shutdown. Use a naked
>>pointer and a shutdown listener instead:
>>
>>+  static nsCOMPtr<nsIPrintSettings> gGlobalPrintSettings;
>>+

I fix that.

>>This is really overkill. QI will leave |sqf| untouched if the QI fails:
>>
>>+    nsIPageSequenceFrame * sqf = nsnull;
>>+    if (NS_SUCCEEDED(CallQueryInterface(rootFrame, &sqf)) && sqf) {

Fixed

+  } else if (aType == nsIWebBrowserPrint::PRINTPREVIEW_NEXT_PAGE) {
+    if (currentPage) {
+      currentPage->GetNextSibling(&fndPageFrame);

Oops, fixed

>>Why are you #if 0'ing out this code? Just remove it: CVS will remember it for
>>you, I promise.

>>@@ -4828,6 +5002,7 @@
>> 
>> #ifdef NS_PRINT_PREVIEW
 
>>+#if 0
>>   // if we are printing another URL, then exit
>>   // the reason we check here is because this method can be called while 
>>   // another is still in here (the printing dialog is a good example).

I need to leave this in but I fogot to to comment it:

#ifdef NS_PRINT_PREVIEW

  // Use the "else" case when the toolbar UI for PrintPreview is checked in
  // and remove "if" part
#if 1
  // if we are printing another URL, then exit


>>This is zany. Why not hoist the |if (printSettings)| to the outermost condition
>>and simplify the logic?
>>
Here is the current code:

  mPrt->mPrintSettings = aPrintSettings;
  mPrt->mPrintOptions  = do_GetService(kPrintOptionsCID, &rv);
  if (NS_SUCCEEDED(rv) && mPrt->mPrintOptions) {
    // if they don't pass in a PrintSettings, then make one
    if (mPrt->mPrintSettings == nsnull) {
     
mPrt->mPrintOptions->CreatePrintSettings(getter_AddRefs(mPrt->mPrintSettings));
    }
    NS_ASSERTION(mPrt->mPrintSettings, "You can't PrintPreview without a
PrintSettings!");

    // Get the default printer name and set it into the PrintSettings
    if (NS_FAILED(CheckForPrinters(mPrt->mPrintOptions, mPrt->mPrintSettings,
NS_ERROR_GFX_PRINTER_PRINTPREVIEW, PR_FALSE))) {
      delete mPrt;
      mPrt = nsnull;
      return NS_ERROR_FAILURE;
    }
  }

No, actually it's not zany. But it is missing a good comment.
You have to have both a PrintOptions and a PrintSetting to call CheckForPrinters. 
The user can pass in a null PrintSettings, but you can only create one if you
have a PrintOptions.

So we we might as check to if we have a PrintOptions first, because we can't do
anything below without it
then inside we check to se if the printSettings is null to know if we need to
create on.

>>You're sanity-checking aCount and aResult twice! NS_ENSURE_ARG already tests
>>|aCount != nsnull|, etc.
>>
>>+  NS_ENSURE_ARG(aCount);
>>+  NS_ENSURE_ARG_POINTER(aResult);

Code I copied.... Fixed (I should have looked at it closer)


>>I think we leak the docURLStr here in the case that docTitleStr is non-null

Fixed.

>>Wow, this is really bad! (But I see you're just copying the pattern here.) We'll
>>transiently keep have a dangling reference to the viewer that we'll addref
>>later! Better hope we're not the only one with this reference, or else this'll
>>crash.

Yes, I caught this when I was doing my final debugging and forgot to put a
comment in the bug. I tried finding where I got the code from

Fixed.


All the rest of the comments are no longer vaild brcause they were changed in
the lastest patch.

Comment 18

[Chris Waterson]

I reviewed the last patch that was attached to the bug (attachment 67107 [details] [diff] [review]),
didn't I? Was that not what you were planning to check in? Thanks for fixing up
nsGlobalWindow, I know it wasn't your mess to start with! Anyway, if you
wouldn't mind attaching the whole patch just so I could take one more look
that'd be great.

Comment 19

[rods (gone)]

Attachment: final patch (hopefully)

I solved the static comptr for global PrintSettings by adding a new readonly
attr in the nsIPrintOptions. It now owns the global PS object. I should done it
that in the first place.

Comment 20

[Chris Waterson]

Looks good, thanks for addressing the comments!

I'm still not sure whether it's right to be using the `next-sibling' pointer or
the `next-in-flow' pointer to get the next page frame:
DocumentViewerImpl::PrintPreviewNavigate uses next-sibling to count out pages,
but then prev-in-flow/next-in-flow to navigate back/forwards a single page. The
point I was trying to make in comment 15 was that next-in-flow is not the same
as next-sibling in the block and line code.

sr=waterson, but it may make sense to revisit the above issue (as another bug,
if need be)

Comment 21

[rods (gone)]

fixed.

Comment 22

[rods (gone)]

Oops, really fixed