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.

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

Implemented `GetCachedMessages` #6168

Merged
merged 1 commit into from May 26, 2015
Merged

Implemented `GetCachedMessages` #6168

merged 1 commit into from May 26, 2015

Conversation

@tamird
Copy link
Contributor

tamird commented May 23, 2015

Rebase of #4175, closes #4175. r? @jdm

Review on Reviewable

@highfive
Copy link

highfive commented May 23, 2015

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

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 23, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5087

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member

jdm commented May 23, 2015

Ooh, thanks! I'll take a look through this on Monday!

@bors-servo
Copy link
Contributor

bors-servo commented May 25, 2015

The latest upstream changes (presumably #6125) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented May 25, 2015

-S-awaiting-review +S-needs-code-changes +S-needs-rebase


Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/devtools/actors/console.rs @ r1
  • components/devtools_traits/lib.rs @ r1
  • components/script/devtools.rs @ r1
  • components/script/script_task.rs @ r1

components/devtools/actors/console.rs, line 90 [r1] (raw file):
I 'd prefer to leave the message type string match in this code, and only send the script task an enum value.


components/devtools_traits/lib.rs, line 159 [r1] (raw file):
No need for the Message suffix on these variants. I'd also prefer to leave these definitions in devtools/ and use simpler types when possible for sending between the script and devtools tasks. For example, we could re-use the ConsoleMessage type directly above this.


components/devtools_traits/lib.rs, line 182 [r1] (raw file):
Vec, I believe.


components/script/devtools.rs, line 144 [r1] (raw file):
Let's just return an empty vector here.


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor Author

tamird commented May 25, 2015

Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.


components/devtools/actors/console.rs, line 90 [r1] (raw file):
Sounds good.


components/devtools_traits/lib.rs, line 159 [r1] (raw file):
How can we use a simpler type here? These definitions are from https://developer.mozilla.org/en-US/docs/Tools/Web_Console/remoting.

How do you decide where to move these types? I moved them because it saved me an import statement and they aren't currently used. The ConsoleMessage type above doesn't conform to what we need here (there seems to be a bit of a mess here, since there is also ConsoleMsg, yet another type).


components/script/devtools.rs, line 144 [r1] (raw file):
We could, but I wanted the commented out code to continue to be carried forward in case type changes are made (i.e. it will still compile).


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor Author

tamird commented May 25, 2015

Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.


components/devtools_traits/lib.rs, line 182 [r1] (raw file):
Right, but a vec of what?


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 25, 2015

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.


components/devtools_traits/lib.rs, line 159 [r1] (raw file):
You're right; I was confused. These can stay where they are.


components/devtools_traits/lib.rs, line 182 [r1] (raw file):
Github ate my angle brackets: Vec<String>.


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor Author

tamird commented May 25, 2015

Review status: 1 of 6 files reviewed, 1 unresolved discussion, all commit checks successful.


components/devtools/actors/console.rs, line 90 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 26, 2015

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


Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.
Reviewed files:

  • components/devtools/actors/console.rs @ r2
  • components/devtools_traits/Cargo.toml @ r2
  • components/devtools_traits/lib.rs @ r2
  • components/script/devtools.rs @ r2
  • components/servo/Cargo.lock @ r2

components/devtools/actors/console.rs, line 91 [r2] (raw file):
I'd prefer to move this into one or two separate bindings, so we just get something like for str_type in &msg_types instead.


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor Author

tamird commented May 26, 2015

Review status: all files reviewed, 1 unresolved discussion, all commit checks successful.


components/devtools/actors/console.rs, line 91 [r2] (raw file):
Done.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 26, 2015

Go ahead and squash these together!
-S-awaiting-review +S-needs-squash -S-needs-rebase


Review status: :shipit: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

  • components/devtools/actors/console.rs @ r3

Comments from the review on Reviewable.io

@tamird
Copy link
Contributor Author

tamird commented May 26, 2015

Done!


Review status: all files reviewed, all discussions resolved, some commit checks pending.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 26, 2015

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

📌 Commit b5f74eb has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

Testing commit b5f74eb with merge 8759d42...

bors-servo pushed a commit that referenced this pull request May 26, 2015
Rebase of #4175, closes #4175. r? @jdm

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

bors-servo commented May 26, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit b5f74eb into servo:master May 26, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@tamird tamird deleted the tamird:get-cached-messages branch May 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.