Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upWebdriver browsing contexts not pipelines #16997
Conversation
highfive
commented
May 22, 2017
|
Heads up! This PR modifies the following files:
|
highfive
commented
May 22, 2017
|
The first commit is from #16916. |
|
r? @jgraham |
|
This is part of the ongoing effort to introduce top-level browsing context ids, in order to support multiple windows (#13994). cc @jdm @cbrewster @paulrouget |
|
IRC conversation with @jgraham: http://logs.glob.uno/?c=mozilla%23servo&s=22+May+2017&e=22+May+2017#c678807 and http://logs.glob.uno/?c=mozilla%23servo&s=22+May+2017&e=22+May+2017#c678945. TL;DR: argh testing! (I ended up driving servo by hand with curl, got it to navigate, traverse and execute a script.) |
|
#16916 has landed, so this PR is next in the queue. |
c472ff8
to
79743b5
|
@jgraham are you OK reviewing this or should I find another victim? |
|
I'm not an expert on all of this code, but the changes seem quite reasonable. Reviewed 12 of 12 files at r1. components/constellation/constellation.rs, line 906 at r1 (raw file):
This seems to be duplicated. components/script/dom/htmliframeelement.rs, line 501 at r1 (raw file):
Seems maybe unrelated? components/webdriver_server/lib.rs, line 785 at r1 (raw file):
It's not quite clear what "these" means in this comment. Comments from Reviewable |
|
Thanks! I made changes based on your comments. Review status: all files reviewed at latest revision, 3 unresolved discussions. components/constellation/constellation.rs, line 906 at r1 (raw file): Previously, jgraham wrote…
Oops. Fixed. components/script/dom/htmliframeelement.rs, line 501 at r1 (raw file): Previously, jgraham wrote…
OK, I reverted this change. components/webdriver_server/lib.rs, line 785 at r1 (raw file): Previously, jgraham wrote…
Fixed. Comments from Reviewable |
|
Reviewed 4 of 4 files at r2. Comments from Reviewable |
|
@bors-servo r+ |
|
|
…elines, r=jgraham Webdriver browsing contexts not pipelines <!-- Please describe your changes on the following line: --> At the moment, a webdriver session stores a `pipeline_id`s, which causes a mismatch with the spec, which asks a session to store a browsing context and a top-level browsing context. This PR fixes this mismatch. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because we are not testing webdriver <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16997) <!-- Reviewable:end -->
|
|
asajeffrey commentedMay 22, 2017
•
edited by larsbergstrom
At the moment, a webdriver session stores a
pipeline_ids, which causes a mismatch with the spec, which asks a session to store a browsing context and a top-level browsing context. This PR fixes this mismatch../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is