Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

We should provide some cue in the Firefox UI that it is under automation and control by Marionette when there is an active connection.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 2 • 7 years ago

Uploading a very quick first draft using the following styling:

> #urlbar {
>  background: repeating-linear-gradient(
>   45deg,
>   transparent,
>   transparent 20px,
>   rgba(255,255,255,.3) 20px,
>   rgba(255,255,255,.3) 40px);
>  background-color: rgb(255,170,68);
> }

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

Btw. how do other browser vendors handle that? I think it would be great to have a unique way to show that the browser under control of a webdriver compatible automation tool.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 7 years ago

Safari uses a similar technique to light up its address bar, seen in https://webkit.org/blog/6900/webdriver-support-in-safari-10/.  I’m not sure what the benefit is in doing something different than them.

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

Sounds good! It's interesting to see that they even use specific windows, which do not have access to the normal browsing. So something like the containers we have in Firefox. Maybe we should also make use of this feature if/when it's stable enough.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 7 years ago

Second draft.

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

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Sounds good! It's interesting to see that they even use specific windows,
> which do not have access to the normal browsing. So something like the
> containers we have in Firefox. Maybe we should also make use of this feature
> if/when it's stable enough.

I think this is a limitation of the Safari architecture: since they don’t have profiles, they need to create “special” windows that have different state.

	:Gijs (he/him)
	Comment 10 • 7 years ago

What's the threat model here? I don't understand what this is going to help with. If the Firefox instance is under control of a marionette instance, the marionette instance will permit running JS with chrome privileges. That JS can just remove the relevant UX cues.

	:Gijs (he/him)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8858103 [details]
Bug 1355890 - Move robot favicon to separate file

https://reviewboard.mozilla.org/r/130066/#review132966

r- pending an answer to the questions on the bug, but also given that this has test failures on try.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 12 • 7 years ago

(In reply to :Gijs from comment #10)
> What's the threat model here? I don't understand what this is going to
> help with. If the Firefox instance is under control of a marionette
> instance, the marionette instance will permit running JS with chrome
> privileges. That JS can just remove the relevant UX cues.

It is true that once an attacker has access to a Marionette session,
pretty much everything in Firefox can be changed.  Indeed, through XPCOM
interfaces, it also puts the attacker in a position to read and write to
the filesystem.

However, if the attacker is already in a position to start
Marionette—which requires you to pass the -marionette flag to
Firefox—you probably have worse remote access problems than being able
to evaluate JS with chrome privileges.

Generally speaking, the WebDriver/remote protocol security story is one
of a layered defense (https://en.wikipedia.org/wiki/Layered_security):
enabling the server necessitates explicit action from the user, it is
disabled by default, and only accepts connections from loopback devices.

Whilst an attack can bypass any visual cue added to the browser chrome,
it is another hurdle to jump through.  A visual cue like this also
doesn’t prevent or limit functionality, meaning it is merely meant to
be a visual indicator to the user that something sinister or suspicious
is going on with the window.  This can be enough to alert the user of
foul play.

The WebDriver standard writes more about this in the
‘Security Considerations’ chapter, but doesn’t
really expand on the motivation behind its recommendations
(https://w3c.github.io/webdriver/webdriver-spec.html#security-considerat
ions), in particular on having a visual cue that:

> It is also suggested that user agents make an effort to visually
> distinguish a user agent session that is under control of WebDriver
> from those used for normal browsing sessions. This can be done through
> a browser chrome element such as a “door hanger”, colorful
> decoration of the OS window, or some widget element that is prevalent
> in the window so that it easy to identify automation windows.

You can see Safari’s approach to identifying automation windows
in https://webkit.org/blog/6900/webdriver-support-in-safari-10/,
specifically
https://webkit.org/wp-content/uploads/automation-window.png.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 15 • 7 years ago

Fixed the test failure.

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

try run looks green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3d678fac166&group_state=expanded

	:Gijs (he/him)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8858103 [details]
Bug 1355890 - Move robot favicon to separate file

https://reviewboard.mozilla.org/r/130066/#review133670

(In reply to Andreas Tolfsen ‹:ato› from comment #12)
> Whilst an attack can bypass any visual cue added to the browser chrome,
> it is another hurdle to jump through.  A visual cue like this also
> doesn’t prevent or limit functionality, meaning it is merely meant to
> be a visual indicator to the user that something sinister or suspicious
> is going on with the window.  This can be enough to alert the user of
> foul play.

I struggle with calling this a hurdle when there's a pref to switch it off - a pref that the marionette caller can just set (or, if we remove the pref, a class the marionette caller can just add/remove from the relevant element).

> The WebDriver standard writes more about this in the
> ‘Security Considerations’ chapter, but doesn’t
> really expand on the motivation behind its recommendations
> (https://w3c.github.io/webdriver/webdriver-spec.html#security-considerat
> ions), in particular on having a visual cue that:
> 
> > It is also suggested that user agents make an effort to visually
> > distinguish a user agent session that is under control of WebDriver
> > from those used for normal browsing sessions. This can be done through
> > a browser chrome element such as a “door hanger”, colorful
> > decoration of the OS window, or some widget element that is prevalent
> > in the window so that it easy to identify automation windows.
> 
> You can see Safari’s approach to identifying automation windows
> in https://webkit.org/blog/6900/webdriver-support-in-safari-10/,
> specifically
> https://webkit.org/wp-content/uploads/automation-window.png.

Right, but in Safari that functionality can presumably not just be overridden by the marionette caller?

I think that, if we do this, we should implement it in a way that doesn't lend itself to easy bypass from the privileged-JS environment that the marionette client has access to, which pretty much implies that we bake it into the C++ XUL/Layout/Graphics stack, to do an always-on-top window decoration of some description.

::: browser/base/content/browser.css:482
(Diff revision 2)
> +  background: repeating-linear-gradient(
> +    -45deg,
> +    transparent,
> +    transparent 25px,
> +    rgba(255,255,255,.3) 25px,
> +    rgba(255,255,255,.3) 50px);
> +  background-color: rgba(255,170,68,.8);

What's the foreground color here, and where/how is it set? Generally, if you set the background color you *need* to set the foreground color as well, to guarantee user-readable behaviour in case they or their OS theme specify non-default foreground colors. If we already set it to (hardcoded) black/white, fine, but we should be sure. Ditto for the 'faded out' "rest of the URL" bits when we domain-highlight the contents of the URL bar, and for the placeholder text.

	David Burns :automatedtester
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8858104 [details]
Bug 1355890 - Add remote-active system notification

https://reviewboard.mozilla.org/r/130068/#review135380

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

Comment on attachment 8858103 [details]
Bug 1355890 - Move robot favicon to separate file

https://reviewboard.mozilla.org/r/130066/#review133670

Out of interest, why is there a need for this prefernce? Cannot Marionette itself set its class in the urlbar bindings? We can then make it dependent of its internal active state.

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

Comment on attachment 8858103 [details]
Bug 1355890 - Move robot favicon to separate file

https://reviewboard.mozilla.org/r/130066/#review133670

It is true that it would be trivial to disable the UX cue by using Marionette’s chrome script injection, but if the attacker already has shell access to your system to launch Firefox, then you probably have bigger issues to worry about than the fact that the UX cue can be removed.

Indeed, even if you would manipulate the styling from C++, it would still be possible to manipulate the XUL through XPCOM from JS?  Unless you are proposing a more invasive UX measure by modifying the widget stack?  I’m asking because it’s not clear to me what the alternative you’re proposing is.

My position on this is still that it adds trivial reassurance to users to know that the window is under automation.  Whilst it is true that the UX cue can be circumvented using somewhat trivial means, given some knowledge of XUL, that is still true if the attacker has access to modify the binary.

	:Gijs (he/him)
	Comment 23 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #20)
> Comment on attachment 8858103 [details]
> Bug 1355890 - Show UX cue when Firefox is under control of Marionette
> 
> https://reviewboard.mozilla.org/r/130066/#review133670
> 
> It is true that it would be trivial to disable the UX cue by using
> Marionette’s chrome script injection, but if the attacker already has shell
> access to your system to launch Firefox, then you probably have bigger
> issues to worry about than the fact that the UX cue can be removed.

I don't follow the logic here though - if we're not worried about abuse, then why do we need the indicator?

> Indeed, even if you would manipulate the styling from C++, it would still be
> possible to manipulate the XUL through XPCOM from JS?

If you're still relying on the style system, yes.

>  Unless you are
> proposing a more invasive UX measure by modifying the widget stack?

Yes, indeed. I think, for this to raise the bar from a security perspective, we should display an icon in the titlebar area or something similar, via the native widget implementations.

>  I’m
> asking because it’s not clear to me what the alternative you’re proposing is.
> 
> My position on this is still that it adds trivial reassurance to users to
> know that the window is under automation.

Sorry, so can you outline why we're doing this? Who needs to be reassured and under what circumstances? Presumably "deliberate" users of marionette themselves know what is going on, because they're the ones starting the browser... why do they need an indicator? Is there some benefit besides as a warning of potentially nefarious use (which I think is only effective if it can be relied upon to be present in such circumstances)?

>  Whilst it is true that the UX cue
> can be circumvented using somewhat trivial means, given some knowledge of
> XUL, that is still true if the attacker has access to modify the binary.

That would break the binary's signature. Also, on Windows (and, in fewer cases, on OS X), the binary and/or app dir is often not user-writable, but the profile dir and/or temp dir is, and so having a pref or style change (which can be undone through marionette itself, ie where all you need is permission to start the application and/or modify user directories) does actually lower the bar compared to having to modify the binary (and somehow deal with the signature issues), if we're looking at this from a security perspective.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 24 • 7 years ago

(In reply to :Gijs from comment #23)
> (In reply to Andreas Tolfsen ‹:ato› from comment #20)
> > Comment on attachment 8858103 [details]
> > Bug 1355890 - Show UX cue when Firefox is under control of Marionette
> > 
> > https://reviewboard.mozilla.org/r/130066/#review133670
> > 
> > It is true that it would be trivial to disable the UX cue by using
> > Marionette’s chrome script injection, but if the attacker already has shell
> > access to your system to launch Firefox, then you probably have bigger
> > issues to worry about than the fact that the UX cue can be removed.
> 
> I don't follow the logic here though - if we're not worried about abuse,
> then why do we need the indicator?

We are worried about users in control of their own system to be able to distinguish a normal browsing session window from one under automation, and about the unskilled attacker.

> > Indeed, even if you would manipulate the styling from C++, it would still be
> > possible to manipulate the XUL through XPCOM from JS?
> 
> If you're still relying on the style system, yes.
> 
> >  Unless you are
> > proposing a more invasive UX measure by modifying the widget stack?
> 
> Yes, indeed. I think, for this to raise the bar from a security perspective,
> we should display an icon in the titlebar area or something similar, via the
> native widget implementations.

The OS titlebar?  Not all WMs have titlebars.

I thought briefly about increasing the border of the window and giving it a colour, but that will affect window dimensions, which I suspect many users would care about remaining the same under automation conditions.

> > I’m asking because it’s not clear to me what the alternative you’re 
> > proposing is.
> > 
> > My position on this is still that it adds trivial reassurance to users to
> > know that the window is under automation.
> 
> Sorry, so can you outline why we're doing this? Who needs to be reassured
> and under what circumstances?

We are providing legit users reassurance which windows on their screen are under automation, to lessen the chance they use the wrong window for entering potentially private information.

We are also adding one more hurdle for a malicious attacker with shell access to the user’s account to jump through for disguising an automation window as a real window.  This is thought to add to our layered security, but it is correctly possible, if not entirely trivial, to bypass.

It is worth noting that for the attacker to be able to connect to the Marionette session, she must have shell access since it is not possible to remotely connect since the server only accepts connections from loopback devices.

I believe the latter assurance is weaker than the first.

> Presumably "deliberate" users of marionette
> themselves know what is going on, because they're the ones starting the
> browser... why do they need an indicator? Is there some benefit besides as a
> warning of potentially nefarious use (which I think is only effective if it
> can be relied upon to be present in such circumstances)?

You would think so, but I have made the mistake myself of mistaking a window under Marionette control as my real browser session.

> > Whilst it is true that the UX cue can be circumvented using somewhat
> > trivial means, given some knowledge of XUL, that is still true if the
> > attacker has access to modify the binary.
> 
> That would break the binary's signature. Also, on Windows (and, in fewer
> cases, on OS X), the binary and/or app dir is often not user-writable, but
> the profile dir and/or temp dir is, and so having a pref or style change
> (which can be undone through marionette itself, ie where all you need is
> permission to start the application and/or modify user directories) does
> actually lower the bar compared to having to modify the binary (and somehow
> deal with the signature issues), if we're looking at this from a security
> perspective.

This is a fair point, but then with access to the user’s account, nothing prevents the attacker from uploading a custom binary.

I’m skeptical that coding this into the C++ widget layer, which would mean platform specific work, is worth the investment given the obvious flaws with the security tactic that you most correctly point out.  I wonder if it would be worth circling this back to the WebDriver Working Group to have this requirement taken out of the Security chapter would help the argument for implementing this.

Because we at some later date also need to provide a navigator.webdriver IDL attribute, we probably need to at some point add a Marionette C++ interface (I’m treading unfamiliar ground here), but I was hoping to delay that until later.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 25 • 7 years ago

I talked to Gijs and in conclusion, we agreed a better approach would be for Marionette to handle the branding of new windows as automation windows.

Marionette will install a browser-delayed-startup-finished observer that will add an attribute (not a CSS class) to the urlbar.  The style system will pick this up and apply the visual UX cue.

Since Marionette is active for as long as the -marionette flag is passed to the process, we will likely not have to do any cleanup, but it would be possible to do so through iterating over Services.wm.getEnumerator("navigator:browser") and removing the attribute, e.g. in TCPListener#stop or the component’s uninit method.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 26 • 7 years ago

Latest draft of current visual cue.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 30 • 7 years ago

Patch without visual cue added.  No change.

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

Comment on attachment 8858103 [details]
Bug 1355890 - Move robot favicon to separate file

https://reviewboard.mozilla.org/r/130066/#review133670

> What's the foreground color here, and where/how is it set? Generally, if you set the background color you *need* to set the foreground color as well, to guarantee user-readable behaviour in case they or their OS theme specify non-default foreground colors. If we already set it to (hardcoded) black/white, fine, but we should be sure. Ditto for the 'faded out' "rest of the URL" bits when we domain-highlight the contents of the URL bar, and for the placeholder text.

Latest revision sets foreground colour to black, since we override the background to white with an orange stripy pattern.  (See screenshots attached to the bug.)

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 33 • 7 years ago

I’ve updated the patches based on feedback from :gijs.  The first rewrite I did based on the feedback made Marionette listen for the browser-delayed-startup-finished notification and modify #urlbar directly.

After feedback from :jgraham that an orange address bar probably isn’t enough indication to the user that the browser session is under automation, I followed :jaws’ suggestion and re-purposed the robot icon from about:robots.  I add this to #identity-box, next to where the other address bar icons are.  See the attached screenshots for the final result.  The robot icon carries a tooltip text that says the browser is under remote control.

I want to do a follow-up on the current changeset to add more information to the identity popup that appears when clicking the address bar icons about what the robot icon actually means.  This should involve a short description that the browser is under remote control and possibly what that means.  I hope it is acceptable that I make this a follow-up bug, as adding more information to the popup is not a trivial task.

Because of the feedback I got, I decided to introduce a new system notification called remote-control that fires when the Marionette server starts and when new browser windows appear.  I decided to go in this direction because of the need to add the information I described to the identity popup in the future: it’s fine to add an attribute to a XUL element from Marionette, but adding anything slightly more complicated than that makes for very ugly code.

	:Gijs (he/him)
	Comment 34 • 7 years ago

I think having marionette notify start/end of remote control is fine.

I don't think that having marionette send this notification to all the observers for every window that appears is a good idea, mostly because it's inefficient (all the pre-existing windows will also be notified) and also because each attribute setting/removal (even if the same attribute was already present/gone) will trigger mutation observers and potentially style/layout flushes.

Instead, besides the initial/final notification that is fired when marionette control starts/ends, I think we should expose the state on the component somehow. I know that the long-term plan is to expose it via webidl, and I've provided too much stop energy on this bug already (sorry!), so I'm trying to figure out what the least painful way of doing this would be. Adding a dedicated XPCOM IDL thing seems daft - it'll be a lot of boilerplate that will go away once we have webidl support for this.

I suspect that it might be easiest to just set:

this.wrappedJSObject = this

in the constructor (thus exposing the JS object to JS consumers of the XPCOM component), and use a JS getter/property on the component that indicates whether the server is alive. Then we can just check that getter when the window loads (in onLoad in browser.js) and set the attribute then, thus avoiding adding style flushes etc. for new windows.

Does that make sense?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 40 • 7 years ago

Thanks for your detailed feedback, Gijs!  Despite not being very pleasant, I took the route of adding a nsIMarionette XPCOM interface that lets browser.js query if the remote protocol is running.  This lets it set the remotecontrol attribute on <xul:browser> when it is created, saving us from having to send "remote-control" system notifications after every new browser is opened; this notification is only sent when Marionette starts (and stops).

	David Burns :automatedtester
	Comment 41 • 7 years ago
mozreview-review

Comment on attachment 8867286 [details]
Bug 1355890 - Namespace Marionette component

https://reviewboard.mozilla.org/r/138820/#review142488

	David Burns :automatedtester
	Comment 42 • 7 years ago

r+ though Try is a sea of orange/red. I am not sure if it is related to this or not but Marionette is not appearing to startup

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 43 • 7 years ago

(In reply to David Burns :automatedtester from comment #42)
> r+ though Try is a sea of orange/red. I am not sure if it is related to this
> or not but Marionette is not appearing to startup

I see that Firefox fails to start with the following error message, but I’m unable to reproduce it locally:

> 18:24:18     INFO - GECKO(2374) | JavaScript error: resource://gre/modules/XPCOMUtils.jsm, line 230:
> NS_ERROR_XPC_BAD_IID: Component returned failure code:
> 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]

It would of course be helpful if XPCOM gave us a stacktrace so we knew where it originated from, but I guess it’s likely to be caused by this change:

> XPCOMUtils.defineLazyServiceGetter(
>     this, "Marionette", "@mozilla.org/testing/marionette;1", "nsIMarionette");

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 44 • 7 years ago

Triggered a new try run with a clobber: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db6d36e0feb5e534deb51462ce3889521287da0

	:Gijs (he/him)
	Comment 45 • 7 years ago

Total guess while plowing through my bugmail + requests: maybe you need to package the .xpt file? Can you reproduce with the packaged build (ie after ./mach package) ?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 51 • 7 years ago

(In reply to :Gijs from comment #45)
> Total guess while plowing through my bugmail + requests: maybe you need to
> package the .xpt file? Can you reproduce with the packaged build (ie after
> ./mach package) ?

Thanks for pointing this out, Gijs!  After a bit of digging I found that I need to add the XPIDL_MODULE to browser/installer/package-manifest.in.

	:Gijs (he/him)
	Comment 52 • 7 years ago
mozreview-review

Comment on attachment 8858103 [details]
Bug 1355890 - Move robot favicon to separate file

https://reviewboard.mozilla.org/r/130066/#review142564

	:Gijs (he/him)
	Comment 53 • 7 years ago
mozreview-review

Comment on attachment 8862638 [details]
Bug 1355890 - Add visual cue to urlbar when under remote control

https://reviewboard.mozilla.org/r/134486/#review142566

Almost there! This would have been r+ with comments, but given the l10n issue maybe it's a good idea if I have another look.

::: browser/base/content/browser.js:200
(Diff revision 4)
> +XPCOMUtils.defineLazyServiceGetter(
> +    this, "Marionette", "@mozilla.org/remote/marionette;1", "nsIMarionette");

Can you add it to (the eslint comment as well as) the list above (line 100 in this version of the file) instead of adding a single line here?

::: browser/base/content/browser.js:7863
(Diff revision 4)
> +/**
> + * Fired on the "remote-control" system notification, indicating if the
> + * browser session is under remote control.
> + */
> +const gRemoteControl = {
> +  observe (subject, topic, data) {

Nit: no space before '(', here and below.

Does this pass eslint?

::: browser/base/content/browser.xul:799
(Diff revision 4)
>                    <image id="persistent-storage-notification-icon" class="notification-anchor-icon persistent-storage-icon" role="button"
>                           tooltiptext="&urlbar.persistentStorageNotificationAnchor.tooltip;"/>
>                  </box>
>                  <image id="connection-icon"/>
> +                <image id="remote-control-icon"
> +                       tooltiptext="Browser is under remote control"/>

This string should be localized and live in a DTD file (ie browser/locales/en-US/chrome/browser/browser.dtd ).

::: browser/themes/shared/identity-block/icons.inc.css:39
(Diff revision 4)
>  #tracking-protection-icon[state="loaded-tracking-content"]@selectorSuffix@ {
>    list-style-image: url(chrome://browser/skin/tracking-protection-16.svg#disabled@iconVariant@);
>  }
>  
>  
> +#main-window[remotecontrol] #remote-control-icon@selectorSuffix@ {

This file gets included twice with different selector suffix / icon variable replacements. However, this block is completely fixed (ie doesn't use a different icon for a different selector suffix). Can you move it to the bottom of https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-block.inc.css instead, removing the @selectorSuffix@ replacement var?

	:Gijs (he/him)
	Comment 54 • 7 years ago
mozreview-review

Comment on attachment 8858104 [details]
Bug 1355890 - Add remote-active system notification

https://reviewboard.mozilla.org/r/130068/#review142568

::: testing/marionette/server.js:322
(Diff revision 6)
>    start () {
>      if (this.alive) {
>        return;
>      }
>  
> +    Services.obs.notifyObservers(this, "remote-control", true);

I wonder if this should have 'marionette' in the topic. Don't feel strongly either way, up to you.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 55 • 7 years ago
mozreview-review

Comment on attachment 8858104 [details]
Bug 1355890 - Add remote-active system notification

https://reviewboard.mozilla.org/r/130068/#review142626

f+

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 56 • 7 years ago
mozreview-review

Comment on attachment 8862638 [details]
Bug 1355890 - Add visual cue to urlbar when under remote control

https://reviewboard.mozilla.org/r/134486/#review142630

f+

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 57 • 7 years ago
mozreview-review

Comment on attachment 8867287 [details]
Bug 1355890 - Add Marionette XPCOM interface

https://reviewboard.mozilla.org/r/138822/#review142634

This seems fine. I cleared my other review requests in this series with f+: everything looks right to me based on a shallow knowledge of this codebase.

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

Comment on attachment 8858104 [details]
Bug 1355890 - Add remote-active system notification

https://reviewboard.mozilla.org/r/130068/#review142568

> I wonder if this should have 'marionette' in the topic. Don't feel strongly either way, up to you.

It can do, yes.  I’ve changed it to "marionette-remote-control" and updated the subsequent patch.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 69 • 7 years ago
mozreview-review

Comment on attachment 8867822 [details]
Bug 1355890 - Infer identity box background-color by computed style

https://reviewboard.mozilla.org/r/139348/#review142730

::: testing/marionette/harness/marionette_harness/tests/unit/test_chrome_element_css.py:26
(Diff revision 3)
> -            element = self.marionette.find_element(By.ID, "identity-box")
> -            background_colour = element.value_of_css_property("background-color")
> -
> +            expected_bg_colour = self.get_element_computed_style(
> +                identity_box, "backgroundColor")
> +            actual_bg_colour = identity_box.value_of_css_property("background-color")

What am I missing? It looks to me that `expected_bg_color` and `actual_bg_color` are always going to be the same, since they're both ultimately based on `getComputedStyle`. If so, why the comparison? Does this test really just want to check that `value_of_css_property` returns successfully?

	:Gijs (he/him)
	Comment 70 • 7 years ago
mozreview-review

Comment on attachment 8862638 [details]
Bug 1355890 - Add visual cue to urlbar when under remote control

https://reviewboard.mozilla.org/r/134486/#review142742

Thanks for the quick update, this looks good to me! :-)

	:Gijs (he/him)
	Comment 71 • 7 years ago
mozreview-review

Comment on attachment 8862638 [details]
Bug 1355890 - Add visual cue to urlbar when under remote control

https://reviewboard.mozilla.org/r/134486/#review142744

::: browser/base/content/browser.js:7858
(Diff revision 7)
>      this.panel.openPopup(this.button, "bottomcenter topright");
>    },
>  };
>  
> +/**
> + * Fired on the "remote-control" system notification, indicating if the

Oops, late nit: this is now marionette-remote-control. :-)

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

Comment on attachment 8862638 [details]
Bug 1355890 - Add visual cue to urlbar when under remote control

https://reviewboard.mozilla.org/r/134486/#review142744

> Oops, late nit: this is now marionette-remote-control. :-)

Good catch!

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

Comment on attachment 8867822 [details]
Bug 1355890 - Infer identity box background-color by computed style

https://reviewboard.mozilla.org/r/139348/#review142730

> What am I missing? It looks to me that `expected_bg_color` and `actual_bg_color` are always going to be the same, since they're both ultimately based on `getComputedStyle`. If so, why the comparison? Does this test really just want to check that `value_of_css_property` returns successfully?

They are both ultimately based on getComputedStyle, but we’re not testing getComputedStyle.  This checks that Marionette is in the correct context (chrome), that it is possible to look up a chrome element’s computed style, and that the command has the correct plumbing.

This test is of course written with the expectation that the execute_script call returns the correct thing, and if you don’t like this interdependency, I can put back the hard-coded expected value.  But I’d like to reiterate that it’s not getComputedStyle we’re testing here.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 84 • 7 years ago
mozreview-review-reply

Comment on attachment 8867822 [details]
Bug 1355890 - Infer identity box background-color by computed style

https://reviewboard.mozilla.org/r/139348/#review142730

> They are both ultimately based on getComputedStyle, but we’re not testing getComputedStyle.  This checks that Marionette is in the correct context (chrome), that it is possible to look up a chrome element’s computed style, and that the command has the correct plumbing.
> 
> This test is of course written with the expectation that the execute_script call returns the correct thing, and if you don’t like this interdependency, I can put back the hard-coded expected value.  But I’d like to reiterate that it’s not getComputedStyle we’re testing here.

I understand. That's fine.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 85 • 7 years ago
mozreview-review

Comment on attachment 8867822 [details]
Bug 1355890 - Infer identity box background-color by computed style

https://reviewboard.mozilla.org/r/139348/#review143056

	Pulsebot
	Comment 92 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc9c5c0a15d1
Namespace Marionette component r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/3d995af23408
Add Marionette XPCOM interface r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/04092bfa74d5
Move robot favicon to separate file r=Gijs
https://hg.mozilla.org/integration/autoland/rev/89c597001a47
Add remote-active system notification r=automatedtester,Gijs
https://hg.mozilla.org/integration/autoland/rev/afc3591a1646
Add visual cue to urlbar when under remote control r=Gijs
https://hg.mozilla.org/integration/autoland/rev/05410ef975e2
Infer identity box background-color by computed style r=maja_zf

	Carsten Book [:Tomcat]
	Comment 93 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cc9c5c0a15d1
https://hg.mozilla.org/mozilla-central/rev/3d995af23408
https://hg.mozilla.org/mozilla-central/rev/04092bfa74d5
https://hg.mozilla.org/mozilla-central/rev/89c597001a47
https://hg.mozilla.org/mozilla-central/rev/afc3591a1646
https://hg.mozilla.org/mozilla-central/rev/05410ef975e2

	Johann Hofmann [:johannh]
	Comment 94 • 7 years ago

Mozscreenshots results, in case you want to review:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=41958333867b0f537271dbd4cb4ba9e8a67a85a8&newProject=mozilla-central&newRev=95990be385ca8331c466ba5f174794771955751e