Webdriver - SetWindowSize/Close

[craftytrickster]

#8623

Please let me know if any changes are needed. Additionally, I noticed that many of the methods in lib.rs use the same code repetition in several places.

        let pipeline_id = try!(self.root_pipeline());

        let (sender, receiver) = ipc::channel().unwrap();

        let cmd_msg = .....;
        self.constellation_chan.send(ConstellationMsg::WebDriverCommand(cmd_msg)).unwrap();

Please let me know if I should refactor this bit of logic into a function in order to avoid code duplication.

Thanks.

[jdm]

Refactoring would be great. Feel free to file an issue or just make a PR for it :)

[Ms2ger]

r? @jgraham

[craftytrickster]

This is not related to the PR, more related to the refactoring suggesting:
@jdm After taking a look at the code for quite a bit, I do not see a great way to refactor this. Unfortunately, the IpcSender and IpcReceiver both have to be generated prior to forming the WebDriverScriptCommand enum (which requires the sender to be passed in).

The cleanest way I see to maybe do this would be to place the IpcSender on the WebDriverCommandMsg::ScriptCommand instead, but the way to do that would require the use of HKT.

The "best" available way to refactor this, (probably not worth it) would be to change from

        let pipeline_id = try!(self.root_pipeline());

        let (sender, receiver) = ipc::channel().unwrap();

        let cmd_msg = WebDriverCommandMsg::ScriptCommand(pipeline_id,
                                                         WebDriverScriptCommand::GetWindowSize(sender));
        self.constellation_chan.send(ConstellationMsg::WebDriverCommand(cmd_msg)).unwrap();

       match receiver.recv().unwrap() {}

to

   let (sender, receiver) = ipc::channel().unwrap();
   let command = WebDriverScriptCommand::GetWindowSize(sender);
   match new_method(command, receiver) {}


fn new_method(command: WebDriverScriptCommand, receiver: IpcReceiver) -> Result {
            let pipeline_id = try!(self.root_pipeline());
            let cmd_msg = WebDriverCommandMsg::ScriptCommand(pipeline_id, command);
            self.constellation_chan.send(ConstellationMsg::WebDriverCommand(cmd_msg)).unwrap();
            receiver.recv().unwrap()
}

[jgraham]

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

---

components/script/webdriver_handlers.rs, line 256 [r1] (raw file):
I think this should be handled in the constellation / compositor rather than in the script crate because in the current patch the command will return before the resize occurs, which isn't expected.

---

components/script/webdriver_handlers.rs, line 262 [r1] (raw file):
No need for this to return a JsValue::Null (or anything really; it can just send ().

---

components/script/webdriver_handlers.rs, line 267 [r1] (raw file):
Again it feels like this approach makes the synchronization off because the window will still actually be open when we return. I guess I'm less bothered here though since the session will end once the window gets closed.

---

components/webdriver_server/lib.rs, line 380 [r1] (raw file):
This should be a VoidResponse, but see https://www.w3.org/Bugs/Public/show_bug.cgi?id=29324.

---

components/webdriver_server/lib.rs, line 381 [r1] (raw file):
I don't think your implementation can actually generate this error at the moment, so it seems rather misleading to have this here.

---

components/webdriver_server/lib.rs, line 395 [r1] (raw file):
So this doesn't quite work; servo can only have one window at the moment, so closing it must end the session. You need to do the cleanup work to ensure that the session is shut down, and then return an empty list in the response.

---

components/webdriver_server/lib.rs, line 396 [r1] (raw file):
I don't think this can ever happen.

---

Comments from the review on Reviewable.io

[craftytrickster]

@jgraham Thank you for reviewing this. When I have time, I will make the appropriate changes.

[craftytrickster]

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

---

components/script/webdriver_handlers.rs, line 256 [r1] (raw file):
The window.ResizeTo method already goes through the compositor, as can be seen in its implementation in the window.rs file. Alternatively, instead of making another function for handle_set_window_size, I can use the method already specified (handle_resize_event) in the script_task.rs file. The only complicated thing there would be that I would have to populate my own WindowSizeData before sending my SetWindowSize message from lib.rs, which would not be ideal, since I would have to assume certain defaults (such as the scale factor).

---

Comments from the review on Reviewable.io

[craftytrickster]

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

---

components/script/webdriver_handlers.rs, line 256 [r1] (raw file):
The ResizeTo method already goes through the compositor, as can be seen in its implementation in the window.rs file. Alternatively, instead of making another function for handle_set_window_size, I can use the method already specified (handle_resize_event) in the script_task.rs file. The only complicated thing there would be that I would have to populate my own WindowSizeData before sending my SetWindowSize message from lib.rs, which is not ideal since I do not have the complete data information. What do you think?

---

components/webdriver_server/lib.rs, line 395 [r1] (raw file):
I am not sure how to handle cleanup, is it as simple as sending a FromCompositorMsg::Exit message to the constellation_chan?

---

Comments from the review on Reviewable.io

[jdm]

@craftytrickster Why'd you close this?

[craftytrickster]

I was unsure how to do several things here in the code, and I felt that the PR was not really in a good state to be merged. If you'd like I can reopen it.