u279076 Reporter
	Description • 14 years ago

ERROR:
waitFor([object Proxy],"TabView is still open.")@resource://mozmill/modules/utils.js:449 @:0 

All of our Panorama tests have been failing since 2011-04-01.  This is most likely due to a recent change in functionality in Firefox.

Since daily testruns do not happen on 4.0, I need to investigate there first.  This may be trunk-only.

This bug blocks landing of any new Panorama tests.

	u279076 Reporter
	Comment 1 • 14 years ago

The tests do not fail on Firefox 4.0.  This is trunk-only.

	Aaron Train [:aaronmt]
	Comment 2 • 14 years ago

This is the strangest thing, from digging in a bit and looking at the entity lookup for tabView.commandKey on tabView::close. This entity is retrieving the appropriate entity fine, which has the keycode value: 'e' - the same as entity value retrieved for tabView::open, but the controller::keypress call is not appropriately closing Tab View. Here comes the strange part, replace cmdKey with the keycode value 'E' (uppercase), and the tests now pass. 

Why is keypress not appropriately closing tabView with the retrieved keycode value?
Why will it act appropriately on a hardcoded uppercase keycode?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 3 • 14 years ago

Can one of you please give the changeset between those two builds when the regression has been started?

	Aaron Train [:aaronmt]
	Comment 4 • 14 years ago

31st->1st
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2f45c30741c5&tochange=1a89509e25e4

	Ian Gilman [:iangilman]
	Comment 5 • 14 years ago

Bug 587276 landed during that window; I'm guessing that's the cause. It's possible the tests just need to be updated to the new invocation pattern. Raymond can hopefully shed some light.

	u279076 Reporter
	Comment 6 • 14 years ago

Interesting to see that the Panorama development team uses hardcoded string values instead of DTD strings in their own tests.

http://hg.mozilla.org/mozilla-central/diff/565c588e3e51/browser/base/content/test/tabview/browser_tabview_launch.js

-    EventUtils.synthesizeKey("e", { accelKey: true, shiftKey: true }, win);
+    EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);

	u279076 Reporter
	Comment 7 • 14 years ago

The DTD still refers to lowercase 'e':
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317

Are these key values case sensitive?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 8 • 14 years ago

This is not something our test is doing wrong. It's clearly a regression in the TabView code. The key identifier as read from the DTD file is the one which has to work. And we are synthesizing the right character:

var cmdKey = utils.getEntity(this.dtds, "tabView.commandkey");
this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});

(In reply to comment #6)
> Interesting to see that the Panorama development team uses hardcoded string
> values instead of DTD strings in their own tests.

That's something which happens across the different components and that's one of the reasons why we have Mozmill tests. Only our framework is currently able to get that information from the DTD files.

> http://hg.mozilla.org/mozilla-central/diff/565c588e3e51/browser/base/content/test/tabview/browser_tabview_launch.js
> 
> -    EventUtils.synthesizeKey("e", { accelKey: true, shiftKey: true }, win);
> +    EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);

This change looks broken and conflicts with the value we currently have in the DTD file as given by Anthony above.

Moving this over to Panorama. Also it would be great to get this fixed hopefully soon. Thanks.

	Raymond Lee [:raymondlee] Assignee
	Comment 9 • 14 years ago

I've mentioned the reason why I changed the lower case e to upper case E in our test here bug 587276 comment c31.

" Furthermore, you would notice in some tests, I've changed to the letter "e" to
"E" when the key combination involves shiftKey=true.  When ctrl/cmd + shift + e
is pressed in the tabview UI, the charCode of "E" would be passed into the
white list function processBrowserKeys() because the shift button is also
pressed.  However, the charCode of "e" is passed to the white list function
when stimulating a key press using EventUtils.synthesizeKey().  That's why I've
changed the lower case letter to upper one if the key combination involves
shift key.  Another way to leave the tests and fix the problem is to add the
lower case as well as upper case charCodes to the white list function
processBrowserKeys() 

-    EventUtils.synthesizeKey("e", { accelKey: true, shiftKey: true });
+    EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true });
"

I am working on a patch to fix this.

	u279076 Reporter
	Comment 10 • 14 years ago

(In reply to comment #9)
> I am working on a patch to fix this.

Will that be done on bug 587276? If not, is there another bug? Will it be done on this bug?

	Raymond Lee [:raymondlee] Assignee
	Comment 11 • 14 years ago

(In reply to comment #10)
> (In reply to comment #9)
> > I am working on a patch to fix this.
> 
> Will that be done on bug 587276? If not, is there another bug? Will it be done
> on this bug?

Will upload the patch to this bug.

	Raymond Lee [:raymondlee] Assignee
	Comment 12 • 14 years ago

This should fix the issue.

	Raymond Lee [:raymondlee] Assignee
	Comment 13 • 14 years ago

(In reply to comment #12)
> Created attachment 524566 [details] [diff] [review]
> v1
> 
> This should fix the issue.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=15a131a369f0

	Raymond Lee [:raymondlee] Assignee
	Comment 14 • 14 years ago

Tim: We want to land this asap.  If you have time, could you give me feedback on this patch please?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 15 • 14 years ago

Christian, this regression busted all of our Mozmill tests for Panorama and even a couple more for all localized and the en-US build because we can't exit the Panorama view anymore. We would really like to see it fixed on Aurora or the causing check-in to be backed out. We are not on buildbot yet but i think for our tests the same policy should apply. We hope that we can find the best solution for it soon. Thanks.

	Tim Taubert [:ttaubert] (inactive)
	Comment 16 • 14 years ago

Comment on attachment 524566 [details] [diff] [review]
v1

Sorry for the late feedback. This is kind of a solution though we really need to refactor that bit in terms of eliminating the switch statement - we should use a hash map for quick look-up.

So this patch is quite important because MozMill tests do not run at all for Panorama. I would tend to give you a feedback+ and ask you to please open a follow-up bug to refactor this.

Let's see what Ian thinks about this when reviewing :)

	Tim Taubert [:ttaubert] (inactive)
	Comment 17 • 14 years ago

m-c has been merged to aurora:

http://hg.mozilla.org/mozilla-central/rev/a95d42642281

Seems like another 6 weeks to write a proper patch for this :)

	Raymond Lee [:raymondlee] Assignee
	Comment 18 • 14 years ago

Replaced switch blocks with hash map look-up.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 19 • 14 years ago

(In reply to comment #17)
> m-c has been merged to aurora:
>
> Seems like another 6 weeks to write a proper patch for this :)

It's not that trivial. I have talked with Christian about this issue today and as told by him he would like to see a fix even on Aurora. But I will let him comment with details here.

	Dão Gottwald [:dao]
	Comment 20 • 14 years ago

(In reply to comment #15)
> We would really like to see it fixed on Aurora or
> the causing check-in to be backed out. We are not on buildbot yet but i think
> for our tests the same policy should apply.

Mozmill tests aren't developer- but QA-managed, they're naturally arcane to developers. I don't think the same rules can apply. A backout doesn't seem justified, as this code isn't wrong per se, it's just bad test interaction. You can work around it by using toLocaleUpperCase in your test.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 21 • 14 years ago

(In reply to comment #20) 
> justified, as this code isn't wrong per se, it's just bad test interaction. You
> can work around it by using toLocaleUpperCase in your test.

Why do you think that this is bad test interaction? In our Mozmill tests we are doing it the right way by retrieving the appropriate entity from the DTD file, while mostly all the Mochitests are using a hard-coded value, which doesn't allow those tests to be run in localized builds. When you check the DTD entity for opening/closing Panorama you will see that a lower case 'e' is used for those two mentioned actions. CC'ing Axel for further l10n input.

	Dão Gottwald [:dao]
	Comment 22 • 14 years ago

(In reply to comment #21)
> Why do you think that this is bad test interaction? In our Mozmill tests we are
> doing it the right way by retrieving the appropriate entity from the DTD file,

As far as I can tell, there is no problem for users but only for the tests, so apparently the tests don't quite match reality.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 23 • 14 years ago

(In reply to comment #22)
> As far as I can tell, there is no problem for users but only for the tests, so
> apparently the tests don't quite match reality.

If that's the case EventUtils has to be fixed then.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 24 • 14 years ago

We have landed the proposed workaround for the Mozmill tests now.

	Tim Taubert [:ttaubert] (inactive)
	Comment 25 • 14 years ago

Comment on attachment 525629 [details] [diff] [review]
v2

Looks good, thanks! Nits:

>     delete this._browserKeys;
>     this._browserKeys = keys;

>+    delete this._browserKeysWithShift;
>+    this._browserKeysWithShift = keys;

I think we don't need the delete statement right before the assignment, do we? They seem superfluous.

	Raymond Lee [:raymondlee] Assignee
	Comment 26 • 14 years ago

Removed the delete statements.

	Raymond Lee [:raymondlee] Assignee
	Comment 27 • 14 years ago

Comment on attachment 526172 [details] [diff] [review]
v3

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=d1a7ab2827da

	Dão Gottwald [:dao]
	Comment 28 • 14 years ago

(In reply to comment #23)
> (In reply to comment #22)
> > As far as I can tell, there is no problem for users but only for the tests, so
> > apparently the tests don't quite match reality.
> 
> If that's the case EventUtils has to be fixed then.

Maybe. It seems possible that this is the right way to proceed here.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 29 • 14 years ago

Neil, could you please have a look at this problem or CC someone who knows the details of EventUtils and especially synthesizing shortcut events? Would it be safe to assume that we could always use an uppercase letter to synthesize those shortcuts, even when the DTD file lists it as lowercase?

	Neil Deakin
	Comment 30 • 14 years ago

The character supplied to synthesizeKey should be the uppercase letter when the shift key flag is set, usually, yes. I believe that isn't true if caps lock is enabled on Windows however as pressing shift creates a lowercase letter instead. So it may not be desirable to have this be converted automatically.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 31 • 14 years ago

(In reply to comment #30)
> The character supplied to synthesizeKey should be the uppercase letter when the
> shift key flag is set, usually, yes. I believe that isn't true if caps lock is
> enabled on Windows however as pressing shift creates a lowercase letter
> instead. So it may not be desirable to have this be converted automatically.

Would that mean the DTD file has to be updated and exchange 'e' with 'E' to fix it? Sounds like the wrong letter has been used in there.

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317

	Ian Gilman [:iangilman]
	Comment 32 • 14 years ago

Comment on attachment 526172 [details] [diff] [review]
v3

Looks good.

	Dão Gottwald [:dao]
	Comment 34 • 14 years ago

Please don't land this in the middle of the attempt to figure out whether this is the right fix.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 35 • 14 years ago

(In reply to comment #31)
> Would that mean the DTD file has to be updated and exchange 'e' with 'E' to fix
> it? Sounds like the wrong letter has been used in there.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317

Neil, could you please comment on it? Would be nice to get an idea of how to solve this bug.

Tim, I re-add bug 603789 to the dependency list, because it is the reason of the regression and has to be kept on it.

	Raymond Lee [:raymondlee] Assignee
	Comment 36 • 14 years ago

(In reply to comment #35)
> (In reply to comment #31)
> > Would that mean the DTD file has to be updated and exchange 'e' with 'E' to fix
> > it? Sounds like the wrong letter has been used in there.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317
> 
> Neil, could you please comment on it? Would be nice to get an idea of how to
> solve this bug.
> 
> Tim, I re-add bug 603789 to the dependency list, because it is the reason of
> the regression and has to be kept on it.

Hi Neil, could you please comment on it.  Thanks

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 37 • 14 years ago

Dao, how shall we proceed on this bug? Shall we move out the remaining question to a separate bug? Not sure how long it will take for Neil to answer.

	Tim Taubert [:ttaubert] (inactive)
	Comment 38 • 14 years ago

bugspam

	Tim Taubert [:ttaubert] (inactive)
	Comment 39 • 14 years ago

bugspam

	Tim Taubert [:ttaubert] (inactive)
	Comment 40 • 14 years ago

(In reply to comment #31)
> (In reply to comment #30)
> > The character supplied to synthesizeKey should be the uppercase letter when the
> > shift key flag is set, usually, yes. I believe that isn't true if caps lock is
> > enabled on Windows however as pressing shift creates a lowercase letter
> > instead. So it may not be desirable to have this be converted automatically.
> 
> Would that mean the DTD file has to be updated and exchange 'e' with 'E' to
> fix it? Sounds like the wrong letter has been used in there.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#317

Cc'ing Gavin. Hopefully he can shed some light on how to correctly fix this.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 41 • 14 years ago

The panorama key event handling code should just behave the same way the underlying XBL <key> handling code does: compare charcodes case-insensitively. 

That means lowercasing the value retrieved from the <key> elements in _setupBrowserKeys (as in nsXBLPrototypeHandler::ConstructPrototype [1], where "key" attributes are read) and the value from the event in processBrowserKeys (as in nsXBLPrototypeHandler::KeyEventMatched [2] where they're compared to an event's charCode).

[1] http://hg.mozilla.org/mozilla-central/annotate/1dfc5592f438/content/xbl/src/nsXBLPrototypeHandler.cpp#l930
[2] http://hg.mozilla.org/mozilla-central/annotate/1dfc5592f438/content/xbl/src/nsXBLPrototypeHandler.cpp#l604

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 42 • 14 years ago

I suppose that might involve having to also check shiftKey separately, but it looks like the code already does that. Duplicating XBL <key> matching logic is far from ideal. Are there followups to find a better solution?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 43 • 13 years ago

Raymond and Tim, is that enough information you need for an update of the patch?

	Raymond Lee [:raymondlee] Assignee
	Comment 44 • 13 years ago

(In reply to comment #43)
> Raymond and Tim, is that enough information you need for an update of the
> patch?

OK, updated the patch based on comment 41.

	Raymond Lee [:raymondlee] Assignee
	Comment 45 • 13 years ago

Comment on attachment 543080 [details] [diff] [review]
v5

Sent it to Try and waiting for results

	Raymond Lee [:raymondlee] Assignee
	Comment 46 • 13 years ago

Comment on attachment 543080 [details] [diff] [review]
v5

Passed try
http://tbpl.mozilla.org/?tree=Try&rev=7d06d9a7a3d8

	Tim Taubert [:ttaubert] (inactive)
	Comment 47 • 13 years ago

Comment on attachment 543080 [details] [diff] [review]
v5

Review of attachment 543080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

	Tim Taubert [:ttaubert] (inactive)
	Comment 48 • 13 years ago

Dietrich is out this week, maybe you'd want to find someone else to review in the meantime.

	Ian Gilman [:iangilman]
	Comment 49 • 13 years ago

Comment on attachment 543080 [details] [diff] [review]
v5

Review of attachment 543080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. I'm glad we've lost those switch statements. 

I don't know that we need two reviews on this... it's up to you, Dao, if you want to weigh in.

	Tim Taubert [:ttaubert] (inactive)
	Comment 50 • 13 years ago

http://hg.mozilla.org/integration/fx-team/rev/cbeda84b1a5f

	Tim Taubert [:ttaubert] (inactive)
	Comment 51 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/cbeda84b1a5f