Closed Bug 1202381 Opened 9 years ago Closed 9 years ago

Remove null check on element id in Switch To Frame

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: ato, Assigned: dolcinis, Mentored)

Details

(Keywords: pi-marionette-server, Whiteboard: [good first bug][lang=js])

Attachments

(2 files)

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
Mentor: ato
Whiteboard: [good first bug][lang=js]
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
Flags: needinfo?(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
Flags: needinfo?(ato)
Here is the diff for the fix I'm sorry if you wanted to work on it mxm.gilbert@gmail.com 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();
Here is the diff for the fix I'm sorry if you wanted to work on it mxm.gilbert@gmail.com 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();
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`.
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 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.
Attachment #8673279 - Flags: review+
So is it all good now ? Or do I have to do something extra ? :)
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 (-:
Great, can you assign it to me ? :)
Assignee: nobody → dolcinis
Status: NEW → ASSIGNED
I just looked at this patch and there are failing tests. Please have a look at the link in comment 9
Flags: needinfo?(dolcinis)
Flags: needinfo?(ato)
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...
Flags: needinfo?(dolcinis)
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 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/
Attachment #8684985 - Flags: review?(dburns)
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.
Flags: needinfo?(ato)
Attachment #8684985 - Flags: review?(dburns) → review+
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?
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 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/
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.
https://hg.mozilla.org/mozilla-central/rev/ed4a12fe9c82
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: