Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 8 years ago

The b2g code for the client and harness already got removed. What we should do is the same for the server component.

	Andreas Tolfsen ❲:ato❳
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8864491 [details]
Bug 1318351 - Remove readyStateTimer which was only used for B2G.

https://reviewboard.mozilla.org/r/136160/#review140514

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8864492 [details]
Bug 1318351 - Remove B2G related assertions.

https://reviewboard.mozilla.org/r/136162/#review140516

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8864493 [details]
Bug 1318351 - Remove obsolete B2G code from listener frame script.

https://reviewboard.mozilla.org/r/136164/#review140520

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8864494 [details]
Bug 1318351 - Ensure that window ids are internally handled as numbers.

https://reviewboard.mozilla.org/r/136166/#review140522

::: testing/marionette/driver.js:1441
(Diff revision 2)
> +  // Window IDs are internally handled as numbers
> +  let switchTo = cmd.parameters.name * 1;
> +  if (isNaN(switchTo)) {
> +    switchTo = cmd.parameters.name;
> +  }

I wonder if we could not use strings internally?  Ideally we want to change the window IDs to be UUIDs in the future.

::: testing/marionette/driver.js:2613
(Diff revision 2)
>  
>    if (this.mm != globalMessageManager) {
>      this.mm.removeDelayedFrameScript(FRAME_SCRIPT);
>    }
>  
> -  return this.curBrowser.closeTab().then(() => this.windowHandles);
> +  return this.curBrowser.closeTab().then(() => this.windowHandles.map(String));

It would be great if we had a test for this.

::: testing/marionette/driver.js:2652
(Diff revision 2)
>  
>    if (this.mm != globalMessageManager) {
>      this.mm.removeDelayedFrameScript(FRAME_SCRIPT);
>    }
>  
> -  return this.curBrowser.closeWindow().then(() => this.chromeWindowHandles);
> +  return this.curBrowser.closeWindow().then(() => this.chromeWindowHandles.map(String));

It would be great if you could also add a test for this.

::: testing/marionette/driver.js:3429
(Diff revision 2)
>    let id = win.QueryInterface(Ci.nsIInterfaceRequestor)
>        .getInterface(Ci.nsIDOMWindowUtils)
>        .outerWindowID;
>  
> -  return id.toString();
> +  return id;

Just return directly.

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8864495 [details]
Bug 1318351 - Remove mainContent and related code used by B2G.

https://reviewboard.mozilla.org/r/136168/#review140528

This is a great improvement to testing/marionette/browser.js.  Thanks for doing this!

::: testing/marionette/listener.js
(Diff revision 2)
> -function emitTouchEventForIFrame(message) {
> -  message = message.json;
> -  let identifier = legacyactions.nextTouchId;
> -
> -  let domWindowUtils = curContainer.frame.
> -    QueryInterface(Components.interfaces.nsIInterfaceRequestor).
> -    getInterface(Components.interfaces.nsIDOMWindowUtils);
> -  var ratio = domWindowUtils.screenPixelsPerCSSPixel;
> -
> -  var typeForUtils;
> -  switch (message.type) {
> -    case 'touchstart':
> -      typeForUtils = domWindowUtils.TOUCH_CONTACT;
> -      break;
> -    case 'touchend':
> -      typeForUtils = domWindowUtils.TOUCH_REMOVE;
> -      break;
> -    case 'touchcancel':
> -      typeForUtils = domWindowUtils.TOUCH_CANCEL;
> -      break;
> -    case 'touchmove':
> -      typeForUtils = domWindowUtils.TOUCH_CONTACT;
> -      break;
> -  }
> -  domWindowUtils.sendNativeTouchPoint(identifier, typeForUtils,
> -    Math.round(message.screenX * ratio), Math.round(message.screenY * ratio),
> -    message.force, 90);
> -}

I guess removing this does not cause any tests to fail?

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 7 years ago

Talked to whimboo on IRC, and we agreed to store window handles as integers internally still, add type tests for the commands that now return strings, and move to use strings as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1311041.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

Comment on attachment 8864495 [details]
Bug 1318351 - Remove mainContent and related code used by B2G.

https://reviewboard.mozilla.org/r/136168/#review140528

> I guess removing this does not cause any tests to fail?

No. There is only one caller of this method which was behind an if condition for b2g. So nothing else uses it at the moment, and as such should be save to remove.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 18 • 7 years ago

Andreas, please have a look at the latest changes again.

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8864494 [details]
Bug 1318351 - Ensure that window ids are internally handled as numbers.

https://reviewboard.mozilla.org/r/136166/#review141040

::: testing/marionette/driver.js:1442
(Diff revision 2)
> +  let switchTo = cmd.parameters.name * 1;
> +  if (isNaN(switchTo)) {
> +    switchTo = cmd.parameters.name;
> +  }

Since there is not atoi function in JS, you should probably use parseInt(cmd.parameters.name).  This returns NaN if the input cannot be parsed, so the isNaN(switchTo) test can be kept.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 20 • 7 years ago
mozreview-review-reply

Comment on attachment 8864494 [details]
Bug 1318351 - Ensure that window ids are internally handled as numbers.

https://reviewboard.mozilla.org/r/136166/#review141040

> Since there is not atoi function in JS, you should probably use parseInt(cmd.parameters.name).  This returns NaN if the input cannot be parsed, so the isNaN(switchTo) test can be kept.

parseInt() is 99% slower then just a "*1" as seen when run the tests via jsperf.org.

	Andreas Tolfsen ❲:ato❳
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8864494 [details]
Bug 1318351 - Ensure that window ids are internally handled as numbers.

https://reviewboard.mozilla.org/r/136166/#review141046

Tests look good.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_handles_chrome.py:30
(Diff revision 3)
> +        try:
> +            self.assertIsInstance(self.marionette.current_chrome_window_handle, types.StringTypes)
> +            self.assertIsInstance(self.marionette.current_window_handle, types.StringTypes)
> +        except errors.NoSuchWindowException:
> +            pass

So the idea here is that one of them will fail?  You’re aware that if :31 throws, :32 will not be run?  I’m guessing this is fine?

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 7 years ago
mozreview-review-reply

Comment on attachment 8864494 [details]
Bug 1318351 - Ensure that window ids are internally handled as numbers.

https://reviewboard.mozilla.org/r/136166/#review141040

> parseInt() is 99% slower then just a "*1" as seen when run the tests via jsperf.org.

parseInt handles a few corner cases that right-hand-side type coercion does not.  Vitally null * 1 produces 0, whereas parseInt(null) returns NaN.  This is problematic since the input to this function may be null due to a client programming error.

It’s unfortunate that parseInt has performance penalties, but this will go away once I get time to work on the remoteness bug in Marionette.  With that we will switch to use string UUIDs for window IDs, and no type coercion will be necessary.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 25 • 7 years ago
mozreview-review-reply

Comment on attachment 8864494 [details]
Bug 1318351 - Ensure that window ids are internally handled as numbers.

https://reviewboard.mozilla.org/r/136166/#review141040

> parseInt handles a few corner cases that right-hand-side type coercion does not.  Vitally null * 1 produces 0, whereas parseInt(null) returns NaN.  This is problematic since the input to this function may be null due to a client programming error.
> 
> It’s unfortunate that parseInt has performance penalties, but this will go away once I get time to work on the remoteness bug in Marionette.  With that we will switch to use string UUIDs for window IDs, and no type coercion will be necessary.

Makes sense. Thank you for info. It will be fixed in my next update.

	Pulsebot
	Comment 31 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4aa517cee854
Remove readyStateTimer which was only used for B2G. r=ato
https://hg.mozilla.org/integration/autoland/rev/31fdc1fc528e
Remove B2G related assertions. r=ato
https://hg.mozilla.org/integration/autoland/rev/281fa6a43aef
Remove obsolete B2G code from listener frame script. r=ato
https://hg.mozilla.org/integration/autoland/rev/cfa7a86e0b8d
Ensure that window ids are internally handled as numbers. r=ato
https://hg.mozilla.org/integration/autoland/rev/f2e0943a6762
Remove mainContent and related code used by B2G. r=ato

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 32 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4aa517cee854
https://hg.mozilla.org/mozilla-central/rev/31fdc1fc528e
https://hg.mozilla.org/mozilla-central/rev/281fa6a43aef
https://hg.mozilla.org/mozilla-central/rev/cfa7a86e0b8d
https://hg.mozilla.org/mozilla-central/rev/f2e0943a6762