1202381 - Remove null check on element id in Switch To Frame

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

As mwargers pointed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1158219#c9, the null check in Switch To Frame is no longer necessary because bug 1158219 has been deployed.

We can now remove this check:

   https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#1645

Comment 1

[mxm.gilbert]

Hi, I'm currently building the source code of Firefox and I'm interested to contribute. This can be maybe a good start in order to browse in the tree files and understand a little bit better how it's works. Is it this part : "cmd.parameters.id == null" line 1650 that I need to remove ? Thanks for your answer have a nice weekend

Comment 2

[Andreas Tolfsen ❲:ato❳]

(In reply to mxm.gilbert from comment #1)
> Is it this part :
> "cmd.parameters.id == null" line 1650 that I need to remove ? Thanks for
> your answer have a nice weekend

That looks about right.

An updated link to the source code: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#1654

Comment 3

[Stanislas Daniel Claude Dolcini]

Here is the diff for the fix I'm sorry if you wanted to work on it [e-mail] but since there hasn't been any update in 9 days

diff -r d56c816b35c3 testing/marionette/driver.js
--- a/testing/marionette/driver.js	Mon Oct 05 08:28:25 2015 -0700
+++ b/testing/marionette/driver.js	Tue Oct 13 10:12:22 2015 +0100
@@ -1652,8 +1652,7 @@
     let foundFrame = null;
 
     // Bug 1158219: Python client sends id when it shouldn't,
-    // but we know that if it's null it wants us to switch to default
-    if (cmd.parameters.id == null && !cmd.parameters.hasOwnProperty("element")) {
+    if (!cmd.parameters.hasOwnProperty("element")) {
       this.curFrame = null;
       if (cmd.parameters.focus) {
         this.mainFrame.focus();

         this.mainFrame.focus();

Comment 4

[Stanislas Daniel Claude Dolcini]

Here is the diff for the fix I'm sorry if you wanted to work on it [e-mail] but since there hasn't been any update in 9 days

diff -r d56c816b35c3 testing/marionette/driver.js
--- a/testing/marionette/driver.js	Mon Oct 05 08:28:25 2015 -0700
+++ b/testing/marionette/driver.js	Tue Oct 13 10:12:22 2015 +0100
@@ -1652,8 +1652,7 @@
     let foundFrame = null;
 
     // Bug 1158219: Python client sends id when it shouldn't,
-    // but we know that if it's null it wants us to switch to default
-    if (cmd.parameters.id == null && !cmd.parameters.hasOwnProperty("element")) {
+    if (!cmd.parameters.hasOwnProperty("element")) {
       this.curFrame = null;
       if (cmd.parameters.focus) {
         this.mainFrame.focus();

Comment 5

[Andreas Tolfsen ❲:ato❳]

dolcinis: Thanks for the revised diff!  I can commit this as-is, or if you want your name on the patch you need to upload a .patch file (which you can generate using `hg export bug_1202381.patch`.

Comment 6

[Stanislas Daniel Claude Dolcini]

Attachment: bug_1202381.patch

Will that do ? I'm not sure about the review flag if it should be '?' '+' or '-' cause the tooltip gives the same description.

Thanks for your fast reply.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8673279 [details] [diff] [review]
bug_1202381.patch

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

As a peer reviewer, I set this to r+ when I deem it good enough.

Comment 8

[Stanislas Daniel Claude Dolcini]

So is it all good now ? Or do I have to do something extra ? :)

Comment 9

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b6b4142077

(In reply to Stanislas Daniel Claude Dolcini from comment #8)
> So is it all good now ? Or do I have to do something extra ? :)

Just sit back and let the tests and integration do their job (-:

Comment 10

[Stanislas Daniel Claude Dolcini]

Great, can you assign it to me ? :)

Comment 11

[David Burns :automatedtester]

I just looked at this patch and there are failing tests. Please have a look at the link in comment 9

Comment 12

[Stanislas Daniel Claude Dolcini]

I don't really understand. The task is to remove the unneeded check. Do I have to fix the tests for that check too ? Cause I obviously don't know how to do that...

Comment 13

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1202381: Remove null check on element id

Bug 1202381: Remove null check on element id

Corrects type checks on parameters passed to command, indentation level,
and clarifies when the code leaps into content space.

Thanks to Stanislas Daniel Claude Dolcini for doing the first iteration
fix for this.

r=dburns

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8684985 [details]
MozReview Request: Bug 1202381: Remove null check on element id

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24683/diff/1-2/

Comment 15

[Andreas Tolfsen ❲:ato❳]

Please excuse my late reply to this, but I have been travelling.

(In reply to Stanislas Daniel Claude Dolcini from comment #12)
> I don't really understand. The task is to remove the unneeded check. Do I
> have to fix the tests for that check too ? Cause I obviously don't know how
> to do that...

I had another look at this and it appears to be more complicated than I first thought.

GeckoDriver#switchToFrame follows a very unnatural progression whereby it first checks the negativity of the id- and element parameters, then goes on to check element, then id, after which it falls through to what is meant to return an error if none of the above succeed.  It is further complicated by type checks on the id parameter, and the fact that we have two separate code paths depending on whether we're in content- or chrome space.

From what I can tell by closer examination the id check this bug suggests removing is necessary because of this "reverse flow" pattern.  Ideally they would be flipped around so that we fall through to the non-element and non-id case.

I've taken the time to rectify some of the assumptions and correct the indentation to make the code _a bit_ clearer, but this is still rather horrible and we should probably abstract all this away to a "frame manager" component.

The patch I just posted incorporates the patch from Stanislas Daniel Claude Dolcini, who is thanked in the commit message.

Again I'm sorry that I somewhat hijacked this bug, but it turns out the solution was much more complicated that I initially thought.

Comment 16

[David Burns :automatedtester]

Comment on attachment 8684985 [details]
MozReview Request: Bug 1202381: Remove null check on element id

https://reviewboard.mozilla.org/r/24683/#review22247

::: testing/marionette/driver.js:1742
(Diff revision 1)
> -      throw new NoSuchFrameError(
> +      throw new NoSuchFrameError("Unable to locate frame: " + id);

why not use string templating here like we had before?

Comment 17

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24683/#review22247

> why not use string templating here like we had before?

No particular reason other than I wanted to contract this to a single line.  We can still use the template string for that.

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8684985 [details]
MozReview Request: Bug 1202381: Remove null check on element id

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24683/diff/1-2/

Comment 19

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24683/#review22247

> No particular reason other than I wanted to contract this to a single line.  We can still use the template string for that.

Addressed that in revision 2.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed4a12fe9c82

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed4a12fe9c82