#10141 : Document::location set null for documents without a browsing context

[slayerjain]

Fixes #10141.

This change is

[highfive]

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

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/webidls/Document.webidl, components/script/dom/document.rs

[highfive]

Warning

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

[Ms2ger]

Did you compile this locally?

[slayerjain]

oh.. my bad! switching my laptop created the mess!

[Ms2ger]

No problem; let me know when it works :)

[slayerjain]

The internet bandwidth in my country is really pathetic! couldn't build servo yet, it's been like 10 hours!

[slayerjain]

I've fixed my laptop's repo, but now I'm stuck with an error. I've also modified xmldocument.rs's location method to return Option<Root>

[Ms2ger]

Ah, I forgot about that. Attributes that can return null are called GetFoo rather than Foo, so you'll need to rename those two implementations.

[nox]

@slayerjain Please rebase your commit on top of master. There shouldn't be any merge commit in this branch. Feel free to ask questions if you don't know how to do this, either here or on IRC.

[slayerjain]

@nox Can you please explain the process, just to verify what i'm thinking? :)

[nox]

@slayerjain There needs to be only one commit on the branch, 04f6488, and there shouldn't be any merge on top of it.

So I update my local master branch from upstream, check out the branch first_bug, do git rebase -i master, remove the second line, save the file and quit, and let Git do its magic.

[KiChjang]

Here's documentation on how the entire workflow works.

[slayerjain]

@nox Awesome! on it! :)

[slayerjain]

@nox done! :)

[KiChjang]

This could be

self.browsing_context().and_then(|_|
    Some(self.location.or_init(|| Location::new(&self.window)))
)

[nox]

AFAICT it can just be:

self.browsing_context().map(|_| self.location.or_init(|| Location::new(&self.window)))

[KiChjang]

Ugh. I still can't tell when it's okay to use .map().

[frewsxcv]

Lame attempt at clarifying:

If you want the variant to go from Some → Some use map

If you want the variant to go from Some → (Some or None) use and_then

[bors-servo]

⌛ Testing commit 4de04bb with merge 59d205c...

[KiChjang]

It's just telling our buildbot to accept this PR on behalf of Ms2ger.

[slayerjain]

Didn't know that was possible, feels kinda odd.

[bors-servo]

💔 Test failed - mac-rel-wpt

[KiChjang]

Ran 4652 tests finished in 1073.0 seconds.
  • 4647 ran as expected. 1361 tests skipped.
  • 5 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /XMLHttpRequest/responsexml-document-properties.htm:
  └ PASS [expected FAIL] location

  ▶ Unexpected subtest result in /domparsing/DOMParser-parseFromString-html.html:
  └ PASS [expected FAIL] Location value

  ▶ Unexpected subtest result in /domparsing/DOMParser-parseFromString-xml.html:
  └ PASS [expected FAIL] Should parse correctly in type text/xml

  ▶ Unexpected subtest result in /html/browsers/history/the-location-interface/document_location.html:
  └ PASS [expected FAIL] document not in a browsing context

  ▶ Unexpected subtest result in /_mozilla/mozilla/windowproxy.html:
  └ PASS [expected FAIL] document.location is the right thing on non-rendered document

Looks like you'll need to update more test expectations.

[slayerjain]

@KiChjang where to make the changes?

[KiChjang]

Wait, you didn't squash your changes, you REMOVED his commit.

His commit had the diff and the files that you can use to update the test expectations, see slayerjain#1 for where to edit them.

[slayerjain]

@bors-servo try

[slayerjain]

i thought servo bot would run some tests.. :/

[KiChjang]

Unfortunately, bors-servo only listens to people with try access or reviewers. Sorry!

@bors-servo r+

[bors-servo]

📌 Commit 6626c5c has been approved by KiChjang

[bors-servo]

⌛ Testing commit 6626c5c with merge c33bf49...

[slayerjain]

@KiChjang can't wait to use it :)

[bors-servo]

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