Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix for Layout memory reporter uses pre-redirect url #8903

Merged
merged 3 commits into from Dec 16, 2015
Merged

fix for Layout memory reporter uses pre-redirect url #8903

merged 3 commits into from Dec 16, 2015

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Dec 9, 2015

Rebase of #7612.

Fixes #6872.

Review on Reviewable

@highfive
Copy link

highfive commented Dec 9, 2015

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 9, 2015
@KiChjang KiChjang mentioned this pull request Dec 9, 2015
Closed
@KiChjang
Copy link
Contributor Author

KiChjang commented Dec 9, 2015

r? @nox

@KiChjang
Copy link
Contributor Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 2ee446d with merge 6692e96...

bors-servo pushed a commit that referenced this pull request Dec 10, 2015
Auto merge of #8903 - KiChjang:layout-reporter-redirect, r=<try>
6692e96 
fix for Layout memory reporter uses pre-redirect url

Rebase of #7612.

Fixes #6872.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8903)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@nox
Copy link
Contributor

nox commented Dec 10, 2015

Apart from my question to @jdm, this looks good and I'll r+ if he is OK with this patch.

-S-awaiting-review +S-awaiting-answer

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/layout/layout_task.rs, line 123 [r2] (raw file):
@jdm wanted an Option<Url> inside here, and an initial value of None, because the document URL should be set only once (in Layout), did that change? @jdm, care to chime in?

Comments from the review on Reviewable.io

@nox nox added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 10, 2015
@jdm
Copy link
Member

jdm commented Dec 16, 2015

I think this solution is ok. We can change it later if it turns out it causes problems or is confusing.

@jdm
Copy link
Member

jdm commented Dec 16, 2015

@bors-servo: r=nox

@bors-servo
Copy link
Contributor

📌 Commit 2ee446d has been approved by nox

@bors-servo
Copy link
Contributor

⌛ Testing commit 2ee446d with merge bc71583...

bors-servo pushed a commit that referenced this pull request Dec 16, 2015
Auto merge of #8903 - KiChjang:layout-reporter-redirect, r=nox
bc71583 
fix for Layout memory reporter uses pre-redirect url

Rebase of #7612.

Fixes #6872.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8903)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-answer Someone asked a question that requires an answer. labels Dec 16, 2015
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo mentioned this pull request Dec 16, 2015
Closed
@bors-servo bors-servo merged commit 2ee446d into servo:master Dec 16, 2015
@KiChjang KiChjang deleted the layout-reporter-redirect branch December 22, 2015 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@KiChjang @highfive @bors-servo @nox @jdm @amarant