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

Adding a new command line argument e10s profile #51

Merged
merged 1 commit into from Mar 10, 2016

Conversation

martiansideofthemoon
Copy link
Contributor

@jgraham this is the first half of the first part of #43 . Is it alright? I've added a new command line option.


This change is Review on Reviewable

@hoppipolla-critic-bot
Copy link

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

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.

@jgraham
Copy link
Member

jgraham commented Mar 9, 2016

You need to also implement the code that reads this pref and starts Fx with e10s enabled as part of this PR.


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


src/main.rs, line 84 [r1] (raw file):
Remove debugging code.


Comments from the review on Reviewable.io

@jgraham
Copy link
Member

jgraham commented Mar 9, 2016

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


src/main.rs, line 13 [r2] (raw file):
I think this is unused?


src/main.rs, line 75 [r2] (raw file):
Call this --e10s


src/marionette.rs, line 156 [r2] (raw file):
I'm pretty sure you just want to set browser.tabs.remote.autostart (but you want to set .2 to false to ensure e10s is disabled). From the code it looks like .1 doesn't exist anymore, the unsuffixed version represents user opt-in and .2 represents the default value (https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4726)


Comments from the review on Reviewable.io

@martiansideofthemoon
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


src/marionette.rs, line 156 [r2] (raw file):
Haven't I added it a few lines below in a new array NON_E10S_PREFERENCES ?


Comments from the review on Reviewable.io

@jgraham
Copy link
Member

jgraham commented Mar 9, 2016

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


src/marionette.rs, line 156 [r2] (raw file):
My point is that there's too much here, and not enough below. It should be something like:

pub static E10S_PREFERENCES: [(&'static str, PrefValue); 1] = [
    ("browser.tabs.remote.autostart", PrefValue::PrefBool(true)),
];

pub static NON_E10S_PREFERENCES: [(&'static str, PrefValue); 2] = [
    ("browser.tabs.remote.autostart", PrefValue::PrefBool(false)),
    ("browser.tabs.remote.autostart.2", PrefValue::PrefBool(false))
];

Comments from the review on Reviewable.io

@jgraham
Copy link
Member

jgraham commented Mar 10, 2016

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

jgraham added a commit that referenced this pull request Mar 10, 2016
Adding a new command line argument e10s profile
@jgraham jgraham merged commit 5073da8 into mozilla:master Mar 10, 2016
@martiansideofthemoon martiansideofthemoon deleted the bulbasaur branch March 10, 2016 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants