Last Comment Bug 773159 - Add support for returning the title and type of the active chrome window
: Add support for returning the title and type of the active chrome window
Status: RESOLVED FIXED
[good first bug][lang=js][mentor=mdas]
:
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: mathieu bultel:mbu
:
Mentors:
Depends on: 754226
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-11 23:45 PDT by Henrik Skupin (:whimboo)
Modified: 2013-05-28 11:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
fixed
fixed
wontfix
fixed


Attachments
First submission for getting support of getTitle and getWindowType in actor part of marionette (9.91 KB, patch)
2013-05-22 02:10 PDT, mathieu bultel:mbu
no flags Details | Diff | Splinter Review
new patch according to Malini review, hope it will be good (9.31 KB, patch)
2013-05-23 12:39 PDT, mathieu bultel:mbu
no flags Details | Diff | Splinter Review
Add b2g option into manifest (9.38 KB, patch)
2013-05-24 13:16 PDT, mathieu bultel:mbu
malini: review+
Details | Diff | Splinter Review

Description Henrik Skupin (:whimboo) 2012-07-11 23:45:47 PDT
+++ This bug was initially created as a clone of Bug #754226 +++

It would be great if beside the page title Marionette can also return the actual title and type of the chrome window, when it's in chrome mode. In case of Mozmill we would need it a lot.

title: window.document.documentElement.getAttribute('title')
type:  window.document.documentElement.getAttribute('windowtype')
Comment 1 Malini Das [:mdas] - Away, not checking bugmail 2013-05-17 16:32:31 PDT
For setting the title:

https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-actors.js#1076

