create a config facade #15919

karan1276 · 2017-03-12T05:10:29Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15460(github issue number if applicable).
These changes do not require tests because no new functionality is added. Existing test should cover all the changes.

This change is

highfive · 2017-03-12T05:10:35Z

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

highfive · 2017-03-12T05:10:35Z

Heads up! This PR modifies the following files:

@bholley: components/style/lib.rs, components/style/servo_config_facade.rs, components/style/stylesheets.rs, components/style/traversal.rs, components/style/context.rs, components/style/matching.rs
@emilio: components/style/lib.rs, components/style/servo_config_facade.rs, components/style/stylesheets.rs, components/style/traversal.rs, components/style/context.rs, components/style/matching.rs

highfive · 2017-03-12T05:10:36Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

karan1276 · 2017-03-12T05:15:29Z

./mach build-geckolib --verbose gives an error i am not able to debug, it says:
error: could not execute process rustc -vV (never executed)

If someone can help that will be great

canova

It looks like the problem is the use statement as I mentioned in the comment below.
Also nox suggested to make this name simply config instead of servo_config_facade. Could you change this?

canova · 2017-03-15T13:03:47Z

components/style/servo_config_facade.rs

+ /* Ref: https://github.com/servo/servo/issues/15460 for more info for why it's needed. */
+
+// TODO: write docs for this mod
+use servo_config;


You've changed the extern crate servo_config; to #[cfg(feature = "servo")] extern crate servo_config; in components/style/lib.rs.
In gecko build it tries to find this crate but fails. You can either add a cfg to this use statement or you can simply add related use statements inside servo functions.(as Manishearth wrote in issue example.)

aah, that explains it. I think ill add an cfg. Just curious, how did you debug that because error did not log anything

rustc should give you an error like this when you run ./mach build-geckolib:

error[E0432]: unresolved import `servo_config` --> /Users/canova/projects/servo/components/style/config.rs:11:5 | 11 | use servo_config; | ^^^^^^^^^^^^ no `servo_config` in the root error: aborting due to previous error

What OS do you use? If this didn't show up, maybe it was a rustc bug?

canova · 2017-03-15T13:03:47Z

components/style/servo_config_facade.rs

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+ /* Ref: https://github.com/servo/servo/issues/15460 for more info for why it's needed. */


I think adding a brief explanation about this module with //! comments would be better instead.

KiChjang · 2017-03-15T17:29:55Z

I'm still not comfortable calling it a facade.

karan1276 · 2017-03-17T04:12:51Z

@KiChjang I am fine calling it anything. Maybe you and @Manishearth can talk and come to a conclusion ?

KiChjang · 2017-03-17T07:52:17Z

In the issue, @nox suggested to call it config instead, and there seemed to be no objections from anyone.

karan1276 · 2017-03-17T08:31:13Z

Okay, cool 👍

canova · 2017-03-17T16:09:01Z

components/style/config.rs

+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+ /*
+ * servo_config is a crate in components/config that pulls in a lot of random stuff. In gecko mode,


Rustdoc uses //! for module or crate level documentation. Could you convert /* .. */ to that please? With this change you can remove the #[allow(missing_docs)] attribute in the
#[allow(missing_docs)] pub mod config; line. Also we will be able to see this comment in the doc.servo.org after this.

nit: also it would be wonderful if you can shorten these lines to match license length while you're here.

canova · 2017-03-17T16:09:01Z

components/style/servo_config_facade.rs

+ /* Ref: https://github.com/servo/servo/issues/15460 for more info for why it's needed. */
+
+// TODO: write docs for this mod
+use servo_config;


rustc should give you an error like this when you run ./mach build-geckolib:

error[E0432]: unresolved import `servo_config` --> /Users/canova/projects/servo/components/style/config.rs:11:5 | 11 | use servo_config; | ^^^^^^^^^^^^ no `servo_config` in the root error: aborting due to previous error

What OS do you use? If this didn't show up, maybe it was a rustc bug?

canova · 2017-03-17T16:11:00Z

components/style/config.rs

+#[cfg(feature = "servo")] use servo_config;
+
+// prefs functions
+#[cfg(feature = "servo")] #[allow(missing_docs)]


nit: I'm not comfortable with these #[allow(missing_docs)] attributes but it's OK, I guess. At least we can move them to a separate line maybe?

bors-servo · 2017-03-19T22:12:33Z

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

cbrewster · 2017-03-24T02:08:49Z

This looks good, maybe address @canaltinova's comment about either documenting the settings or moving the no_doc to a new line
There some tidy failures:

Checking files for tidiness...
./components/style/matching.rs:27: use statement is not in alphabetical order
	expected: selectors::matching::AFFECTED_BY_PSEUDO_ELEMENTS
	found: config
./components/style/stylesheets.rs:22: use statement is not in alphabetical order
	expected: selectors::parser::SelectorList
	found: config
./components/style/traversal.rs:16: use statement is not in alphabetical order
	expected: selector_parser::RestyleDamage
	found: config

KiChjang · 2017-03-24T10:40:56Z

Please also remove the merge commit.

karan1276 · 2017-03-24T10:55:23Z

Actually i tried to rebase 2-3 times but always mess it up. There are conflicts at same line for multiple commits. It gets really difficult to remove just the right change.

merge on the other hand just compares final results and points out all conflicts in one go. If it's not really a big issue i would suggest we go for a merge here.

wafflespeanut · 2017-03-25T04:07:54Z

No, we don't allow merge commits in general because it's a result of an improper (or absence of) rebase. If you want help with it, please ask.

karan1276 · 2017-03-25T05:27:50Z

Yes it will be awesome if you can help.

canova · 2017-03-25T16:09:50Z

https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing

I think this guide is very good for explaining how to do rebase and squash. But for this specific situation here's how I would do it:

First you need to delete this merge commit and squash others with interactive rebase:
git rebase -ip HEAD~9
-i tells that you want to use interactive rebase, and -p tells to preserve merge commits.
This will open an editor that you can manipulate the commits. You can delete the merge commit with deleting its line. And you should write the fixup or f keyword instead of pick to other commits except the first commit. Fixup will merge the commits into previous ones and they will become one.
When you save and exit, rebase will take care of it. And you will have just one commit. After that, you will need to fetch and rebase again like this:
git pull upstream master --rebase (you should replace upstream with your servo/servo remote name)
During rebase, some conflicts will happen. You should resolve the conflicts and git add <file> them. After that, you can write git rebase --continue and you are done.

Let us know if you still have questions. Or we can do it for you if you check the "Allow edits from maintainers" checkbox.

karan1276 · 2017-03-27T09:33:42Z

@canaltinova Thanks! That's a good strategy. I won't follow it exactly but i will first squash and then rebase. I don't want to do them together because i suspect squash will have it's own conflicts(maybe not, not sure) and then rebase will defiantly have conflicts. And it's easier for me to handle them separately.

Thanks again

canova · 2017-03-27T15:51:40Z

Yes, you can probably try to delete merge commit with git rebase -ip HEAD~2 first and then proceed to squash them. I don't think you will have merge conflicts during squash but it would be easier.

bors-servo · 2017-04-03T12:48:28Z

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

jdm · 2017-04-04T10:13:50Z

./mach test-tidy says:

./components/style/matching.rs:26: use statement is not in alphabetical order
	expected: selectors::matching::AFFECTED_BY_PSEUDO_ELEMENTS
	found: config
./components/style/stylesheets.rs:31: use statement is not in alphabetical order
	expected: selectors::parser::SelectorList
	found: config
./components/style/traversal.rs:16: use statement is not in alphabetical order
	expected: selector_parser::RestyleDamage
	found: config

bors-servo · 2017-04-07T10:11:21Z

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

karan1276 · 2017-04-07T11:26:03Z

These conflicts will pop up every time there is a merge in master. I would resolve them at the end just before this PR gets merged. Are there any other outstanding issues except for the conflicts?

cbrewster · 2017-04-07T13:15:18Z

this looks good if you address @canaltinova's comment about converting /* ... */ to //! and rebase/squash


        Change code for serialization for {box,text}-shadow so blur-radius

can be parsed as non-negavite


        conflict patch

karan1276 · 2017-04-11T09:02:45Z

some changes in #16297 are pretty major, ill remove conflicts but be cautious while reviewing

canova

Oh, it looks like c857878 implemented something like this. Sorry about that :/ I don't know whether we should still continue to this or not.

canova · 2017-04-11T14:49:38Z

components/style/context.rs

 impl TraversalStatistics {
    /// Returns whether statistics dumping is enabled.
-    pub fn should_dump() -> bool {


It looks like you removed the function header here.

canova · 2017-04-11T14:49:38Z

components/style/context.rs

 impl TraversalStatistics {
    /// Returns whether statistics dumping is enabled.
-    pub fn should_dump() -> bool {
-        shall_stat_style_sharing()
+        *DUMP_STYLE_STATISTICS || config::style_sharing_stats_enabled()


This looks wrong. Currently servo returns opts::get().style_sharing_stats and gecko returns *DUMP_STYLE_STATISTICS. We need to preserve that behavior. You should remove the *DUMP_STYLE_STATISTICS || from here and return *DUMP_STYLE_STATISTICS in gecko's style_sharing_stats_enabled function.

canova · 2017-04-11T14:49:38Z

components/style/matching.rs

        if is_share_style_cache_disabled() {
+=======
+        if config::disable_share_style_cache_enabled() {
+>>>>>>> Change code for serialization for {box,text}-shadow so blur-radius


There is a merge conflict here.

canova · 2017-04-11T14:49:38Z

components/style/traversal.rs


-        if traversal_flags.for_reconstruct() {
+        if cfg!(feature = "servo") && config::nonincremental_layout_enabled() {


Why did you remove traversal_flags.for_reconstruct()?

bors-servo · 2017-04-12T04:49:10Z

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

karan1276 · 2017-04-12T05:55:22Z

Oh, it looks like c857878 implemented something like this. Sorry about that :/ I don't know whether we should still continue to this or not.

Yup, you are probably right. Maybe you can tell me for sure if this pr is needed anymore ? If not then lets just shut it right away, why spend more time.

cbrewster · 2017-04-22T03:35:35Z

Closing as it looks like this is already implemented.

highfive assigned cbrewster Mar 12, 2017

highfive added the S-awaiting-review label Mar 12, 2017

nox added the S-needs-squash label Mar 13, 2017

canova reviewed Mar 15, 2017

View changes

cbrewster added S-needs-code-changes and removed S-awaiting-review labels Mar 15, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 17, 2017

canova reviewed Mar 17, 2017

View changes

canova added S-needs-code-changes and removed S-awaiting-review labels Mar 17, 2017

highfive added S-needs-rebase S-awaiting-review and removed S-needs-code-changes labels Mar 19, 2017

cbrewster added S-fails-tidy S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Mar 24, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 24, 2017

cbrewster removed the S-awaiting-review label Mar 29, 2017

highfive added the S-needs-rebase label Apr 3, 2017

karan1276 force-pushed the karan1276:karan_config_facade branch from cb93038 to 978f945 Apr 4, 2017

highfive added the S-awaiting-review label Apr 4, 2017

highfive added the S-needs-rebase label Apr 7, 2017

cbrewster removed the S-awaiting-review label Apr 10, 2017

karan1276 added 2 commits Feb 15, 2017

Change code for serialization for {box,text}-shadow so blur-radius

91fca28

can be parsed as non-negavite

conflict patch

Loading status checks…

8ad84f7

karan1276 force-pushed the karan1276:karan_config_facade branch from 6a1caa4 to 8ad84f7 Apr 11, 2017

highfive added the S-awaiting-review label Apr 11, 2017

canova requested changes Apr 11, 2017

View changes

highfive added the S-needs-rebase label Apr 12, 2017

cbrewster closed this Apr 22, 2017


		if traversal_flags.for_reconstruct() {
		if cfg!(feature = "servo") && config::nonincremental_layout_enabled() {

servo / servo

Join GitHub today

create a config facade #15919

create a config facade #15919

Conversation

karan1276 commented Mar 12, 2017 • edited

highfive commented Mar 12, 2017

highfive commented Mar 12, 2017

highfive commented Mar 12, 2017

karan1276 commented Mar 12, 2017 • edited

canova left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Mar 15, 2017

karan1276 commented Mar 17, 2017

KiChjang commented Mar 17, 2017

karan1276 commented Mar 17, 2017

This comment has been minimized.

canova Mar 17, 2017 • edited

This comment has been minimized.

This comment has been minimized.

bors-servo commented Mar 19, 2017

cbrewster commented Mar 24, 2017

KiChjang commented Mar 24, 2017

karan1276 commented Mar 24, 2017

wafflespeanut commented Mar 25, 2017

karan1276 commented Mar 25, 2017

canova commented Mar 25, 2017

karan1276 commented Mar 27, 2017

canova commented Mar 27, 2017

bors-servo commented Apr 3, 2017

jdm commented Apr 4, 2017

bors-servo commented Apr 7, 2017

karan1276 commented Apr 7, 2017

cbrewster commented Apr 7, 2017

karan1276 commented Apr 11, 2017

canova left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Apr 12, 2017

karan1276 commented Apr 12, 2017

cbrewster commented Apr 22, 2017

karan1276 commented Mar 12, 2017 •

edited

karan1276 commented Mar 12, 2017 •

edited

canova Mar 17, 2017 •

edited