Plumb Document to MediaQueryExpValue::Consume() [chromium/src : main]
Alison Maher (Gerrit)
Alison Maher voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Kevin Babbitt
- Rune Lillesveen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
const Document* document,If document is non-null here, isn't execution_context typically a LocalDOMWindow, and you can get the Document from that via DynamicTo<LocalDOMWindow>(execution_context) when creating the fake_context_ instead of plumbing it down here?
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 1 comment![]( "Open in Gerrit")
const Document* document,If document is non-null here, isn't execution_context typically a LocalDOMWindow, and you can get the Document from that via DynamicTo<LocalDOMWindow>(execution_context) when creating the fake_context_ instead of plumbing it down here?
Document isn't always guaranteed (although there were several call sites, so it could be that this one is guarenteed and others weren't). I tried to pass it in wherever it was available, though. From what I found, the execution context is also sometimes fake. As a result, I decided to pass this in separately, but I do think this could likely use some further cleanup at some point since it would be nice to guarantee both and then avoid passing one of them in.
Related details
- Kevin Babbitt
- Rune Lillesveen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
const Document* document,Alison MaherIf document is non-null here, isn't execution_context typically a LocalDOMWindow, and you can get the Document from that via DynamicTo<LocalDOMWindow>(execution_context) when creating the fake_context_ instead of plumbing it down here?
Document isn't always guaranteed (although there were several call sites, so it could be that this one is guarenteed and others weren't). I tried to pass it in wherever it was available, though. From what I found, the execution context is also sometimes fake. As a result, I decided to pass this in separately, but I do think this could likely use some further cleanup at some point since it would be nice to guarantee both and then avoid passing one of them in.
Where do we create fake ExecutionContexts? And in those cases, would we have a valid Document instead? It seems to me if we can pass in a valid Document, it would be trivial to pass a real ExecutionContext too?
If the ExecutionContext is an instance of LocalDOMWindow, and the passed in document is non-null, then getting the document from the LocalDOMWindow should match the document parameter, right?
Related details
- Alison Maher
- Kevin Babbitt
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher added 1 comment![]( "Open in Gerrit")
const Document* document,Alison MaherIf document is non-null here, isn't execution_context typically a LocalDOMWindow, and you can get the Document from that via DynamicTo<LocalDOMWindow>(execution_context) when creating the fake_context_ instead of plumbing it down here?
Rune LillesveenDocument isn't always guaranteed (although there were several call sites, so it could be that this one is guarenteed and others weren't). I tried to pass it in wherever it was available, though. From what I found, the execution context is also sometimes fake. As a result, I decided to pass this in separately, but I do think this could likely use some further cleanup at some point since it would be nice to guarantee both and then avoid passing one of them in.
Where do we create fake ExecutionContexts? And in those cases, would we have a valid Document instead? It seems to me if we can pass in a valid Document, it would be trivial to pass a real ExecutionContext too?
If the ExecutionContext is an instance of LocalDOMWindow, and the passed in document is non-null, then getting the document from the LocalDOMWindow should match the document parameter, right?
Just walked back all of the call sites and I think you are right. There are some cases where we pass in nullptr for the context, but we don't have access to the Document in those cases either. There was at least a few cases, example `ManifestManager::ParseManifestFromString()`, where we had a context, but it seemed to not be created via a Document [1]. But this wasn't a case of concern for what I was looking into.
I tried updating to what you had suggested in code, and we are able to still access the Document where we were hoping to, so this will simplify things drastically. Thanks for the super useful feedback. I'll get this cleaned up and re-uploaded.
Related details
- Kevin Babbitt
- Rune Lillesveen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |