139911 - Create API to run regression tests, dump frames, content etc

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Kevin McCluskey (gone)]

Replace viewer with a plugin which can be installed into any Gecko layout
engine. The plugin will provide basic functions for dumping the frame model and
comparing frame models. The control logic for cycling through test pages will be
provided by a web page which contains JavaScript which makes calls to the
plugin. The plugin should also provide calls for dumping the frame, content, and
view tree's to provide a complete replacement for viewer

Comment 1

[Christine Hoffman]

What's the progress on this? 

Comment 2

[Johnny Stenback (:jst)]

We're talking about an XPCOM component here and not a 4x style plugin, right?

Comment 3

[dcone (gone)]

Yes.. the debugging part is a service, access to this sevice is thru a plugin or
XUL.  The plugin and service are both finished if you have a debug build.  I am
going to start working on getting this working for release builds now.  You can
find all the source and build this in Mozilla/extensions/layout-debug. 
Currently the way to build this is to go into that directory and do an nmake -f
makefile.win and then go into the plugin folder and do an nmake -f makefile.win.
 I will combine these to in a day or so.  I also need to make a gmake build for
linux and mac which I have not done yet.  I will get to those within this week.

Comment 4

[Christine Hoffman]

Don - what's the latest ETA on this - to be in the commerical build?

Comment 5

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 6

[Markus Hübner]

great thing - eager to contribute!

Comment 7

[dcone (gone)]

Attachment: Cleanup of the current plugin code. Really simplifies.

Comment 8

[rods (gone)]

Comment on attachment 99978 [details] [diff] [review]
Cleanup of the current plugin code.  Really simplifies.

r=rods

Comment 9

[Markus Hübner]

Can we get this into for 1.3b ?

Comment 10

[Kevin McCluskey (gone)]

-> peterl

Comment 11

[Simon Fraser [no longer active]]

Taking. This isn't going to be a plugin.

Comment 12

[Simon Fraser [no longer active]]

Attachment: WIP patch, extends nsIDebugObject

Comment 13

[Simon Fraser [no longer active]]

Attachment: Some html/js for layout/tools/tests that drive nsIDebugObject

Comment 14

[Bernd]

Simon, does that also include the printing regression tests? I will send out a
beer if you fix that in a way  that those linux printing guys who love to break
those tests so frequently will be stopped.

Comment 15

[Simon Fraser [no longer active]]

I haven't done anything specific to printing tests. Would it be possible to get
layout to dump frame models for printing output? I'm not sure.

Comment 16

[Simon Fraser [no longer active]]

Attachment: Patch fills out nsDebugObject impl

This patch fleshes out nsIDebugObject and its impl to add methods for dumping
frames, content, views, and webshells for a given nsIDOMWindow.

I also changed the impl so that passing a null nsIFile destination will dump
data to stdout.

Comment 17

[Simon Fraser [no longer active]]

Patch is ready for r/sr stamping.

I decided that keeping this code in a separate component was a good thing, to
avoid adding debug-only dependencies to the layout module.

Comment 18

[chris hofmann]

any ideas on the footprint win?

Comment 19

[Kevin McCluskey (gone)]

Once this lands we will no longer have to link against the native implementation
for nsLabel, nsButton, nsCheckButton, nsTextWidget on Linux (bug 17027). I'm not
sure what the fooprint reduction will be, it depends on how big the GTK
implementation of these are. I assume nsTextWidget will provide the largest
reduction.

Comment 20

[Peter Lubczynski]

Comment on attachment 112215 [details] [diff] [review]
Patch fills out nsDebugObject impl

Does changing the browser.underline_anchors pref cause a reframe or does it
reload the document's DOM? Can you note that in a comment.

Is nsIDebugObject::createDirectory really needed since nsIFile can do that?

Comment 21

[Simon Fraser [no longer active]]

> Does changing the browser.underline_anchors pref cause a reframe or does it
> reload the document's DOM? Can you note that in a comment.

It changes some style rules and does a reflow.

> Is nsIDebugObject::createDirectory really needed since nsIFile can do that?

Not really, but it was there before so I didn't remove it. I can.

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 112215 [details] [diff] [review]
Patch fills out nsDebugObject impl

>Index: idl/nsIDebugObject.idl
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/layout-debug/idl/nsIDebugObject.idl,v
>retrieving revision 1.6
>diff -b -u -8 -r1.6 nsIDebugObject.idl
>--- idl/nsIDebugObject.idl	4 Dec 2002 22:11:35 -0000	1.6
>+++ idl/nsIDebugObject.idl	22 Jan 2003 00:20:29 -0000
>@@ -34,44 +34,71 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the NPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
> 
>+interface nsIDOMWindow;
>+interface nsIFile;
>+
> [scriptable, uuid(1B4CD090-0531-11d6-A876-00105A183419)]
> interface nsIDebugObject : nsISupports
> {
> 
>   /**
>-   * creates a directory.. only locally
>-   * @param 
>+   * creates a local directory
>+   * @param aDirectoryPath a full path to the directory (not a URL)
>    * @param 

Could you remove these empty @param comments? (there are others too)


>+  void dumpRegressionData(in nsIDOMWindow aWindowToDump,in nsIFile aFile, in unsigned long aFlagsMask, [retval] out long aResult);

can't this just return a long?

> 
> %{ C++
> 
> %}

Remove this useless cruft at the end.

>Index: src/nsDebugObject.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/layout-debug/src/nsDebugObject.cpp,v
>retrieving revision 1.10
>diff -b -u -8 -r1.10 nsDebugObject.cpp
>--- src/nsDebugObject.cpp	8 Jan 2003 20:10:25 -0000	1.10
>+++ src/nsDebugObject.cpp	22 Jan 2003 00:20:30 -0000
>@@ -42,36 +42,44 @@
> 
> #include "nsXPIDLString.h"
> #include "nsReadableUtils.h"
> #include "nsIFileSpec.h"
> #include "nsIWindowWatcher.h"
> #include "nsVoidArray.h"
> #include "prmem.h"
> #include "nsIDocShellTreeItem.h"
>+#include "nsIDocShellTreeNode.h"
> #include "nsIDOMWindowInternal.h"
> #include "nsIPresShell.h"
>+#include "nsIDocument.h"
> #include "nsIDOMDocument.h"
> #include "nsIURI.h"
> #include "nsIDOMHTMLDocument.h"
> #include "nsISimpleEnumerator.h"
> #include "nsIScriptGlobalObject.h"
> #include "nsIDocShell.h"
> #include "nsIFrameDebug.h"
> #include "nsIFrame.h"
> #include "nsStyleStruct.h"
> #include "nsIFrameUtil.h"
> #include "nsLayoutCID.h"
> #include "nsNetUtil.h"
> #include "nsIFile.h"
>+#include "nsIPrefService.h"
>+#include "nsIViewManager.h"
>+#include "nsIView.h"
>+#include "nsIStyleSet.h"
>+
> 
> NS_IMPL_ISUPPORTS1(nsDebugObject, nsIDebugObject)
> 
> static NS_DEFINE_IID(kFrameUtilCID, NS_FRAME_UTIL_CID);

can you fix this to be NS_DEFINE_CID while you're here?

> /** ---------------------------------------------------
>  *  See documentation in nsDebugObject.h
>  *	@update 5/16/02 dwc
>  */
> NS_IMETHODIMP
>-nsDebugObject::OutputTextToFile(PRBool aNewFile, const PRUnichar *aFilePath, const PRUnichar *aFileName, const PRUnichar *aOutputString) 
>+nsDebugObject::OutputTextToFile(nsIFile *aFile, PRBool aTruncateFile, const char *aOutputString) 
> {
>-  nsresult      result = NS_ERROR_FAILURE;
>-  nsCAutoString outputPath;
>-  outputPath.AssignWithConversion(aFilePath);
>-  outputPath.AppendWithConversion(aFileName);
>-  char* filePath = ToNewCString(outputPath);
>+  NS_ENSURE_ARG(aFile);
>+  NS_ENSURE_ARG(aOutputString);
>+  
>+  const char* options = (aTruncateFile) ? "wt" : "at";
>   FILE* fp;
>   
>-  if ( aNewFile ) {
>-    fp = fopen(filePath, "wt");
>-  } else {
>-    fp = fopen(filePath, "at");
>-  }
>+  nsCOMPtr<nsILocalFile> localFile(do_QueryInterface(aFile));
>+  if (!localFile) return NS_ERROR_NO_INTERFACE;

Could this function just take a nsILocalFile* as an argument instead of
nsIFile* ?  Same applies to the rest of the Dump* functions.

>+/* void dumpContent (in nsIDOMWindow aWindow, in nsIFile aDestFile); */
>+NS_IMETHODIMP
>+nsDebugObject::DumpContent(nsIDOMWindow *aWindow, nsIFile *aDestFile)
>+{
>+  NS_ENSURE_ARG(aWindow);
>+
>+  nsCOMPtr<nsIDocShell> docShell;
>+  nsresult rv = GetDocShellFromWindow(aWindow, getter_AddRefs(docShell));
>+  if (NS_FAILED(rv)) return rv;
>+
>+  FILE* fp = stdout;
>+  if (aDestFile)
>+  {
>+    nsCOMPtr<nsILocalFile> localFile(do_QueryInterface(aDestFile));
>+    if (!localFile) return NS_ERROR_NO_INTERFACE;
>+    rv = localFile->OpenANSIFileDesc("w", &fp);
>+    if (NS_FAILED(rv)) return rv;
>         }

indenting looks off.

>+/* void dumpReflowStats (in nsIDOMWindow aWindow, in nsIFile aDestFile); */
>+NS_IMETHODIMP
>+nsDebugObject::DumpReflowStats(nsIDOMWindow *aWindow, nsIFile* /* aDestFile */)
>+{
>+  NS_ENSURE_ARG(aWindow);
>+
>+  nsCOMPtr<nsIPresShell> presShell;
>+  nsresult rv = GetPresShellFromWindow(aWindow, getter_AddRefs(presShell));
>+  if (NS_FAILED(rv)) return rv;
>+
>+#ifdef MOZ_REFLOW_PERF
>+  presShell->DumpReflows();
>+#else
>+  fprintf(stdout, "***********************************\n");
>+  fprintf(stdout, "Sorry, you have built with MOZ_REFLOW_PERF=1\n");

should this be _haven't_ built?

>+  fprintf(stdout, "***********************************\n");
>+#endif
>+
>+  return  NS_OK;
> }
> 
> /** ---------------------------------------------------
>  *  See documentation in nsDebugObject.h
>  *	@update 5/16/02 dwc
>  */
> NS_IMETHODIMP
>-nsDebugObject::CompareFrameModels(const PRUnichar *aBasePath, const PRUnichar *aVerifyPath,
>-            const PRUnichar *aBaseLineFileName, const PRUnichar *aVerifyFileName, PRUint32 aFlags) 
>+nsDebugObject::CompareFrameModels(nsIFile *aBaseFile, nsIFile *aVerFile, PRUint32 aFlags, PRInt32 *aResult) 
> {
...
>+  FILE* verFile;
>+  rv = localVer->OpenANSIFileDesc("r", &verFile);
>+  if (NS_FAILED(rv)) {
>+    fclose(baseFile);
>+    return rv;
>     }

indenting here.

>+nsresult
>+nsDebugObject::RefreshAllWindows()
>+{
>+  nsresult rv;
>+  // hack. Toggle the underline links pref to get stuff to redisplay
>+  nsCOMPtr<nsIPrefBranch> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  if (prefBranch)
>+  {
>+    PRBool underlineLinksPref;
>+    rv = prefBranch->GetBoolPref("browser.underline_anchors", &underlineLinksPref);
>+    if (NS_SUCCEEDED(rv))
>+    {
>+      prefBranch->SetBoolPref("browser.underline_anchors", !underlineLinksPref);
>+      prefBranch->SetBoolPref("browser.underline_anchors", underlineLinksPref);
>+    }
>+  }
>+  return NS_OK;

Maybe call nsIPresContext::ClearStyleDataAndReflow() instead? (that's what it
does in response to this pref, more or less).

>+}
>+
>+
>+nsresult
>+nsDebugObject::GetDocShellFromWindow(nsIDOMWindow* inWindow, nsIDocShell** outShell)
>+{
>+  nsCOMPtr<nsIDOMWindowInternal> theInternWindow = do_QueryInterface(inWindow);
>+  if (!theInternWindow)
>+    return NS_ERROR_FAILURE;
>+  
>+  nsCOMPtr<nsIScriptGlobalObject> scriptObj(do_QueryInterface(theInternWindow));
>+  if (!scriptObj) return NS_ERROR_FAILURE;

Why not just QI directly to nsIScriptGlobalObject?

Comment 23

[Simon Fraser [no longer active]]

Attachment: New patch; addresses comments, renames module and API

This patch addresses the above comments, other than bryner's suggestion to use
a method on nsIPresContext (rather than flipping prefs) to get the windows to
refresh. Doing that would require enumerating all open windows.

I removed a bunch of unimplemented methods related to printing on
nsIDebugOject.

This patch also renames nsIDebugObject to nsIFrameDebugObject, and changes the
module name from 'debug' to 'layout_debug'.

Comment 24

[chris hofmann]

it would be nice to get this before 1.4alpha if possible...  what are the chances?

Comment 25

[Simon Fraser [no longer active]]

Attachment: Revised version of last patch, changed to use nsILocalFiles in the API

Comment 26

[Simon Fraser [no longer active]]

Attachment: Final answer: idl methods changed to return long, rather than using [retval

Comment 27

[Simon Fraser [no longer active]]

Attachment: Final answer: idl methods changed to return long, rather than using [retval]

Comment 28

[Simon Fraser [no longer active]]

Checked in, and old debug plugin files removed.

Comment 29

[Boris Zbarsky [:bzbarsky]]

So how does this affect the process of running regression tests, if at all?

Comment 30

[Simon Fraser [no longer active]]

I'll post some docs on mozilla.org, and reference here.

Comment 31

[Boris Zbarsky [:bzbarsky]]

See the links to such instructions in http://www.mozilla.org/newlayout/doc/ --
you may want to put whatever you put up in the same dir and link it from there....