Closed
Bug 1355890
Opened 7 years ago
Closed 7 years ago
Show UX cue when Firefox is under control by Marionette
Categories
(Remote Protocol :: Marionette, enhancement)
Tracking
(firefox55 fixed)
VERIFIED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
Details
Attachments
(8 files, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
12.07 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
12.66 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
We should provide some cue in the Firefox UI that it is under automation and control by Marionette when there is an active connection.
Assignee | ||
Updated•7 years ago
|
Group: mozilla-employee-confidential
Updated•7 years ago
|
Group: core-security-release
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);
> }
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.
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.
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.
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.
Assignee | ||
Updated•7 years ago
|
Group: mozilla-employee-confidential
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
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.
Flags: needinfo?(ato)
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.
Attachment #8858103 -
Flags: review?(gijskruitbosch+bugs) → review-
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.
Flags: needinfo?(ato)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Fixed the test failure.
Assignee | ||
Comment 16•7 years ago
|
||
try run looks green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3d678fac166&group_state=expanded
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.
Attachment #8858103 -
Flags: review?(gijskruitbosch+bugs) → review-
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
Attachment #8858104 -
Flags: review?(dburns) → review+
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.
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8858103 -
Flags: review?(dburns)
Assignee | ||
Updated•7 years ago
|
Attachment #8858103 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8858104 -
Flags: review+
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.
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.
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.
Assignee | ||
Comment 26•7 years ago
|
||
Latest draft of current visual cue.
Attachment #8858073 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
Patch without visual cue added. No change.
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.)
Comment hidden (mozreview-request) |
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.
Flags: needinfo?(gijskruitbosch+bugs)
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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ato)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8867286 -
Flags: review?(dburns)
Attachment #8867287 -
Flags: review?(mjzffr)
Attachment #8858104 -
Flags: review?(mjzffr)
Attachment #8858104 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8862638 -
Flags: review?(mjzffr)
Attachment #8862638 -
Flags: review?(gijskruitbosch+bugs)
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).
Flags: needinfo?(ato)
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8867286 [details] Bug 1355890 - Namespace Marionette component https://reviewboard.mozilla.org/r/138820/#review142488
Attachment #8867286 -
Flags: review?(dburns) → review+
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
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");
Assignee | ||
Comment 44•7 years ago
|
||
Triggered a new try run with a clobber: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db6d36e0feb5e534deb51462ce3889521287da0
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) ?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
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
Attachment #8858103 -
Flags: review?(gijskruitbosch+bugs) → review+
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?
Attachment #8862638 -
Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8858104 -
Flags: review?(gijskruitbosch+bugs) → review+
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+
Attachment #8858104 -
Flags: review?(mjzffr)
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+
Attachment #8862638 -
Flags: review?(mjzffr)
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.
Attachment #8867287 -
Flags: review?(mjzffr) → review+
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8858104 -
Flags: review?(mjzffr)
Attachment #8862638 -
Flags: review?(mjzffr)
Attachment #8862638 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8867822 -
Flags: review?(mjzffr)
Assignee | ||
Updated•7 years ago
|
Attachment #8862638 -
Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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?
Attachment #8867822 -
Flags: review?(mjzffr) → review-
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! :-)
Attachment #8862638 -
Flags: review?(gijskruitbosch+bugs) → review+
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. :-)
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!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
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
Attachment #8867822 -
Flags: review?(mjzffr) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Status: RESOLVED → VERIFIED
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•