If we're in chrome context, then we'll need to get the current window (https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-actors.js#407) and return the document.documentElement.getAttribute('title') value to the client.

To test this, you'll have to write a test.xul file and add it to (mozilla-central)/testing/marionette/client/marionette/www/, and set the chrome window's title to something. To test it, write a test in (mozilla-central)/testing/marionette/client/marionette/tests/unit/ and in your test, you should switch to chrome context, load this page, and verify that the title is what you expect. You'll need to add this test to the test manifest (mozilla-central)/testing/marionette/client/marionette/tests/unit/unit-test.ini

For the window type:

Henrik, I don't think we have a method for getting the window type in webdriver. We can add it as a special function, since it requires knowledge of the current chromewindow, and it's not fun to have that in an execute_script call. dburns, thoughts?
Comment 2 Malini Das [:mdas] - Away, not checking bugmail 2013-05-17 16:37:56 PDT
an example of a .xul file: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/test.xul
Comment 3 Henrik Skupin (:whimboo) 2013-05-18 01:25:54 PDT
(In reply to Malini Das [:mdas] from comment #1) 
> Henrik, I don't think we have a method for getting the window type in
> webdriver. We can add it as a special function, since it requires knowledge
> of the current chromewindow, and it's not fun to have that in an
> execute_script call. dburns, thoughts?

Not sure I understand that. If we can retrieve the window title, what's so complicated to do the same for the window type? Similar to the title it is a DOM attribute on the window itself (see my comment 0). Can't we add a get(Window)Type() to the actor?
Comment 4 mathieu bultel:mbu 2013-05-18 01:58:45 PDT
hello,

i'll try to focus on this issue this week end.

Thx Malini for the details.
Comment 5 David Burns :automatedtester 2013-05-20 09:14:23 PDT
(In reply to Malini Das [:mdas] from comment #1)
> For setting the title:
> 
> https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> actors.js#1076
> 
> If we're in chrome context, then we'll need to get the current window
> (https://mxr.mozilla.org/mozilla-central/source/testing/marionette/
> marionette-actors.js#407) and return the
> document.documentElement.getAttribute('title') value to the client.
> 
> To test this, you'll have to write a test.xul file and add it to
> (mozilla-central)/testing/marionette/client/marionette/www/, and set the
> chrome window's title to something. To test it, write a test in
> (mozilla-central)/testing/marionette/client/marionette/tests/unit/ and in
> your test, you should switch to chrome context, load this page, and verify
> that the title is what you expect. You'll need to add this test to the test
> manifest
> (mozilla-central)/testing/marionette/client/marionette/tests/unit/unit-test.
> ini
> 
> For the window type:
> 
> Henrik, I don't think we have a method for getting the window type in
> webdriver. We can add it as a special function, since it requires knowledge
> of the current chromewindow, and it's not fun to have that in an
> execute_script call. dburns, thoughts?

That's fine by me to add a new getWindowType() to the actor. We don't need it on a listener though since this is more for Chrome automation unless there is a good use case that I cant think of
Comment 6 mathieu bultel:mbu 2013-05-21 14:42:52 PDT
I add new function getWindowType() to the actor and i also add a function get_window_type() to marionette.py which call actor function.

I'll put the patch tomorrow evening prehaps... I just want to add unit tests for getWindowType to the marionette client.

For the xul file, i update one on /testing/marionette/client/marionette/chrome/test.xul 
Is there any pb with that, or not ?
Comment 7 mathieu bultel:mbu 2013-05-21 14:49:11 PDT
(and i also fix the getTitle issue of course ;) )
Comment 8 mathieu bultel:mbu 2013-05-22 02:10:42 PDT
Created attachment 752618 [details] [diff] [review]
First submission for getting support of getTitle and getWindowType in actor part of marionette

First submission for getting support of getTitle in chrome context and getWindowType.
Comment 9 Malini Das [:mdas] - Away, not checking bugmail 2013-05-22 11:34:42 PDT
Comment on attachment 752618 [details] [diff] [review]
First submission for getting support of getTitle and getWindowType in actor part of marionette

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

Awesome work, mbu! This is looking really good and I'm glad you were able to figure out how things work so quickly!

I added a few suggestions, but it also seems that this patch can't be applied to the latest mozilla-central code cleanly. I keep getting errors when I try to apply it. Can you fix the errors in your patch? To do this, do:

hg qpop # to pop the patch
hg pull -u # to update mozilla-central to the latest commit
hg qpush # to push the patch back

and then you can resolve any conflicts, then update patch with 'hg qrefresh'.

::: testing/marionette/client/marionette/tests/unit/test_window_title.py
@@ +20,5 @@
> +        MarionetteTestCase.tearDown(self)
> +
> +    def test_get_chrome_title(self):
> +        title = self.marionette.execute_script("return window.document.documentElement.getAttribute('title');")
> +        self.assertEqual(title, self.marionette.title)

can you add a check that the title is equal to "Title Test" as well?

::: testing/marionette/client/marionette/www/testTitle.html
@@ +10,5 @@
> +</head>
> +<body>
> +  <p> Check the Title
> +</body>
> +</html>

instead of adding a new test file, you can use test.html, and use its title.
Comment 10 Malini Das [:mdas] - Away, not checking bugmail 2013-05-22 11:35:23 PDT
I'll wait for that patch before finishing my review, since I'll need it to test that everything is working when run against the phone.
Comment 11 Malini Das [:mdas] - Away, not checking bugmail 2013-05-22 11:49:35 PDT
Comment on attachment 752618 [details] [diff] [review]
First submission for getting support of getTitle and getWindowType in actor part of marionette

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

Awesome work, mbu! This is looking really good and I'm glad you were able to figure out how things work so quickly!

I added a few suggestions, but it also seems that this patch can't be applied to the latest mozilla-central code cleanly. I keep getting errors when I try to apply it. Can you fix the errors in your patch? To do this, do:

hg qpop # to pop the patch
hg pull -u # to update mozilla-central to the latest commit
hg qpush # to push the patch back

and then you can resolve any conflicts, then update patch with 'hg qrefresh'.

::: testing/marionette/client/marionette/tests/unit/test_window_title.py
@@ +20,5 @@
> +        MarionetteTestCase.tearDown(self)
> +
> +    def test_get_chrome_title(self):
> +        title = self.marionette.execute_script("return window.document.documentElement.getAttribute('title');")
> +        self.assertEqual(title, self.marionette.title)

can you add a check that the title is equal to "Title Test" as well?

::: testing/marionette/client/marionette/www/testTitle.html
@@ +10,5 @@
> +</head>
> +<body>
> +  <p> Check the Title
> +</body>
> +</html>

instead of adding a new test file, you can use test.html, and use its title.
Comment 12 Malini Das [:mdas] - Away, not checking bugmail 2013-05-22 11:50:04 PDT
Heh whoops, I republished my draft response
Comment 13 mathieu bultel:mbu 2013-05-23 12:39:47 PDT
Created attachment 753424 [details] [diff] [review]
new patch according to Malini review, hope it will be good
Comment 14 Malini Das [:mdas] - Away, not checking bugmail 2013-05-24 08:57:19 PDT
Comment on attachment 753424 [details] [diff] [review]
new patch according to Malini review, hope it will be good

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

This looks great and runs well!

I just realized that since we're testing on .xul pages, there is one thing we'll have to add. We run these tests on b2g phones/emulators, and we can't run xul on them successfully, so we'll have to skip those tests. We'll want to skip test_window_type.py completely, but for test_window_title.py, we can still run the content test with no problem. Instead of skipping the whole test, we can flag part of the test with the @skip_if_b2g decorator. I've added comments on how to do this.

Thanks!

::: testing/marionette/client/marionette/tests/unit/test_window_title.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from marionette_test import MarionetteTestCase

Can you change this to:

from marionette_test import MarionetteTestCase, skip_if_b2g

@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from marionette_test import MarionetteTestCase
> +
> +class TestTitleChrome(MarionetteTestCase):

then above this class, can you add this:

@skip_if_b2g

::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
@@ +86,5 @@
>  [test_screenshot.py]
>  [test_cookies.py]
>  b2g = false
> +[test_window_title.py]
> +[test_window_type.py]

can you flag this as

b2g = false
Comment 15 Malini Das [:mdas] - Away, not checking bugmail 2013-05-24 08:58:35 PDT
(In reply to Malini Das [:mdas] from comment #14)
> @@ +86,5 @@
> >  [test_screenshot.py]
> >  [test_cookies.py]
> >  b2g = false
> > +[test_window_title.py]
> > +[test_window_type.py]
> 
> can you flag this as
> 
> b2g = false

I meant, just flag [test_window_type.py] with b2g = false
Comment 16 mathieu bultel:mbu 2013-05-24 13:16:56 PDT
Created attachment 753943 [details] [diff] [review]
Add b2g option into manifest
Comment 17 Malini Das [:mdas] - Away, not checking bugmail 2013-05-24 13:37:55 PDT
Comment on attachment 753943 [details] [diff] [review]
Add b2g option into manifest

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

great, thanks! I'll push to m-i soon
Comment 18 Malini Das [:mdas] - Away, not checking bugmail 2013-05-24 13:46:42 PDT
m-i's busted, will land when it's open
Comment 19 Malini Das [:mdas] - Away, not checking bugmail 2013-05-27 08:26:26 PDT
pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/291792974254
Comment 20 Phil Ringnalda (:philor, back in August) 2013-05-27 18:55:59 PDT
https://hg.mozilla.org/mozilla-central/rev/291792974254
Comment 21 Malini Das [:mdas] - Away, not checking bugmail 2013-05-28 11:09:59 PDT
We need to land this in v1.0.1 and mozilla-b2g18 for test harness updates, with a=test-only

Note You need to log in before you can comment on or make changes to this bug.