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

add reload keyboard shortcut #11735

Merged
merged 1 commit into from Jun 23, 2016
Merged

add reload keyboard shortcut #11735

merged 1 commit into from Jun 23, 2016

Conversation

@mrmiywj
Copy link
Contributor

mrmiywj commented Jun 13, 2016


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11686 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because this cannot be automated tested.

This change is Reviewable

@highfive
Copy link

highfive commented Jun 13, 2016

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @KiChjang: components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@highfive
Copy link

highfive commented Jun 13, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm jdm added the S-fails-tidy label Jun 13, 2016
@KiChjang
Copy link
Member

KiChjang commented Jun 13, 2016

This fails tidy and is missing the preferences that @paulrouget specified in the issue.

@cbrewster
Copy link
Member

cbrewster commented Jun 13, 2016

From travis:

Checking files for tidiness...
./components/script/script_thread.rs:2054: use statement is not in alphabetical order
    expected: dom::bindings::codegen::Bindings::WindowBinding::WindowMethods
    found: dom::bindings::codegen::Bindings::LocationBinding::LocationMethods
@highfive
Copy link

highfive commented Jun 13, 2016

New code was committed to pull request.

@mrmiywj
Copy link
Contributor Author

mrmiywj commented Jun 13, 2016

@KiChjang Sorry for that I missed this tidy

@mrmiywj
Copy link
Contributor Author

mrmiywj commented Jun 13, 2016

The preference has been renamed and guarded the key shortcut.

@cbrewster
Copy link
Member

cbrewster commented Jun 13, 2016

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.


components/script/script_thread.rs, line 2053 [r1] (raw file):

    fn handle_reload(&self, pipeline_id: PipelineId) {
        use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;

nit: could we move these use statements to the top of the file?


Comments from Reviewable

@mrmiywj
Copy link
Contributor Author

mrmiywj commented Jun 13, 2016

@ConnorGBrewster I used to write use statements on the top of file, but I'm not sure whether it will dirty the namespace. So I used it in this clause.

@highfive
Copy link

highfive commented Jun 13, 2016

New code was committed to pull request.

@mrmiywj
Copy link
Contributor Author

mrmiywj commented Jun 21, 2016

@jdm -reply

@jdm
Copy link
Member

jdm commented Jun 21, 2016

My apologies for the delay - I was at an event all last week. I'll review this today.

@@ -822,7 +822,7 @@ impl WindowMethods for Window {
}

(NONE, Key::Escape) => {
if let Some(true) = prefs::get_pref("shell.quit-on-escape.enabled").as_boolean() {
if let Some(true) = prefs::get_pref("shell.builtin-key-shortcuts.enable").as_boolean() {

This comment has been minimized.

Copy link
@paulrouget

paulrouget Jun 21, 2016

Contributor

s/enable/enabled

@jdm
Copy link
Member

jdm commented Jun 22, 2016

This looks fine; just a few small changes required to merge. Thanks for doing this work!
-S-awaiting-review +S-needs-code-changes


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


components/constellation/constellation.rs, line 1399 [r1] (raw file):

        match root_pipeline_id {
            Some(pipeline_id) => {

This can be an if let instead of a match.


components/script/script_thread.rs, line 2055 [r1] (raw file):

        use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
        use dom::bindings::codegen::Bindings::LocationBinding::LocationMethods;
        let context = self.browsing_context.get().unwrap().find(pipeline_id).expect("There is no such context");

Let's use self.find_child_context(pipeline_id) instead, and just return if it can't be found.


components/script_traits/lib.rs, line 192 [r1] (raw file):

    /// Report an error from a CSS parser for the given pipeline
    ReportCSSError(PipelineId, String, usize, usize, String),
    /// Reload the current page.

Reload the given page


Comments from Reviewable

@mrmiywj
Copy link
Contributor Author

mrmiywj commented Jun 23, 2016

@jdm -reply

@paulrouget
Copy link
Contributor

paulrouget commented Jun 23, 2016

You also need to change quit-on-escape to builtin-key-shortcuts in:

  • python/servo/package_commands.py
  • python/servo/post_build_commands.py
rename the preference to shell.builtin-key-shortcuts.enabled
@jdm
Copy link
Member

jdm commented Jun 23, 2016

@mrmiywj Just so you know, when you push new changes to a branch there's a github notification and emails that are sent out. You don't need to explicitly do the "-reply" thing each time.

@jdm
Copy link
Member

jdm commented Jun 23, 2016

@bors-servo: r+
Thanks @mrmiywj!


Reviewed 4 of 4 files at r4, 8 of 8 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2016

📌 Commit 909f0da has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2016

Testing commit 909f0da with merge eeed5b6...

bors-servo added a commit that referenced this pull request Jun 23, 2016
add reload keyboard shortcut

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

---
<!-- 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 #11686  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because this cannot be automated tested.

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11735)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2016

@bors-servo bors-servo merged commit 909f0da into servo:master Jun 23, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jun 23, 2016
4 of 4 tasks complete
@paulrouget paulrouget mentioned this pull request Jun 24, 2016
3 of 5 tasks complete
bors-servo added a commit that referenced this pull request Jun 24, 2016
Fix preference name

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

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because not testable

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

Follow up of #11735. `.enabled` is missing in the python scripts.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11849)
<!-- Reviewable:end -->
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…date); r=jdm

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

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because not testable

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

Follow up of servo/servo#11735. `.enabled` is missing in the python scripts.

Source-Repo: https://github.com/servo/servo
Source-Revision: 86d65b60647c185e656e3ae607a3762e667974ad

UltraBlame original commit: 57b84af25bae5af4b5f407f74017319d5695d829
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…date); r=jdm

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

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because not testable

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

Follow up of servo/servo#11735. `.enabled` is missing in the python scripts.

Source-Repo: https://github.com/servo/servo
Source-Revision: 86d65b60647c185e656e3ae607a3762e667974ad

UltraBlame original commit: 57b84af25bae5af4b5f407f74017319d5695d829
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…date); r=jdm

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

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because not testable

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

Follow up of servo/servo#11735. `.enabled` is missing in the python scripts.

Source-Repo: https://github.com/servo/servo
Source-Revision: 86d65b60647c185e656e3ae607a3762e667974ad

UltraBlame original commit: 57b84af25bae5af4b5f407f74017319d5695d829
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.

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