Open Bug 512263 Opened 11 years ago Updated 6 years ago

Browser chrome tests for toolbar customization.

Categories

(Toolkit :: Toolbars and Toolbar Customization, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: Natch, Unassigned)

References

Details

Attachments

(1 file, 7 obsolete files)

Make more extensive tests for toolbar customizations (i.e. drag expectations, tooltips, etc.)

Looked through all the tests and it seems there aren't any for this.
Flags: in-testsuite?
Depends on: 512347
Attached patch wip 1 (obsolete) — Splinter Review
This works fine, although I'd like to get bug 512347 sorted out before this lands.
Attachment #396311 - Attachment is patch: true
Attachment #396311 - Attachment mime type: application/octet-stream → text/plain
Attached patch wip 2 (obsolete) — Splinter Review
Still having some issues with synthesizeDrop (it calls all the command functions which is why I override them), I keep on getting an error that aDragSession is null over here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/nsDragAndDrop.js#591

For some reason the drag session cannot be retrieved through the drag service, that times out the test which is why I commented that code out.

This is all after the fix in bug 512347.
Attachment #396311 - Attachment is obsolete: true
Attached patch wip 3 (obsolete) — Splinter Review
Major cleanup, stronger tests with better coverage.

I'm crashing now somewhere now, gonna have to figure that out sometime soon.
Attachment #396367 - Attachment is obsolete: true
Depends on: 512889
Attached patch patch ver. 1 (obsolete) — Splinter Review
Tests all pass now, added the "stop-button" to the test. Fixed up some scoping issues, should be good to go. Sending it to try now, will request review if that turns out well :)
Attachment #396660 - Attachment is obsolete: true
Damn try went nuts, most didn't even _run_ my test :(

I think there might be a need for a focus call though (when the second window is opened), so I'll repatch and retry.
Attached patch patch ver. 1 - split up (obsolete) — Splinter Review
Tryserver keeps on choking on this test, so I've split them up, not off to try again.
Attachment #397212 - Attachment is obsolete: true
Tryserver results:

WINNT: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1251595954.1251602450.21361.gz&fulltext=1 (passed)

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1251595954.1251602766.24807.gz&fulltext=1 (passed, other unrelated errors)

Mac: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1251595954.1251603299.30414.gz&fulltext=1 (failed)

The mac failure can only be happening if the progress listener isn't working, gonna add some dumps to find out what's going on.
Depends on: 513619
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1251672391.1251680032.22429.gz&fulltext=1

For some reason onLocationChange isn't being fired on Mac when |back-button| is clicked. Filed bug 513619.
No longer depends on: 513619
Ok, I'm a bit lost now, anyone that has a mac that can qimport this patch and run the test, then post results here would be greatly appreciated.
Keywords: helpwanted, qawanted
Whiteboard: [mac testing needed]
Natch: Are these the instructions to use qimport ->http://robarnold.org/hg-qimport-my-bugzilla-patch-redux/? I am not overly familiar with it but perhaps someone in testdev is.
Sorry, I thought I commented here on what exactly to do, must've been bug 513619...

1) download the patch as-is to $SRCDIR/.hg/patches
2) run: hg qimport -e buttonTest
3) run: hg qpush buttonTest
4) cd $OBJDIR/toolkit/content
5) run: make
6) run: TEST_PATH=toolkit/content/tests/widgets make -C $OBJDIR mochitest-plain

On mac (at least on the tryserver) the test "test_toolbarButtons.js" times out. I'm trying to figure out why.

Thanks in advance.
whoops, correction:

step 4: cd $OBJDIR/browser/base
step 5: make
step 6: TEST_PATH=browser/base/content make -C $OBJDIR mochitest-browser-chrome


I was thinking of a different test, sorry for making this confusing.
adding ctalbert to the bug for some guidance...
Any update here? Did anyone try the tests out on a Mac?

Not really all that important, but it would be nice to get a test out on these things (most _were_ regressed in the past)...
I've tried out the tests on a Mac, I'm getting these js errors prior to the test time out:

Running chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js...
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | reload-button button must have tooltip text
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | stop-button button must have tooltip text
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | home-button button must have tooltip text
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | new-tab-button button must have tooltip text
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | back-button button must have tooltip text
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | forward-button button must have tooltip text
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | back-forward-dropmarker button must have tooltip text
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | The history popup should be showing now.
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | A new tab should have been opened.
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | The stop button should be enabled now
TEST-PASS | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | The page load should stop when stop-button is clicked
TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | Console message: [JavaScript Error: "[Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemChanged]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js :: LS__setSiteURISecure :: line 803"  data: no]" {file: "file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js" line: 803}]
TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | Console message: [JavaScript Error: "[Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemChanged]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js :: LS__setSiteURISecure :: line 803"  data: no]" {file: "file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js" line: 803}]
TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | Console message: [JavaScript Error: "[Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemChanged]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js :: LLL_handleResult :: line 976"  data: no]" {file: "file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js" line: 976}]
TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | Console message: [JavaScript Error: "[Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemChanged]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js :: LLL_handleResult :: line 976"  data: no]" {file: "file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js" line: 976}]
TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | Console message: [JavaScript Error: "[Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemChanged]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js :: LLL__setResourceTTL :: line 1068"  data: no]" {file: "file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js" line: 1068}]
TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | Console message: [JavaScript Error: "[Exception... "'Container view not found' when calling method: [nsINavHistoryResultViewer::itemChanged]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js :: LLL__setResourceTTL :: line 1068"  data: no]" {file: "file:///Users/mw22/mozilla-build/mozcentral/_firefox/dist/MinefieldDebug.app/Contents/MacOS/components/nsLivemarkService.js" line: 1068}]
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_toolbarButtons.js | Timed out

I have no idea why this is happening, but I think the js errors are causing the timeout at least.
Mabye this bug list is useful:
https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=container+view+not+found&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=
Especially bug 415390 might give a little bit of explanation of what is going on.
Ok, never mind, it turns out the back button is never clicked with this function:
EventUtils.synthesizeMouse(elm, 0, 0, {});
The 0, 0 activates the history drop down instead.
When I change it into 1, 1 then the tests seem to work just fine.

Hopefully, I'll attach a patch later today with all the '0, 0'-s replaced with '1, 1'. That should be safer.
Thanks Matijn, that was very helpful. I'll attach a patch with that change, and hopefully get this pushed through.
Status: NEW → ASSIGNED
Keywords: helpwanted, qawanted
Whiteboard: [mac testing needed]
Attached patch patch ver. 1.1 (obsolete) — Splinter Review
Tests should be good with this change, sending to try now.

Once again, thanks Martijn.
Attachment #397484 - Attachment is obsolete: true
Attachment #404571 - Flags: review?(gavin.sharp)
No longer depends on: 512889
Natc, I guess you can do check-ins yourself (because you can do tryserver builds). Otherwise, you can set the checkin-needed keyword to get it checked in.
(In reply to comment #20)
I don't have commit privileges, but this is still pending review from gavin...
You can always apply for commit privileges :)

(In reply to comment #21)
> (In reply to comment #20)
> I don't have commit privileges, but this is still pending review from gavin...
(In reply to comment #22)
I'd need to find people to vouch for me first, as of now the tryserver access is what's crucial to me, and that I've got already ;)
Comment on attachment 404571 [details] [diff] [review]
patch ver. 1.1

Dao, do you have time for this? It's just some additional tests for the toolbar buttons.
Attachment #404571 - Flags: review?(gavin.sharp) → review?(dao)
Dao: review ping?
Comment on attachment 404571 [details] [diff] [review]
patch ver. 1.1

>+++ b/browser/base/content/test/browser_newWindowButton.js

>+    gPrefService.setCharPref("startup.homepage_welcome_url", "");

I don't think this makes a difference for opening new windows.

>+    this.eventTarget = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>+    this.eventTarget.addEventListener("load", this, true);
>+  },
>+
>+  handleEvent: function(aEvent) {
>+    if (aEvent.type == "load") {
>+      this.eventTarget.removeEventListener("load", arguments.callee, true);

use aEvent.currentTarget instead of this.eventTarget

>+      let count = 0;
>+      let currentWindow;
>+      while(windows.hasMoreElements()) {

while (

>+        count++;
>+        currentWindow = windows.getNext();
>+      }
>+      is(count, 2, "There should be two windows now");

Is this test actually useful?

>+      this.chromeWindow = currentWindow.QueryInterface(Ci.nsIDOMChromeWindow);
>+      let browser = this.chromeWindow.gBrowser.selectedBrowser;
>+      browser.addEventListener("pageshow", this, true);

What tells you that currentWindow is the one you just opened? Can you use aEvent.currentTarget instead?

>+    gPrefService.setCharPref("browser.startup.homepage", this.initialHome);
>+    gPrefService.setIntPref("browser.startup.page", this.initialStartupPage);

Can you use clearUserPref for this?

>+++ b/browser/base/content/test/browser_toolbarButtons.js

>+var progressListener = {
>+  QueryInterface: function(aIID) {
>+    if (aIID.equals(Ci.nsIWebProgressListener) ||
>+        aIID.equals(Ci.nsISupportsWeakReference) ||
>+        aIID.equals(Ci.nsISupports))
>+      return this;
>+    throw Components.results.NS_NOINTERFACE;
>+  },

Tabs progress listeners are called from tabbrowser.xml, no XPCOM involved.

>+  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {},
>+
>+  onProgressChange: function(aWebProgress, aRequest, aCurSelf, aMaxSelf, aCurTotal, aMaxTotal) {},

All the arguments can be left out.

>+  browserReady: function() {
>+    is(this.browser.currentURI.spec, this.expected[this._currentTestLevel],
>+         "The correct page should show when " + this.elementIds[this._currentTestLevel] + " is clicked");
>+    if (gBrowser.tabContainer.childNodes.length > 2)
>+          gBrowser.removeTab(gBrowser.tabContainer.lastChild);

the indentation is broken

>+    if (pageNumber < this.testPages.length - 1) {
>+      setTimeout(function(self) {
>+        content.location = self.testPages[++pageNumber];
>+      }, 0, this);
>+      return;
>+    }
>+    browser.removeEventListener("pageshow", this, true);
>+    setTimeout(function(self) {
>+      gBrowser.selectedBrowser.goBack();
>+      self.prepareForTests();
>+    }, 0, this);

Can you use executeSoon instead of setTimeout?

>+    let tab = gBrowser.selectedTab = gBrowser.addTab();
>+    content.location =
>+      "http://example.org/browser/browser/base/content/test/dummy_page.html";
>+    let elm = $("stop-button");
>+    is(elm.disabled, false, "The stop button should be enabled now");
>+    EventUtils.synthesizeMouse(elm, 1, 1, {});
>+    is(tab.linkedBrowser.currentURI.spec, "about:blank",
>+       "The page load should stop when stop-button is clicked");

you could probably spare the tab variable and use content.location instead of tab.linkedBrowser.currentURI.spec
Attachment #404571 - Flags: review?(dao)
Attached patch patch (obsolete) — Splinter Review
Updated to comments, besides this one:

>+    gPrefService.setCharPref("startup.homepage_welcome_url", "");

This pref is actually used in the first newly opened window in any installation, and this test triggers it. The first mochitest window overrides it, so it remains as originally set. However the "browser.startup.page" pref was not needed for the test so I removed it.
Attachment #404571 - Attachment is obsolete: true
Attachment #420022 - Flags: review?(dao)
(In reply to comment #26)
>+        count++;
>+        currentWindow = windows.getNext();
>+      }
>+      is(count, 2, "There should be two windows now");

> Is this test actually useful?

It asserts that we are indeed dealing with two windows, I can remove it if you'd like, seemed like it didn't hurt, as this test is otherwise pretty bare. I can't really add anything to it though since it opens a window which chews up a lot of the allotted mochitest time.
Comment on attachment 420022 [details] [diff] [review]
patch

>+      this.chromeWindow = aEvent.currentTarget.QueryInterface(Ci.nsIDOMChromeWindow);

No need for the QueryInterface. (And no need for this.chromeWindow.)

>+    else if (aEvent.type == "pageshow") {
>+      let browser = this.chromeWindow.gBrowser.selectedBrowser;

s/this.chromeWindow.gBrowser.selectedBrowser/aEvent.currentTarget/

>+      is(browser.currentURI.spec, "about:robots",
>+         "New windows should open the home page by default");
>+      this.chromeWindow.close();

Here you can do browser.contentWindow.close() or browser.ownerDocument.defaultView.close().
(In reply to comment #27)
> This pref is actually used in the first newly opened window in any
> installation, and this test triggers it. The first mochitest window overrides
> it

Where does this happen?

(In reply to comment #28)
> It asserts that we are indeed dealing with two windows, I can remove it if
> you'd like, seemed like it didn't hurt, as this test is otherwise pretty bare.

You got domwindowopened and a load event, I think that's enough.
Attached patch patchSplinter Review
Ok, comments addressed. Also added an executeSoon so that we don't trigger errors in tabbrowser.xml (which I believe may be another bug). The stop/reload merge caused some errors, so I had to place the stop button before the reload one so that they don't merge...
Attachment #420022 - Attachment is obsolete: true
Attachment #420458 - Flags: review?(dao)
Attachment #420022 - Flags: review?(dao)
(In reply to comment #30)
> (In reply to comment #27)
> > This pref is actually used in the first newly opened window in any
> > installation, and this test triggers it. The first mochitest window overrides
> > it
> 
> Where does this happen?

I don't know, it's my theory of why my code was originally not working, this pref is the cause, so...
> (In reply to comment #28)
> > It asserts that we are indeed dealing with two windows, I can remove it if
> > you'd like, seemed like it didn't hurt, as this test is otherwise pretty bare.
> 
> You got domwindowopened and a load event, I think that's enough.

Fair enough, removed. If the window doesn't open it will timeout, and the idea of there being an extra window doesn't really warrant a test...
(In reply to comment #32)
> (In reply to comment #30)
> > (In reply to comment #27)
> > > This pref is actually used in the first newly opened window in any
> > > installation, and this test triggers it. The first mochitest window overrides
> > > it
> > 
> > Where does this happen?
> 
> I don't know, it's my theory of why my code was originally not working, this
> pref is the cause, so...

I'd really like to understand this. Sounds like something that should be fixed centrally in the testing profile.
Well, the code that brings up this page is here: http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js?mark=525-528#515

Beyond that, I'd have to did into the mochitest code and figure out how it opens a window that doesn't get the default behavior (i.e. default home page, default first pages, larry bar, etc.), which I am not really qualified to do, so....

Notice that the harness doesn't change the default values for any of those prefs, and yet the first window is still always a blank, one-tab window...
Dao, any chance of getting this reviewed? I'm really not sure how the harness code works or where it does its deed, but I assure you the pref change is necessary (and harmless).
I just commented out the startup.homepage_welcome_url stuff and it didn't make a difference for the test...
Ok, I'll retest that and fix appropriately. I'm testing on Vista trunk.

Thanks.
Attachment #420458 - Flags: review?(dao)
Assignee: highmind63 → nobody
Status: ASSIGNED → NEW
Is this bug still relevant in an Australis world?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #38)
> Is this bug still relevant in an Australis world?

The patch is not, but the bug should still be. Australis still sports a toolbar and allows for (easier) customization. IMHO. Wish I could help, just don't have time these days...
I was assuming we've been writing tests for customization as we implemented it...
Yes, we have. At least for the bits inside browser - the toolkit bits are still largely untested, so we could leave this open for that.
Component: Toolbars and Customization → Toolbars and Toolbar Customization
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.