Closed Bug 1317659 Opened 8 years ago Closed 7 years ago

Implement Preview Panel

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox53 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Iteration:
53.3 - Dec 26
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file, 1 obsolete file)

Bug 1317659 - Implement Preview Panel
58 bytes, text/x-review-board-request
jsnajdr
: review+
Honza
: review+
Details 
This is a breakdown task for bug 1308450 in order to integrate HTTP inspector into the Net panel.

- Implement Preview Panel
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor]
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 53.1 - Nov 28
Bug 1309866 will introduce the state.requests.selected to reference a selected request ID. Every sidebar panels will depend on this state to determine which request is opened.

So it makes sense that this implementation needs to wait for bug 1309866.
Depends on: 1309866
No longer blocks: 1308450
I'm working on implementing react preview panel without integrating redux since the new redux architecture is depending on bug 1309866.

WIP for react migration is ready but I'm struggling with the timeout issue of browser_net_html-preview.js. I cannot figure out why test case is unable to receive the event from details-view.js

yield monitor.panelWin.once(EVENTS.RESPONSE_HTML_PREVIEW_DISPLAYED);
Fixed timeout issue mentioned on comment 2 by replacing 

yield monitor.panelWin.once(EVENTS.RESPONSE_HTML_PREVIEW_DISPLAYED);

with

yield once(iframe, "DOMContentLoaded");

Here is an explanation:

When toggleDetailsPane() happen, _setHtmlPreview will trigger immediately as well as emitting a EVENTS.RESPONSE_HTML_PREVIEW_DISPLAYED event.
We inserted `yield monitor.panelWin.once(EVENTS.RESPONSE_HTML_PREVIEW_DISPLAYED)` in wrong order after toggleDetailsPane(), so that event lost leads to timeout.

It was so lucky that it works in the old patch because there was an async call `yield gNetwork.getString(text);` which delayed the EVENTS.RESPONSE_HTML_PREVIEW_DISPLAYED event.
One more thing:

I noticed that we used an internal API for disabling Javascript in iframe.

iframe.contentDocument.docShell.allowJavascript = false;

Then I try to port it through using iframe sandbox. Everything works perfectly but the content for youtube page doesn't look identical. `docShell.allowJavascript = false` which turns off css effect accidentally but I don't know why. I think the current result make more sense to me and that should be the expected consequence we want.
Current WIP patch is focusing on providing react migration without integrating redux store since all details sub-panels depend on bug 1309866, but it hasn't landed yet.

I will continue finishing WIP patch once bug 1309866 is landed.
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review95962

I like the patch, nice and simple!


There is one inline comment, but we might discuss that yet.

R+ assuming try is green.


Honza

::: devtools/client/netmonitor/components/http-inspector/preview-panel.js:32
(Diff revision 6)
> +
> +PreviewPanel.propTypes = {
> +  src: PropTypes.string.isRequired,
> +};
> +
> +module.exports = connect(

I know we are using this construct at a few other places, but I think that having separate mapStateToProps() function that is passed to connect() cal would be better. We can discuss at out meeting if needed.
Attachment #8813551 - Flags: review?(odvarko) → review+
It's hard to see what is actually implemented on top of Jarda's patch. Could it be done as separate patch?

Honza
Honza,

Patch has already separated as you want, see bug attachment at the top

* Bug 1317659 - Implement Preview Panel (my patch)
* Request List WIP (Jarda's patch)

So everything you see at https://reviewboard.mozilla.org/r/94998/diff/6#index_header is my implementaion.
(In reply to Ricky Chien [:rickychien] from comment #16)
> Honza,
> 
> Patch has already separated as you want, see bug attachment at the top
Ok, great.

Honza
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review95962

> I know we are using this construct at a few other places, but I think that having separate mapStateToProps() function that is passed to connect() cal would be better. We can discuss at out meeting if needed.

We can keep code style consistency but refactor this part in a follow up.

Btw, do you mean to move this arrow function out of connect() so that it is going to look like the sample in redux document? http://redux.js.org/docs/basics/UsageWithReact.html#containersvisibletodolistjs
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review95984

::: devtools/client/netmonitor/components/http-inspector/preview-panel.js:36
(Diff revision 6)
> +    const src = (selectedRequest && Filters.html(selectedRequest)) ?
> +      selectedRequest.url : "";

The Preview iframe should display the actual data that were received in the response. Not fetch the data from the same URL again. There's no guarantee that a second request will return the same response.

This code also always triggers a GET request, no matter what the original method was.
Attachment #8813551 - Flags: review?(jsnajdr) → review-
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review95986

::: devtools/client/netmonitor/components/http-inspector/preview-panel.js:22
(Diff revision 6)
> +function PreviewPanel({
> +  src,
> +}) {
> +  return iframe({
> +    id: "response-preview",
> +    frameBorder: "0",

MDN says that the frameBorder attribute is HTML4-only. Can it be set by CSS in HTML5? Is it even necessary?

::: devtools/client/netmonitor/components/http-inspector/preview-panel.js:23
(Diff revision 6)
> +  src,
> +}) {
> +  return iframe({
> +    id: "response-preview",
> +    frameBorder: "0",
> +    sandbox: "",

I'm not sure how secure this is - loading untrusted content from network into an iframe in a privileged document. Does the sandbox attribute really solve all potential security issues?

The previous code was setting the "allowJavascript" attribute on the docShell. What's the difference between this and the "allow-scripts" keyword for the sandbox?

I'd prefer if someone more knowledgeable reviewed this - I don't feel qualified enough here.
(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> I know we are using this construct at a few other places, but I think that
> having separate mapStateToProps() function that is passed to connect() cal
> would be better. We can discuss at out meeting if needed.

I like how the zeit/hyper project organizes the container components created by connect() - they have them in a separate "containers" directory, apart from the presenational components that render the UI. For example, here:

https://github.com/zeit/hyper/blob/master/lib/containers/terms.js

I suppose it makes unit testing much easier.

I don't mind if the mapStateToProps are inlined in the connect call. However, it's a code smell if the mapStateToProps has any nontrivial logic in it. Ideally, it should only map pieces of state directly, or call selectors. If there's anything else, it should be moved to a selector.
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review96424

::: devtools/client/netmonitor/components/http-inspector/preview-panel.js:35
(Diff revision 6)
> +};
> +
> +module.exports = connect(
> +  (state) => {
> +    const { requests, selectedId } = state.requests;
> +    const selectedRequest = requests.find((req) => req.id === selectedId);

can reuse `selectors.requests.getRequestById(selectedId)`
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review96424

> can reuse `selectors.requests.getRequestById(selectedId)`

It might worth to move this code block as a new requests selector, therefore we keep the connector code simple and have a testable function on selector/
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review96622

::: devtools/client/netmonitor/components/moz.build:6
(Diff revision 6)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +DIRS += [
> +    'http-inspector',

I suggest use `dissector`  for the folder name, which is also used in wireshark for dissecting the details of the protocol https://www.wireshark.org/docs/wsdg_html_chunked/ChapterDissection.html

I think its a good metaphor since all tab panels' goal are trying to dissect part of the http packet and provide the meaningful information.

How do you think?
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review96646

::: devtools/client/netmonitor/components/http-inspector/preview-panel.js:27
(Diff revision 6)
> +    frameBorder: "0",
> +    sandbox: "",
> +    src,
> +  });
> +}
> +

add `PreviewPanel.displayName = "PreviewPanel";`
(In reply to Fred Lin [:gasolin] from comment #24)
> Comment on attachment 8813551 [details]
> Bug 1317659 - Implement Preview Panel
> 
> https://reviewboard.mozilla.org/r/94998/#review96622
> 
> ::: devtools/client/netmonitor/components/moz.build:6
> (Diff revision 6)
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +DIRS += [
> > +    'http-inspector',
> 
> I suggest use `dissector`  for the folder name, which is also used in
> wireshark for dissecting the details of the protocol
> https://www.wireshark.org/docs/wsdg_html_chunked/ChapterDissection.html
> 
> I think its a good metaphor since all tab panels' goal are trying to dissect
> part of the http packet and provide the meaningful information.
> 
> How do you think?

If you import `dissector` in webconsole, it doesen't tell us what it is literally. I would guess that could be a kind of processor or analyzer for console information. The key part of the component aims to display the network relevant information, so everyone can understand it instantly. So it would be better if there has a network keyword (net / http) within its name.
My suggestion for the directory name is "net-details". When imported inside the Netmonitor code, the name tells you that the components here are about the request details. When imported by other tool (console or storage), it suggests that it's something "net"-work related.
What about just simple 'shared' ?

If this directory is required by any other tool/module the full path will indicate from where the module comes from:

require("devtools/client/netmonitor/components/shared");

This is also inline with existing 'shared' directories we have in our code base.

It would also be ok for me to have 'shared' directory top level (within 'netmonitor' dir)

require("devtools/client/netmonitor/shared/components");

Which would help to share other stuff if needed e.g.

require("devtools/client/netmonitor/shared/actions");

Honza
"shared" sound good to me!

I prefer to have 'shared' directory top level, it could end up becoming like this

require("devtools/client/netmonitor/shared/components/xxx");
require("devtools/client/netmonitor/shared/components/net-details");
require("devtools/client/netmonitor/shared/components/preview-panel");
Attachment #8814832 - Attachment is obsolete: true
Patch has rebased on top of bug 1309866.
In this patch:

* Created a new async action `updateRequestContent()` through redux thunk middleware to fetch full text with gNetwork.getString().
* Using iframe.srcdoc to render html content, and that can prevent sending new request if we use iframe.src
* HTML content doesn't render properly in preview panel but it is identical to old panel.

In current implementation, PreviewPanel will act as container component in order to use react life cycle methods `componentWillUpdate` to get full html text, but we could improve that by moving this architecture to higher component - sidebar once all sub-panels are completed.
(In reply to Jarda Snajdr [:jsnajdr] from comment #20)
> 
> I'm not sure how secure this is - loading untrusted content from network
> into an iframe in a privileged document. Does the sandbox attribute really
> solve all potential security issues?
> 
> The previous code was setting the "allowJavascript" attribute on the
> docShell. What's the difference between this and the "allow-scripts" keyword
> for the sandbox?

After tracing iframe sandbox implementation (bug 341604 and patch [1]), I found the place to determine JS content should load [2] which ends up triggering allowJavascript in docshell [3]. As a result, we confirmed that `iframe.sandbox = ""` and `allowJavascript = false` are able to provide the identical restriction to web content.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=650371&action=diff
[2] http://searchfox.org/mozilla-central/source/embedding/browser/nsWebBrowserContentPolicy.cpp#53
[3] http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2182
> I prefer to have 'shared' directory top level, it could end up becoming like
> this
> 
> require("devtools/client/netmonitor/shared/components/xxx");

top level shared looks good to me, it also align inspector's structure
Patch updated again.

I removed `updateRequestContent` action since I found that in bug 1309866 has been introduced a updateRequest() [1] to deal with responseContent. This scenario is similar to my case, so it's the best way to reuse that.

However, this kind of workaround in request-menu-view.js is supposed to be removed in the end of refactoring. For now, we should follow the existing implementation and focus on sidebar panels migration.


[1] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/requests-menu-view.js#202
(In reply to Fred Lin [:gasolin] from comment #36)
> > I prefer to have 'shared' directory top level, it could end up becoming like
> > this
> > 
> > require("devtools/client/netmonitor/shared/components/xxx");
> 
> top level shared looks good to me, it also align inspector's structure
Yes, good point. 

Honza
@Gijs, just wanted to make sure we are doing the right and safe thing here.

DevTools Network panel is displaying a preview for HTML documents and it's using sandboxed iframe as follows:

<iframe sandbox="" srcdoc="document-source" />

Where `document-source` is the source we get from nsIHttpChannel request (response body) associated with the HTML document. We are loading the `document-source` (untrusted content) directly in an iframe. Note that the iframe lives in privileged scope. 

Is this concept safe?
Is the sandbox attribute set on the iframe the only thing we need to do?


You might also see the attached patch,
file: devtools/client/netmonitor/components/shared/preview-panel.js
  
Honza
Flags: needinfo?(gijskruitbosch+bugs)
I won't have time to look at this properly until maaaybe tonight, otherwise maaaaybe tomorrow (in hawaii travel) and otherwise on Monday/Tuesday. It sounds like you want something vaguely resembling a security review, so maybe ping :freddyb?

Off-hand, my answer is "no". You should make sure the iframe is content-privileged as well (type=content) and set a CSP, though there are issues with sandboxed srcbox iframes, IIRC. :ckerschb would know more than me.
(In reply to :Gijs Kruitbosch from comment #42)
> It sounds like you want something vaguely resembling a security review, so
> maybe ping :freddyb?
Yes

@Frederik: can you please look at comment #41?

Honza
Flags: needinfo?(fbraun)
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review97482

Waiting for the sec review...

Honza
Attachment #8813551 - Flags: review+ → review?(odvarko)
(Oddly, setting the sec-review flag does not work for me).
I will take a look on Monday.

@Ricky, it seems like you did some good first analysis in comment 35 and I'd like to verify that.
Do you have a simplified test case that I could use?
Flags: needinfo?(fbraun) → needinfo?(rchien)
(In reply to Frederik Braun [:freddyb] from comment #46)
> I will take a look on Monday.

I shouldn't promise that, actually. Do you have a deadline to get this landed?
Don't worry, we don't have a deadline to get this landed :)

There is a test case for html preview (devtools/client/netmonitor/test/browser_net_html-preview.js) but unfortunately it doesn't verify any potential issues that relate to security. We're not security experts, so we cannot figure out what potential security problems we are supposed to be care.

This patch aims to preview a HTML content in devtools network monitor, which will load a HTML with a chrome privileged iframe (because devtools is currently running on chrome). So we'd like to ensure that loading these external websites do not cause security problems.

I think we can verify this by tracing the gecko implementation, and I hope that it is not too complicated than I think. My idea is that it looks like some docshell APIs ex: `docshell.allowjavascript` are similar to iframe.sandbox attributes ex: `allow-scripts`. I suspect that in gecko there are some different paths to set web content restrictions, and they end up invoking the same API which disables these web content features.
Flags: needinfo?(rchien)
(In reply to :Gijs Kruitbosch from comment #42) 
> Off-hand, my answer is "no". You should make sure the iframe is
> content-privileged as well (type=content) and set a CSP, though there are
> issues with sandboxed srcbox iframes, IIRC. :ckerschb would know more than
> me.

I noticed that type="content" attribute only appears in XUL iframe [1] but it is not for HTML iframe. In current XUL implementation, we just disable JS `docShell.allowJavascript = false` but do not set a CSP as well as type="content".

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/iframe
Here is current XUL implementation:

First we created a static iframe element. This html:iframe setting is definitely wrong because it doesn't add xmlns="http://www.w3.org/1999/xhtml" as second attribute, so after inspecting the element in console I found it's still a XUL iframe and sandbox="" is useless.

http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/netmonitor.xul#519

And then we disabled javascript and set up innerHTML when preview tab is selected.

http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/details-view.js#786
IIRC the previous implementations used <iframe mozbrowser>, who wrote that? That'd make sense security-wise, though being pretty much non-standard.
To my knowledge, <iframe sandbox=""> should block JS fine.
In fact, the script is not being ran here:
data:text/html,<iframe sandbox="" srcdoc="<script>alert('hello')</script>"/>
but it is being ran here:
data:text/html,<iframe srcdoc="<script>alert('hello')</script>"/>

So it seems like sandbox="" does its job properly.

The only issue that comes to mind for me when switching to an HTML iframe is whether the Preview feature works or not with a website with X-Frame-Options (eg. https://google.com), the feature not working is not a problem security-wise, but rather an usability regression.

If that doesn't work, can't we just stick to what's currently being done ?

iframe.contentDocument.docShell.allowJavascript = false;

should just work (even on HTML iframes) as long as the devtools script injecting the iframe has chrome privileges (which it should have).

<iframe mozbrowser> is also a solution (which requires chrome privileges on the HTML document to work).
(Clearing ni as I don't think I have anything to add to the ongoing discussion / Freddy's input.)
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

@smaug,

We'd like to see someone is qualified enough to guarantee that the implementation of 

docshell.allowJavascript = false;

is functional equally to

iframe.sandbox = "";

Just want to ensure that it's safe enough for using an iframe sandbox to display a regular website in preview panel of devtools netmonitor which is running on chrome process. (please see also comment 41)
Attachment #8813551 - Flags: feedback?(bugs)
docshell.allowJavascript = false; is definitely not at all the same as iframe.sandbox = "";
sandbox disables by default many other features too.
https://html.spec.whatwg.org/multipage/browsers.html#sandboxing

And setting allowJavascript to false changes the behavior of the currently loaded page (and pages to be loaded).
setting sandbox attribute changes the behavior of the page to be loaded next.
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

Since I don't know the requirements for devtools, hard to give feedback about the patch itself. But hopefully comment 57 is useful.
Attachment #8813551 - Flags: feedback?(bugs)
Iteration: 53.2 - Dec 12 → 53.3 - Dec 26
(In reply to Frederik Braun [:freddyb] from comment #53)
> IIRC the previous implementations used <iframe mozbrowser>, who wrote that?
> That'd make sense security-wise, though being pretty much non-standard.

Preview panel was introduced in bug 956357 which didn't use <iframe mozbrowser>.


@smaug, thanks for your information.

I just wonder if iframe.sandbox = ""; includes the identical javascript restriction with docshell.allowJavascript = false;

In other words, my question is that is it functional equivalence between 

docshell.allowJavascript = false;

and 

iframe.sandbox = "allows all restrictions except allow-scripts"

I hope above question could be more clear for you :)
There are at least couple of differences.
sandbox = "...everything else but allow-scripts" does still cause SANDBOXED_AUTOMATIC_FEATURES and SANDBOXED_DOMAIN to be set.

You may want to read http://searchfox.org/mozilla-central/source/dom/base/nsSandboxFlags.h and 
http://searchfox.org/mozilla-central/source/dom/base/IframeSandboxKeywordList.h
and check where the relevant flags are used.
SANDBOXED_DOMAIN probably doesn't matter, if scripts are anyhow disabled, but 
SANDBOXED_AUTOMATIC_FEATURES might.
IIRC, both docshell.allowJavascript = false; and iframe.sandbox = "allows all restrictions except allow-scripts"; are able to block script execution literally. 

Moreover, the iframe.sandbox = "allows all restrictions except allow-scripts"; also disable some automatic features.

My conclusion is that iframe.sandbox is able to offer a better protection rather than using docshell.allowJavascript = false;. 

Jarda, I think the current implementation is safe enough. If someone has concerned for security, we can do a workaround by adding mozbrowser to preview webpage on a separate process. And then remove it once netmonitor is able to run on content process.
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #61)
> Jarda, I think the current implementation is safe enough. If someone has
> concerned for security, we can do a workaround by adding mozbrowser to
> preview webpage on a separate process. And then remove it once netmonitor is
> able to run on content process.

So, you want to use sandbox=""? Or to remove some restrictions? The purpose of the Preview feature is to display the HTML data as a static document, without ever executing anything. So, the full set of restrictions (sandbox="") looks more appropriate to me.

By the way, I wouldn't mind at all if the "Preview" feature was removed completely - it seems completely useless to me. Why would anyone want to see a preview of the page's HTML? With no CSS styles, with no scripting... The "Response" tab already contains a preview of the markup, which is sufficient. Chrome also displays the markup in both "Preview" and "Response" tabs.
Flags: needinfo?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #62)
> So, you want to use sandbox=""? Or to remove some restrictions? The purpose
> of the Preview feature is to display the HTML data as a static document,
> without ever executing anything. So, the full set of restrictions
> (sandbox="") looks more appropriate to me.

Yes, the current XUL implementation used sandbox="" as well, although the iframe tag doesn't define xmln properly.

> By the way, I wouldn't mind at all if the "Preview" feature was removed
> completely - it seems completely useless to me. Why would anyone want to see
> a preview of the page's HTML? With no CSS styles, with no scripting... The
> "Response" tab already contains a preview of the markup, which is
> sufficient. Chrome also displays the markup in both "Preview" and "Response"
> tabs.

We still remember that we had some similar discussion on abandoning preview panel since it seems to be useless. I'm fine with that. Let's discuss on this in tomorrow's meeting.
Rebased on m-c and fixed conflicts.
Kindly remind that the patch is almost ready but it's still waiting for review+ flags :)
Flags: needinfo?(odvarko)
Flags: needinfo?(jsnajdr)
Ricky, can you modify your tests to ensure that scripts do not execute?
You could load a document in the preview panel that contains something like this:
>  <img src=x onerror="ok(false, 'Preview should not execute scripts');">

(I'm not using <script> tags because these are not triggered by innerHTML)
Flags: needinfo?(rchien)
I understand adding this is really helpful to ensure that scripts are blocked properly. However, there are two places where we run the document. One runs on website and the other one runs on netmonitor itself. So, I've no idea how to distinct them in mochitest environment.
Flags: needinfo?(rchien)
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review99080

Looks good, thanks!
Attachment #8813551 - Flags: review?(jsnajdr) → review+
Flags: needinfo?(jsnajdr)
Comment on attachment 8813551 [details]
Bug 1317659 - Implement Preview Panel

https://reviewboard.mozilla.org/r/94998/#review99090

Thanks for the patch Ricky!

R+ assuming try is green

Also, as we mentioned at our standup meeting we'll likely remove the Preview panel entirely.
We'll make the decision as par or our UI/UX audit in Januray.

Honza
Attachment #8813551 - Flags: review?(odvarko) → review+
Flags: needinfo?(odvarko)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a1fc85aecac
Implement Preview Panel r=Honza,jsnajdr
https://hg.mozilla.org/mozilla-central/rev/2a1fc85aecac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I can confirm that this issue is verified fixed on latest Nightly 53.0a1 (2016-12-19), under the following OSes:
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS
- Windows 10 x64

Updating the flag accordingly.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: