Centralise creation of valid UUID in Marionette

RESOLVED FIXED in Firefox 46

Status

Testing
Marionette
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: automatedtester, Assigned: Shaif, Mentored)

Tracking

({ateam-marionette-server, ateam-marionette-task})

unspecified
mozilla46
ateam-marionette-server, ateam-marionette-task
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

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

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Currently UUID for session and WebElement is created in 2 separate places and would be good to consolodate.

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#52 and https://dxr.mozilla.org/mozilla-central/source/testing/marionette/elements.js#34 and everything they use them will need to be updated
(Reporter)

Updated

2 years ago
Keywords: ateam-marionette-server, ateam-marionette-task
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 1

2 years ago
I would like to work on this bug. Could I get some more information in terms of what needs to be done?
I would perhaps suggest creating a new function in testing/marionette/elements.js that returns a new UUID as a string.  Ideally elements.js ought to encapsulate all interaction with web elements in Marionette, and this would be a small step in that direction.

My idea would entail creating a new `element` prototype on which you attach a function:

    this.elements = {};
    elements.generateUUID = function() { /* implementation goes here */ };

You’d need to add this to the EXPORTED_SYMBOLS constant at the top of the file to expose it to consumers:

    this.EXPORTED_SYMBOLS = [
      "elements",
      …
    ];

Finally consumers would use it as such:

    Cu.import("chrome://marionette/content/elements.js");
    logger.info(elements.generateUUID());
(Assignee)

Comment 3

2 years ago
Created attachment 8702799 [details] [diff] [review]
patch1198797.diff

Please review it and tell me what are the changes required.
Attachment #8702799 - Flags: review?(ato)
Comment on attachment 8702799 [details] [diff] [review]
patch1198797.diff

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

The patch looks good.  Once you’ve fixed up the syntax error I can push this to try.  Please also make sure you include the bug number in the commit message.

::: testing/marionette/driver.js
@@ -509,5 @@
>  };
>  
>  /** Create a new session. */
>  GeckoDriver.prototype.newSession = function(cmd, resp) {
> -  let uuid = uuidGen.generateUUID().toString();

You can also remove the uuidGen constant near the top of the file now.

::: testing/marionette/elements.js
@@ +21,5 @@
>   */
>  
>  this.EXPORTED_SYMBOLS = [
>    "Accessibility",
> +  "elements"

Missing , (comma).

This will give you a syntax error.  I’m surprised this works?

@@ -279,5 @@
>          // cleanup reference to GC'd element
>          delete this.seenItems[i];
>        }
>      }
> -    let uuid = uuidGen.generateUUID().toString();

You can also remove the uuidGen constant near the top of the file now.

@@ +755,5 @@
>    },
>  }
> +
> +this.elements = {};
> +elements.generateUUID = function(){

Nit: Space before { (brace).

@@ +758,5 @@
> +this.elements = {};
> +elements.generateUUID = function(){
> +  let uuid = uuidGen.generateUUID();
> +  let uString = uuid.substring(1, uuid.length - 1);
> +  return uString;

Nit: `uString` seems excessive for a transitive variable like this.  Either return from the substring directly, or shorten it to something like `s`.
Attachment #8702799 - Flags: review?(ato)
By the way, if you’re using Mercurial you might consider using MozReview for your next patch, as it provides a nicer review tool as well as automatic landing to mozilla-inbound: http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html
Summary: Centralize creation of valid uuid in marionette → Centralize creation of valid UUID in Marionette
Assignee: nobody → chowdhuryshaif
Status: NEW → ASSIGNED
Summary: Centralize creation of valid UUID in Marionette → Centralise creation of valid UUID in Marionette
(Assignee)

Comment 6

2 years ago
(In reply to Andreas Tolfsen (:ato) from comment #4)

> ::: testing/marionette/elements.js
> @@ -279,5 @@
> >          // cleanup reference to GC'd element
> >          delete this.seenItems[i];
> >        }
> >      }
> > -    let uuid = uuidGen.generateUUID().toString();
> 
> You can also remove the uuidGen constant near the top of the file now.

according to the example here (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIUUIDGenerator) nsIUUIDGenerator is needed to generate UUID . So, should i remove the constant or leave it in element.js?
(In reply to Shaif Chowdhury:retro from comment #6)
> (In reply to Andreas Tolfsen (:ato) from comment #4)
> > You can also remove the uuidGen constant near the top of the file now.
> 
> according to the example here
> (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIUUIDGenerator) nsIUUIDGenerator is needed to generate UUID .
> So, should i remove the constant or leave it in element.js?

It appears I was a bit eager to mark this as an issue.  Of course I mean the references to uuidGen in driver.js, like you say, that are now not in use any longer.
(Assignee)

Comment 8

2 years ago
Created attachment 8702945 [details] [diff] [review]
Bug1198797.diff
Attachment #8702799 - Attachment is obsolete: true
Attachment #8702945 - Flags: review?(ato)
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94d2e3709e62&group_state=expanded
Comment on attachment 8702945 [details] [diff] [review]
Bug1198797.diff

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

Try to run your code before you submit the patch.  You can use `./mach marionette-test --gecko-log -` to check that everything works.

::: testing/marionette/elements.js
@@ +756,5 @@
>  }
> +
> +this.elements = {};
> +elements.generateUUID = function() {
> +  let uuid = uuidGen.generateUUID();

This needs to be casted to a string in order for the next substring statement to work:

    uuidGen.generateUUID().toString()
Attachment #8702945 - Flags: review?(ato)
(Assignee)

Comment 11

2 years ago
Created attachment 8703358 [details] [diff] [review]
Bug1198797.diff
Attachment #8702945 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Comment on attachment 8703358 [details] [diff] [review]
Bug1198797.diff

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

Sorry for that mistake. Please review this one.
Attachment #8703358 - Flags: review?(ato)
Created attachment 8707871 [details]
MozReview Request: Bug 1198797 - Centralise creation of UUIDs in Marionette; r=ato

Review commit: https://reviewboard.mozilla.org/r/30903/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30903/
Attachment #8707871 - Flags: review?(ato)
Comment on attachment 8707871 [details]
MozReview Request: Bug 1198797 - Centralise creation of UUIDs in Marionette; r=ato

https://reviewboard.mozilla.org/r/30903/#review27681
Attachment #8707871 - Flags: review?(ato) → review+
Sorry about the delay in looking at this patch, it slipped my mind.  I’ve done a try push now and if that’s fine I’ll land it on inbound.

I also had to correct your hg name/email.
Attachment #8703358 - Flags: review?(ato) → review+
Attachment #8703358 - Flags: review+

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c589202b1eaf
Shaif, thanks for you contribution!  And sorry it took me so long to get around to this.
(Assignee)

Comment 18

2 years ago
Thanks for the review :)

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c589202b1eaf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.