Replace side-effecting unwrap_or_else by if let in constellation.

[asajeffrey]

This addresses @nox's comments in #10295.

---

This change is

[nox]

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

---

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

---

components/compositing/constellation.rs, line 717 [r1] (raw file):
This code isn't the same, now you don't call anymore handle_send_error when the pipeline couldn't be found at all. I guess that's intended though?

In that case, I would do:

if let Some(pipeline) = self.pipelines.get(&pipeline_id) {
    if let Err(e) = pipeline.script_chan.send(msg) {
        self.handle_send_error(pipeline_id, e);
    }
} else {
    debug!(...);
    return true;
}

---

components/compositing/constellation.rs, line 729 [r1] (raw file):
I don't hate all combinators in all circumstances, the map_or is fine here. :)

---

components/compositing/constellation.rs, line 908 [r1] (raw file):
Just rewrite the whole function:

if let Some(pipeline) = self.pipelines.get_mut(&pipeline_id) {
    pipeline.size = Some(*size);
    if let Err(e) = pipeline.script_chan.send(msg) {
        self.handle_send_error(pipeline_id, e);
    }
}

---

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

if let Some(pipeline) = self.pipelines.get(&subframe_parent_id) {
    if let Err(e) = pipeline.script_chan.send(msg) {
        self.handle_send_error(subframe_parent_id, e);
    }
} else {
    debug!("Pipeline {:?} subframe loaded after closure.", subframe_parent_id)
    return;
}

---

components/compositing/constellation.rs, line 1030 [r1] (raw file):
Again, it seems to me that you can just rewrite the whole function:

let pipeline = match self.pipelines.get(&pipeline_id) {
    Some(pipeline) => pipeline,
    None => {
        debug!(...);
        return;
    }
};

let result = match tick_type {
    AnimationTickType::Script => {
        pipeline.script_chan.0.send(ConstellationControlMsg::TickAllAnimations(pipeline_id))
    },
    AnimationTickType::Layout => {
        pipeline.layout_chan.0.send(LayoutControlMsg::TickAnimations)
    }
};

if let Err(e) = result {
    self.handle_send_error(pipeline_id, e);
}

I really think we should have a specific function on pipelines, which would take a channel and a message and would call handle_send_error itself. We don't even distinguish whether it's script_chan or layout_chan that encountered an error in this case.

---

components/compositing/constellation.rs, line 1042 [r1] (raw file):
Why not just:

let (parent_info, window_size) =
    self.pipelines.get(&source_id)
        .map(|source| (source.parent_info, source.size))
        .unwrap_or((None, None));

---

components/compositing/constellation.rs, line 1058 [r1] (raw file):
As usual, I think this idiom is better conveyed as two nested if let without intermediate results.

---

components/compositing/constellation.rs, line 1083 [r1] (raw file):
I preferred the original way, where both parent_info and window_size were bound at the same moment.

---

components/compositing/constellation.rs, line 1235 [r1] (raw file):
As usual, I think this idiom is better conveyed as two nested if let without intermediate results.

---

components/compositing/constellation.rs, line 1253 [r1] (raw file):
An if let expression here would decrease the indentation, and thus look nicer.

---

components/compositing/constellation.rs, line 1262 [r1] (raw file):
As usual, I think this idiom is better conveyed as two nested if let without intermediate results.

---

components/compositing/constellation.rs, line 1338 [r1] (raw file):
As usual, I think this idiom is better conveyed as two nested if let without intermediate results.

---

components/compositing/constellation.rs, line 1411 [r1] (raw file):
As usual, I think this idiom is better conveyed as two nested if let without intermediate results.

---

components/compositing/constellation.rs, line 1431 [r1] (raw file):
What happens if that one send call fails, btw?

---

Comments from Reviewable

[asajeffrey]

We have different opinions about coding style, quel surprise.

---

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

components/compositing/constellation.rs, line 717 [r1] (raw file):
Done.

---

components/compositing/constellation.rs, line 729 [r1] (raw file):
OK, I don't see why, doesn't the debug! count as a side-effect?

---

components/compositing/constellation.rs, line 908 [r1] (raw file):
Does your version pass the borrow checker?

---

components/compositing/constellation.rs, line 930 [r1] (raw file):
Ditto.

---

components/compositing/constellation.rs, line 1030 [r1] (raw file):
Done.

---

components/compositing/constellation.rs, line 1042 [r1] (raw file):
Done.

---

components/compositing/constellation.rs, line 1058 [r1] (raw file):
The problem with the nested-if idiom is that the code gradually drifts right, due to indentation. You also end up with spurious diffs whenever the indentation changes, which makes code maintenance annoying.

Also in this case, borowchk.

---

components/compositing/constellation.rs, line 1083 [r1] (raw file):
Done.

---

components/compositing/constellation.rs, line 1235 [r1] (raw file):
Ditto.

---

components/compositing/constellation.rs, line 1253 [r1] (raw file):
I prefer match to if let in the case that there's an else branch. The extra indentation makes the code easier to read, IMO. YMMV.

---

components/compositing/constellation.rs, line 1262 [r1] (raw file):
Ditto.

---

components/compositing/constellation.rs, line 1338 [r1] (raw file):
Ditto.

---

components/compositing/constellation.rs, line 1411 [r1] (raw file):
Ditto.

---

components/compositing/constellation.rs, line 1431 [r1] (raw file):
The API for the compositor proxy doesn't return a Result.

---

Comments from Reviewable

[asajeffrey]

@nox: I reverted some of your suggested changes, since I couldn't get them to pass borrowchk. Serves me right for checking in changes without compiling them!

[nox]

Review status: 0 of 1 files reviewed at latest revision, 14 unresolved discussions.

---

components/compositing/constellation.rs, line 717 [r1] (raw file):
So was there a borrowck problem here if you remove the intermediate result value?

---

components/compositing/constellation.rs, line 729 [r1] (raw file):
Because that's not the main point of that snippet, we do need the result value, which when the clipboard is broken is an empty string.

---

components/compositing/constellation.rs, line 908 [r1] (raw file):
I don't see what can make it fail. :/

---

Comments from Reviewable

[asajeffrey]

Review status: 0 of 1 files reviewed at latest revision, 14 unresolved discussions.

---

components/compositing/constellation.rs, line 717 [r1] (raw file):
Yes, the pipeline is borrowed from self.pipelines, which means self.handle_send_error can't be called whole pipeline is in scope.

---

components/compositing/constellation.rs, line 729 [r1] (raw file):
Done.

---

components/compositing/constellation.rs, line 908 [r1] (raw file):
Calling self.handle_send_error while pipeline is still in scope. Pipeline is borrowed from self.pipelines, and self.handle_send_error needs &mut self.

---

Comments from Reviewable

[asajeffrey]

Rebased and squashed.

[bors-servo]

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

[asajeffrey]

Rebased.

[bors-servo]

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

[asajeffrey]

Rebased. @nox: this is bitrotting.

[bors-servo]

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

[asajeffrey]

Another rebase. Serious bitrotting here :(

[nox]

Sorry about this @asajeffrey. Looks good to me with your last remarks and changes.

@bors-servo r+

[bors-servo]

📌 Commit 0cca4a9 has been approved by nox

[bors-servo]

⌛ Testing commit 0cca4a9 with merge 6c5cfb7...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

[asajeffrey]

Thanks!