Pipeline lookup in webdriver

[craftytrickster]

Fixes #11712

Removed a method that seemed to duplicate already existing functionality, and returned BrowsingContextErrors in the web_handler file where panics were previously occurring.

I am not sure if I like all the unwrapping that occurs in the script thread, but the current methods are not set up to return Option/Result.

Also, should line the method on line 37 find_node_by_unique_id of components/script/webdriver_handlers.rs return None if the context is not found like I have it currently? Or should it return a Result<Option...> instead?

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #11712 .

• There are tests for these changes OR
• These changes do not require tests because I simply removed a method that duplicated already existing functionality. On the other part, I added better error sending instead of forcing a panic, which does not require testing to my knowledge.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/webdriver_server/lib.rs
• @jgraham: components/webdriver_server/lib.rs
• @KiChjang: components/script/script_thread.rs, components/script/devtools.rs, components/script/webdriver_handlers.rs

[highfive]

Warning

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

[cbrewster]

This does have some overlap with #11695.

[craftytrickster]

Maybe I should rebase my changes over those ones, replacing my script thread file with the one from that PR. @asajeffrey do you agree?

[asajeffrey]

Sorry about the delay, Mozilla has been off in London last week!

It might be worth rebasing once #11695 has landed. There is some overlap between this PR and it.

Alternatively, we could have this PR leave get_browsing_context alone, and I'll rebase #11695 once this one lands.

[craftytrickster]

No worries, whatever you prefer.

[bors-servo]

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

[jdm]

@craftytrickster Could you rebase this, now that #11695 has merged?

[asajeffrey]

Reviewed the changes, mostly this much better, thanks! The only thing that's worth looking at is the remaining panics in devtools.rs. It would be nice to replace those with warnings.

---

Reviewed 2 of 3 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[craftytrickster]

Rebasing now, I will take a look at the comments and address them after.

[craftytrickster]

@asajeffrey I removed the unwraps, but I am not sure if the way I did it is preferable, is returning early and not panicking really a better solution? Should these methods maybe return a Result type?

[izgzhen]

@craftytrickster Maybe you can make reply a IpcSender<Result<NodeInfo, SomeErrorType>>? (I am not an expert on that, but it looks like no other places are using this message now so I think it is safe to change its type)

[Manishearth]

Can we return Results everywhere instead of panicking?

[Manishearth]

Everything should be returning a Result, which bubble up until the error can be sent down a channel.

[craftytrickster]

@Manishearth do you mean actually return a Result from the function, or do you mean what @izgzhen is saying?

[Manishearth]

Functions which do not send anything down a sender should use Result in their return type. Functions which do, should use Result in the sender, like you do here

[craftytrickster]

Thanks for the clarification. When I have time after work I will make the changes.

[asajeffrey]

We now have warnings included in issue reports, so whether or not we return a Result is about whether there's sensible error recovery we can do. If there is an error recovery action (e.g. returning an error via a webdriver or devtools channel) then returning a Result makes sense. If we're just going to include a warning if there's a subsequent error, then a warn! makes more sense.

[asajeffrey]

A nit about linebreaking style, otherwise lgtm. Squash and then r=me.

---

Reviewed 1 of 3 files at r7, 2 of 3 files at r8, 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

components/script/devtools.rs, line 73 [r9] (raw file):

Previously, izgzhen (Zhen Zhang) wrote…

@craftytrickster Maybe you can make reply a IpcSender<Result<NodeInfo, SomeErrorType>>? (I am not an expert on that, but it looks like no other places are using this message now so I think it is safe to change its type)

Using `Option` for lookups which may miss seems fine to me.

---

components/webdriver_server/lib.rs, line 744 [r10] (raw file):

            Err(WebDriverJSError::BrowsingContextNotFound) => Err(WebDriverError::new(
                ErrorStatus::JavascriptError, "Pipeline id not found in browsing context"
            ))

Nit: linebreaking style.

---

Comments from Reviewable

[craftytrickster]

@asajeffrey should I keep your initial commit and just squash my commits, or should I just squash them all?

[asajeffrey]

Squash everything.

[craftytrickster]

r=asajeffrey

[KiChjang]

@craftytrickster Sorry, bors-servo only listens to reviewers.

@bors-servo r=asajeffrey

[bors-servo]

📌 Commit f31ce88 has been approved by asajeffrey

[bors-servo]

⌛ Testing commit f31ce88 with merge 983c30d...

[bors-servo]

💔 Test failed - linux-dev

[jdm]

./components/script/devtools.rs:263: found an empty line following a {
./components/script/devtools.rs:279: found an empty line following a {

[craftytrickster]

Sorry about that, the tidy check has been fixed now.

[KiChjang]

@bors-servo r=asajeffrey

[bors-servo]

📌 Commit 2475dc1 has been approved by asajeffrey

[bors-servo]

⌛ Testing commit 2475dc1 with merge a94b92f...

[bors-servo]

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