New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added devtools support to fetch for XHR #12502

Merged
merged 1 commit into from Jul 29, 2016

Conversation

Projects
None yet
6 participants
@avadacatavra
Contributor

avadacatavra commented Jul 19, 2016

Added devtools support for fetch for XHR, but devtools can't show the request in the XHR tab. I've attached a picture of an XHR request in devtools.

screenshot 2016-07-19 11 07 55


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11774 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because I don't know how to automate devtools testing for this

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Jul 19, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

@highfive

This comment has been minimized.

highfive commented Jul 19, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/xmlhttprequest.rs, components/net/fetch/methods.rs, components/net/resource_thread.rs, components/net_traits/request.rs, components/net_traits/request.rs, components/net/http_loader.rs
@highfive

This comment has been minimized.

highfive commented Jul 19, 2016

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!
}

pub fn fetch_with_cors_cache(request: Rc<Request>,
cache: &mut CORSCache,
target: &mut Target,
context: FetchContext) -> Response {
context: FetchContext,
devtools_chan: Option<Sender<DevtoolsControlMsg>>) -> Response {

This comment has been minimized.

@Manishearth

Manishearth Jul 19, 2016

Member

I feel like the devtools_chan can be rolled into the FetchContext

@avadacatavra

This comment has been minimized.

Contributor

avadacatavra commented Jul 19, 2016

CC @jdm

@@ -601,6 +601,7 @@ fn prepare_devtools_request(request_id: String,
now: Tm,
connect_time: u64,
send_time: u64) -> ChromeToDevtoolsControlMsg {
debug!("preparing!!!");

This comment has been minimized.

@Manishearth

Manishearth Jul 19, 2016

Member

might want to clean these up 😄

It's fine to leave debug statements around, but try to have them print something useful, e.g. prepare_devtools_request: <url> or something

This comment has been minimized.

@Manishearth

Manishearth Jul 19, 2016

Member

(This applies to the other debug statements in this PR, too)

@@ -586,7 +587,7 @@ impl CoreResourceManager {
// todo service worker stuff
let mut target = Some(Box::new(sender) as Box<FetchTaskTarget + Send + 'static>);
let context = FetchContext { state: http_state, user_agent: ua };
fetch(Rc::new(request), &mut target, context);
fetch(Rc::new(request), &mut target, context, dc); //FIXME put a devtools channel here

This comment has been minimized.

@Manishearth

Manishearth Jul 19, 2016

Member

fixme was fixed?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 19, 2016

r? @jdm

Overall lgtm, with the few nits I mentioned here and in IRC. I think I've got a fix for the XHR thing ready.

@highfive highfive assigned jdm and unassigned asajeffrey Jul 19, 2016

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 19, 2016

Manishearth@a388da3 should work for getting XHR to work. However, I'm unable to test it, since I can't get any network requests to show in my devtools pane.

@jdm

This comment has been minimized.

Member

jdm commented Jul 19, 2016

Looking good! The unit tests in tests/unit/net/fetch.rs will need to be updated too - ./mach test-unit -p net.
-S-awaiting-review +S-tests-failed +S-needs-code-changes


Reviewed 5 of 5 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 10 unresolved discussions.


components/net/fetch/methods.rs, line 961 [r1] (raw file):

    let cancellation_listener = CancellationListener::new(None);

    let request_id = "";

We should generate an ID like https://dxr.mozilla.org/servo/rev/5f702d507c908348382d376f43e3cfc08094b517/components/net/http_loader.rs#968


components/net/fetch/methods.rs, line 984 [r1] (raw file):

            let done_sender = done_chan.as_ref().map(|ch| ch.0.clone());
            let devtools_sender = devtools_chan.clone();
            let metadata = meta.clone();

Let's clone the fields we actually need, instead. Then we don't need to clone them later.


components/net/fetch/methods.rs, line 1022 [r1] (raw file):

                        }
                        if let Some(m) = msg {
                            send_request_to_devtools(m, devtools_sender.as_ref().unwrap());

Let's do this before the loop.


components/net/fetch/methods.rs, line 1032 [r1] (raw file):

                            devtools_sender, request_id.into(),
                            metadata.headers.clone(), metadata.status.clone(),
                            pipeline_id);

nit: the arguments should be indented one level.


components/net/fetch/methods.rs, line 1052 [r1] (raw file):

        }
    };

nit: remove this extra newline.


components/net_traits/request.rs, line 288 [r1] (raw file):

            redirect_count: Cell::new(0),
            response_tainting: Cell::new(ResponseTainting::Basic),
            pipeline_id: Cell::new(Some(PipelineId::new())),

We should pass the pipeline id in as an argument, instead.


components/script/dom/xmlhttprequest.rs, line 609 [r1] (raw file):

            referer_url: self.referrer_url.clone(),
            referrer_policy: self.referrer_policy.clone(),
            pipeline_id: self.pipeline_id().clone(),

This clone shouldn't be necessary.


Comments from Reviewable

@jdm

This comment has been minimized.

Member

jdm commented Jul 22, 2016

-S-awaiting-review +S-needs-code-changes +S-fails-travis


Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


components/net/fetch/methods.rs, line 1095 [r3] (raw file):

    // Step 1
    let mut preflight = Request::new(request.current_url(), Some(request.origin.borrow().clone()), 
                                     false, Some(PipelineId::new()));

The pipeline id should be passed in as an argument instead.


Comments from Reviewable

@avadacatavra

This comment has been minimized.

Contributor

avadacatavra commented Jul 23, 2016

@jdm I was confused on what you meant by "Let's clone the fields we actually need, instead. Then we don't need to clone them later."

will be adding the unit tests today/tomorrow, and I put @Manishearth's fix for identifying XHR in

@jdm

This comment has been minimized.

Member

jdm commented Jul 28, 2016

Almost there! Also, there are a couple remaining debug! uses in http_loader.rs that were added in this PR; let's remove them.
-S-awaiting-review +S-needs-code-changes


Reviewed 1 of 1 files at r8, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


components/net/http_loader.rs, line 632 [r8] (raw file):

                             status: Option<RawStatus>,
                             pipeline_id: PipelineId) {
    if devtools_chan.is_none() {

I'd prefer to revert this change; the previous interface that accepted a channel worked fine.


components/net/http_loader.rs, line 1099 [r8] (raw file):

        // TODO: Send this message even when the load fails?
        if let Some(pipeline_id) = load_data.pipeline_id {
           // if let Some(ref chan) = devtools_chan {

Revert this, please.


Comments from Reviewable

@avadacatavra

This comment has been minimized.

Contributor

avadacatavra commented Jul 28, 2016

WTF I know I removed those!


Review status: all files reviewed at latest revision, 6 unresolved discussions.


tests/unit/net/http_loader.rs, line 40 [r5] (raw file):

Previously, jdm (Josh Matthews) wrote…

Why are all of these types and methods being made public? Many don't seem to be used by the fetch tests.

Done.

Comments from Reviewable

@avadacatavra avadacatavra force-pushed the avadacatavra:devtools branch from fbad459 to e2d1a00 Jul 28, 2016

@avadacatavra avadacatavra force-pushed the avadacatavra:devtools branch from e2d1a00 to 17da4ac Jul 28, 2016

@jdm

This comment has been minimized.

Member

jdm commented Jul 28, 2016

-S-awaiting-review -S-fails-travis +S-needs-code-changes


Reviewed 2 of 2 files at r10.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


components/net/http_loader.rs, line 627 [r10] (raw file):

}

pub fn send_response_to_devtools(devtools_chan: Sender<DevtoolsControlMsg>,

&?


components/net/http_loader.rs, line 1094 [r10] (raw file):

            if let Some(ref chan) = devtools_chan {
                send_response_to_devtools(
                    chan.clone(), request_id,

This clone shouldn't be necessary.


components/net/fetch/methods.rs, line 991 [r10] (raw file):

                        if let Some(ref sender) = devtools_sender {
                            if let Some(m) = msg {
                                send_request_to_devtools(m, &sender.clone());

This clone shouldn't be necessary.


components/net/fetch/methods.rs, line 998 [r10] (raw file):

                            if let Some(pipeline_id) = pipeline_id {
                                send_response_to_devtools(
                                    sender.clone(), request_id.into(),

This clone shouldn't be necessary.


Comments from Reviewable

Added devtools support to fetch for XHR + Manish's XHR ident fix
added unit test for request fetch with devtools

added devtools/fetch test

@avadacatavra avadacatavra force-pushed the avadacatavra:devtools branch from 17da4ac to db808ca Jul 29, 2016

@jdm

This comment has been minimized.

Member

jdm commented Jul 29, 2016

@bors-servo: r+


Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


Comments from Reviewable

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

📌 Commit db808ca has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

⌛️ Testing commit db808ca with merge b5fe7db...

bors-servo added a commit that referenced this pull request Jul 29, 2016

Auto merge of #12502 - avadacatavra:devtools, r=jdm
Added devtools support to fetch for XHR

<!-- Please describe your changes on the following line: -->

Added devtools support for fetch for XHR, but devtools can't show the request in the XHR tab. I've attached a picture of an XHR request in devtools.

<img width="694" alt="screenshot 2016-07-19 11 07 55" src="https://cloud.githubusercontent.com/assets/11877868/16944480/210b0e8a-4da1-11e6-8288-48005ede33f6.png">

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11774  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because I don't know how to automate devtools testing for this

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12502)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 29, 2016

@bors-servo bors-servo merged commit db808ca into servo:master Jul 29, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment