Add support for returning the title and type of the active chrome window

RESOLVED FIXED in Firefox 24

Status

Testing
Marionette
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: whimboo, Assigned: mbu)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [good first bug][lang=js][mentor=mdas])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
+++ 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')
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?
Flags: needinfo?(dburns)
Whiteboard: [good first bug][lang=js][mentor=mdas]
an example of a .xul file: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/test.xul
(Reporter)

Comment 3

4 years ago
(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?
(Assignee)

Comment 4

4 years ago
hello,

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

Thx Malini for the details.
(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
Flags: needinfo?(dburns)
(Assignee)

Comment 6

4 years ago
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 ?
(Assignee)

Comment 7

4 years ago
(and i also fix the getTitle issue of course ;) )
(Assignee)

Comment 8

4 years ago
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.
Attachment #752618 - Flags: review?(mdas)
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.
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 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.
Attachment #752618 - Flags: review?(mdas)
Heh whoops, I republished my draft response
(Assignee)

Comment 13

4 years ago
Created attachment 753424 [details] [diff] [review]
new patch according to Malini review, hope it will be good
Attachment #753424 - Flags: review?(mdas)
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
Attachment #753424 - Flags: review?(mdas)
(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
(Assignee)

Comment 16

4 years ago
Created attachment 753943 [details] [diff] [review]
Add b2g option into manifest
Attachment #752618 - Attachment is obsolete: true
Attachment #753424 - Attachment is obsolete: true
Attachment #753943 - Flags: review?(mdas)
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
Attachment #753943 - Flags: review?(mdas) → review+
m-i's busted, will land when it's open
pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/291792974254
https://hg.mozilla.org/mozilla-central/rev/291792974254
Assignee: nobody → mat.bultel
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
We need to land this in v1.0.1 and mozilla-b2g18 for test harness updates, with a=test-only
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9da66738d78d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d7cb2e4b99ab
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.